> -----Original Message----- > From: Hannes Reinecke [mailto:hare@xxxxxxx] > Sent: Thursday, 06 November, 2014 2:31 AM > To: James Bottomley > Cc: Christoph Hellwig; Ewan Milne; Elliott, Robert (Server Storage); > linux-scsi@xxxxxxxxxxxxxxx; Hannes Reinecke > Subject: [PATCH 05/10] scsi: use external buffer for command logging > ... > diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c > index 4f502f9..ed0d10d 100644 > --- a/drivers/scsi/ch.c > +++ b/drivers/scsi/ch.c > @@ -195,8 +195,10 @@ ch_do_scsi(scsi_changer *ch, unsigned char *cmd, > int cmd_len, > retry: > errno = 0; > if (debug) { > - DPRINTK("command: "); > - __scsi_print_command(cmd, cmd_len); > + char logbuf[SCSI_LOG_BUFSIZE]; > + > + __scsi_format_command(logbuf, SCSI_LOG_BUFSIZE, cmd, > cmd_len); Use sizeof(logbuf) rather than SCSI_LOG_BUFSIZE to avoid the macro name ever mismatching. > + DPRINTK("command: %s", logbuf); > } > > result = scsi_execute_req(ch->device, cmd, direction, buffer, ... > diff --git a/drivers/scsi/scsi_logging.c > b/drivers/scsi/scsi_logging.c > index dca45fe..d166d12 100644 > --- a/drivers/scsi/scsi_logging.c > +++ b/drivers/scsi/scsi_logging.c > @@ -13,10 +13,10 @@ > #include <scsi/scsi.h> > #include <scsi/scsi_cmnd.h> > #include <scsi/scsi_device.h> > +#include <scsi/scsi_eh.h> > #include <scsi/scsi_dbg.h> > > #define SCSI_LOG_SPOOLSIZE 4096 > -#define SCSI_LOG_BUFSIZE 128 > > struct scsi_log_buf { > char buffer[SCSI_LOG_SPOOLSIZE]; > @@ -64,6 +64,21 @@ static void scsi_log_release_buffer(char *bufptr) > preempt_enable(); > } > > +static size_t scmd_format_header(char *logbuf, size_t logbuf_len, > + struct gendisk *disk, int tag) > +{ > + size_t off = 0; > + > + if (disk) > + off += scnprintf(logbuf + off, logbuf_len - off, > + "[%s] ", disk->disk_name); > + > + if (tag >= 0) > + off += scnprintf(logbuf + off, logbuf_len - off, > + "tag#%d ", tag); If the first scnprintf consumes the full logbuf_len, then logbuf_len - off will cause an unsigned wrap (since it's using size_t types; it'd go negative if it used signed types), triggering this from vsnprintf (called by scnprintf): if (WARN_ON_ONCE((int) size < 0)) return 0; It might be prudent to check that it hasn't gone too far before calling scnprintf again. > + return off; > +} > + > int sdev_prefix_printk(const char *level, const struct scsi_device > *sdev, > const char *name, const char *fmt, ...) > { > @@ -106,13 +121,8 @@ int scmd_printk(const char *level, const struct > scsi_cmnd *scmd, > logbuf = scsi_log_reserve_buffer(&logbuf_len); > if (!logbuf) > return 0; > - if (disk) > - off += scnprintf(logbuf + off, logbuf_len - off, > - "[%s] ", disk->disk_name); > - > - if (scmd->request->tag >= 0) > - off += scnprintf(logbuf + off, logbuf_len - off, > - "tag#%d ", scmd->request->tag); > + off = scmd_format_header(logbuf, logbuf_len, disk, > + scmd->request->tag); > va_start(args, fmt); Same comment about ensuring off hasn't gone too far. > off += vscnprintf(logbuf + off, logbuf_len - off, fmt, args); > va_end(args); > @@ -121,3 +131,121 @@ int scmd_printk(const char *level, const struct > scsi_cmnd *scmd, > return ret; > } > EXPORT_SYMBOL_GPL(scmd_printk); > + > +static size_t scsi_format_opcode_name(char *buffer, size_t buf_len, > + const unsigned char *cdbp) > +{ > + int sa, cdb0; > + const char *cdb_name = NULL, *sa_name = NULL; > + size_t off; > + > + cdb0 = cdbp[0]; > + if (cdb0 == VARIABLE_LENGTH_CMD) { > + int len = scsi_varlen_cdb_length(cdbp); > + > + if (len < 10) { > + off = scnprintf(buffer, buf_len, > + "short variable length command, > len=%d", > + len); > + return off; > + } > + sa = (cdbp[8] << 8) + cdbp[9]; > + } else > + sa = cdbp[1] & 0x1f; > + > + if (!scsi_opcode_sa_name(cdb0, sa, &cdb_name, &sa_name)) { > + if (cdb_name) > + off = scnprintf(buffer, buf_len, "%s", cdb_name); > + else { > + off = scnprintf(buffer, buf_len, "cdb[0]=0x%x", > cdb0); Since the service action is printed using "sa=" below, consider using "op=" or "opcode=" rather than "cdb[0]=". That might be clearer to readers who don't know the CDB layout but do have an opcode table handy. > + if (cdb0 > VENDOR_SPECIFIC_CDB) The > should be >= since that define is 0xc0 (which itself is vendor-specific). > + off += scnprintf(buffer + off, buf_len - > off, > + " (vendor)"); > + else if (cdb0 > 0x60 && cdb0 < 0x7e) The > should be >= since 0x60 is reserved. The < is correct (0x7e is "extended CDB"). > + off += scnprintf(buffer + off, buf_len - > off, > + " (reserved)"); > + } > + } else { > + if (sa_name) > + off = scnprintf(buffer, buf_len, "%s", sa_name); > + else if (cdb_name) > + off = scnprintf(buffer, buf_len, "%s, sa=0x%x", > + cdb_name, sa); > + else > + off = scnprintf(buffer, buf_len, > + "cdb[0]=0x%x, sa=0x%x", cdb0, sa); > + } > + return off; > +} > + > +size_t __scsi_format_command(char *logbuf, size_t logbuf_len, > + const unsigned char *cdb, size_t cdb_len) > +{ > + int len, k; > + size_t off; > + > + off = scsi_format_opcode_name(logbuf, logbuf_len, cdb); > + len = scsi_command_size(cdb); > + if (cdb_len < len) > + len = cdb_len; > + /* print out all bytes in cdb */ > + for (k = 0; k < len; ++k) { > + if (off + 3 > logbuf_len) > + break; Might need a check for logbuf_len - off causing an unsigned wraparound problem before proceeding to scnprintf. > + off += scnprintf(logbuf + off, logbuf_len - off, > + " %02x", cdb[k]); > + } > + return off; > +} > +EXPORT_SYMBOL(__scsi_format_command); > + > +void scsi_print_command(struct scsi_cmnd *cmd) > +{ > + struct gendisk *disk = cmd->request->rq_disk; > + int k; > + char *logbuf; > + size_t off, logbuf_len; > + > + if (cmd->cmnd == NULL) > + return; Using !cmd->cmnd seems more common in SCSI midlayer code. > + > + logbuf = scsi_log_reserve_buffer(&logbuf_len); > + if (!logbuf) > + return; > + > + off = scmd_format_header(logbuf, logbuf_len, disk, cmd- > >request->tag); Repeat the logbuf_len - off wraparound concern. > + off += scnprintf(logbuf + off, logbuf_len - off, "CDB: "); Repeat the logbuf_len - off wraparound concern. > + off += scsi_format_opcode_name(logbuf + off, logbuf_len - off, > + cmd->cmnd); > + /* print out all bytes in cdb */ > + if (cmd->cmd_len > 16) { > + /* Print opcode in one line and use separate lines for > CDB */ > + off += scnprintf(logbuf + off, logbuf_len - off, "\n"); > + dev_printk(KERN_INFO, &cmd->device->sdev_gendev, logbuf); > + scsi_log_release_buffer(logbuf); > + for (k = 0; k < cmd->cmd_len; k += 16) { > + size_t linelen = min(cmd->cmd_len - k, 16); > + > + logbuf = scsi_log_reserve_buffer(&logbuf_len); > + if (!logbuf) > + break; > + off = scmd_format_header(logbuf, logbuf_len, disk, > + cmd->request->tag); Repeat the logbuf_len - off wraparound concern. > + off += scnprintf(logbuf + off, logbuf_len - off, > + "CDB[%02x]: ", k); Repeat the logbuf_len - off wraparound concern. > + hex_dump_to_buffer(&cmd->cmnd[k], linelen, 16, 1, > + logbuf + off, logbuf_len - off, > + false); > + dev_printk(KERN_INFO, &cmd->device->sdev_gendev, > + logbuf); > + scsi_log_release_buffer(logbuf); > + } > + } else { > + off += scnprintf(logbuf + off, logbuf_len - off, " "); Repeat the logbuf_len - off wraparound concern. > + hex_dump_to_buffer(cmd->cmnd, cmd->cmd_len, 16, 1, > + logbuf + off, logbuf_len - off, false); > + dev_printk(KERN_INFO, &cmd->device->sdev_gendev, logbuf); > + scsi_log_release_buffer(logbuf); > + } > +} > +EXPORT_SYMBOL(scsi_print_command); > diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c > index fb929fa..4f9fb3c 100644 > --- a/drivers/scsi/sr_ioctl.c > +++ b/drivers/scsi/sr_ioctl.c > @@ -188,6 +188,7 @@ int sr_do_ioctl(Scsi_CD *cd, struct > packet_command *cgc) > struct scsi_sense_hdr sshdr; > int result, err = 0, retries = 0; > struct request_sense *sense = cgc->sense; > + char logbuf[SCSI_LOG_BUFSIZE]; > > SDev = cd->device; > > @@ -257,14 +258,20 @@ int sr_do_ioctl(Scsi_CD *cd, struct > packet_command *cgc) > /* sense: Invalid command operation code */ > err = -EDRIVE_CANT_DO_THIS; > #ifdef DEBUG > - __scsi_print_command(cgc->cmd, CDROM_PACKET_SIZE); > + __scsi_format_command(logbuf, SCSI_LOG_BUFSIZE, > + cgc->cmd, CDROM_PACKET_SIZE); Use sizeof(logbuf) rather than SCSI_LOG_BUFSIZE to avoid the macro name ever mismatching. > + sr_printk(KERN_INFO, cd, > + "CDROM (ioctl) invalid command: %s\n", > + logbuf); > scsi_print_sense_hdr(cd->device, cd->cdi.name, > &sshdr); > #endif > break; > default: > + __scsi_format_command(logbuf, SCSI_LOG_BUFSIZE, > + cgc->cmd, CDROM_PACKET_SIZE); Use sizeof(logbuf) rather than SCSI_LOG_BUFSIZE to avoid the macro name ever mismatching. > sr_printk(KERN_ERR, cd, > - "CDROM (ioctl) error, command: "); > - __scsi_print_command(cgc->cmd, CDROM_PACKET_SIZE); > + "CDROM (ioctl) error, command: %s\n", > + logbuf); > scsi_print_sense_hdr(cd->device, cd->cdi.name, > &sshdr); > err = -EIO; > } ... -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
