| Message ID | 1312361359-15445-11-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 3 August 2011 09:49, Paolo Bonzini <pbonzini@redhat.com> wrote: > @@ -157,8 +172,22 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun, > uint8_t *buf, void *hba_private) > { > SCSIRequest *req; > - req = d->info->alloc_req(d, tag, lun, hba_private); > - memcpy(req->cmd.buf, buf, 16); > + SCSICommand cmd; > + > + if (scsi_req_parse(&cmd, d, buf) != 0) { > + trace_scsi_req_parse_bad(d->id, lun, tag, buf[0]); > + req = scsi_req_alloc(&reqops_invalid_opcode, d, tag, lun, hba_private); > + } else { > + trace_scsi_req_parsed(d->id, lun, tag, buf[0], > + cmd.mode, cmd.xfer); > + if (req->cmd.lba != -1) { > + trace_scsi_req_parsed_lba(d->id, lun, tag, buf[0], > + cmd.lba); > + } > + req = d->info->alloc_req(d, tag, lun, hba_private); > + } > + > + req->cmd = cmd; > return req; > } This patch makes current master fail to compile with optimisation on: gcc complains: hw/scsi-bus.c: In function ‘scsi_req_new’: hw/scsi-bus.c:375: error: ‘req’ may be used uninitialized in this function because in the 'else' clause we look at req->cmd.lba before we've called alloc_req(). My guess is that the tracing should just be moved down to after the allocation? -- PMM
On 3 August 2011 09:49, Paolo Bonzini <pbonzini@redhat.com> wrote: > @@ -157,8 +172,22 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun, > uint8_t *buf, void *hba_private) > { > SCSIRequest *req; > - req = d->info->alloc_req(d, tag, lun, hba_private); > - memcpy(req->cmd.buf, buf, 16); > + SCSICommand cmd; > + > + if (scsi_req_parse(&cmd, d, buf) != 0) { > + trace_scsi_req_parse_bad(d->id, lun, tag, buf[0]); > + req = scsi_req_alloc(&reqops_invalid_opcode, d, tag, lun, hba_private); > + } else { > + trace_scsi_req_parsed(d->id, lun, tag, buf[0], > + cmd.mode, cmd.xfer); > + if (req->cmd.lba != -1) { > + trace_scsi_req_parsed_lba(d->id, lun, tag, buf[0], > + cmd.lba); > + } > + req = d->info->alloc_req(d, tag, lun, hba_private); > + } > + > + req->cmd = cmd; > return req; > } Does it still make sense to set req->cmd to cmd (and to look at cmd at all) in the case where scsi_req_parse() failed and might not have actually initialised all of cmd? For instance the tracing code (added to scsi_req_new() after this patch) looks at cmd.buf[] based on the value of buf[0], which seems kind of fragile to me. -- PMM
On 08/12/2011 06:12 PM, Peter Maydell wrote: > This patch makes current master fail to compile with optimisation on: > > gcc complains: > hw/scsi-bus.c: In function ‘scsi_req_new’: > hw/scsi-bus.c:375: error: ‘req’ may be used uninitialized in this function > > because in the 'else' clause we look at req->cmd.lba before we've > called alloc_req(). > > My guess is that the tracing should just be moved down to after the > allocation? You can also use cmd.lba instead of req->cmd.lba. I guess the failure depends on the compiler version and tracing options. Paolo
On 08/12/2011 06:55 PM, Peter Maydell wrote: > Does it still make sense to set req->cmd to cmd (and to look at cmd > at all) in the case where scsi_req_parse() failed and might not have > actually initialised all of cmd? For instance the tracing code (added > to scsi_req_new() after this patch) looks at cmd.buf[] based on the > value of buf[0], which seems kind of fragile to me. At the point tracing is reached, we know that cmd.buf[] has been initialized. But you're right that it is at least not tidy. We know that the size of the cdb is 16 (it is always like that, and we can make it a requirement). So we can copy it to cmd->buf before knowing cmd->len, at the beginning of scsi_req_parse. We can also zero unconditionally len/xfer/mode (plus set lba to -1) in case the parsing fails. Paolo
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c index 07d8704..01dbe02 100644 --- a/hw/scsi-bus.c +++ b/hw/scsi-bus.c @@ -7,6 +7,7 @@ #include "trace.h" static char *scsibus_get_fw_dev_path(DeviceState *dev); +static int scsi_req_parse(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf); static int scsi_build_sense(uint8_t *in_buf, int in_len, uint8_t *buf, int len, bool fixed); @@ -134,6 +135,20 @@ int scsi_bus_legacy_handle_cmdline(SCSIBus *bus) return res; } +/* SCSIReqOps implementation for invalid commands. */ + +static int32_t scsi_invalid_command(SCSIRequest *req, uint8_t *buf) +{ + scsi_req_build_sense(req, SENSE_CODE(INVALID_OPCODE)); + scsi_req_complete(req, CHECK_CONDITION); + return 0; +} + +struct SCSIReqOps reqops_invalid_opcode = { + .size = sizeof(SCSIRequest), + .send_command = scsi_invalid_command +}; + SCSIRequest *scsi_req_alloc(SCSIReqOps *reqops, SCSIDevice *d, uint32_t tag, uint32_t lun, void *hba_private) { @@ -157,8 +172,22 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun, uint8_t *buf, void *hba_private) { SCSIRequest *req; - req = d->info->alloc_req(d, tag, lun, hba_private); - memcpy(req->cmd.buf, buf, 16); + SCSICommand cmd; + + if (scsi_req_parse(&cmd, d, buf) != 0) { + trace_scsi_req_parse_bad(d->id, lun, tag, buf[0]); + req = scsi_req_alloc(&reqops_invalid_opcode, d, tag, lun, hba_private); + } else { + trace_scsi_req_parsed(d->id, lun, tag, buf[0], + cmd.mode, cmd.xfer); + if (req->cmd.lba != -1) { + trace_scsi_req_parsed_lba(d->id, lun, tag, buf[0], + cmd.lba); + } + req = d->info->alloc_req(d, tag, lun, hba_private); + } + + req->cmd = cmd; return req; } @@ -426,27 +455,21 @@ static uint64_t scsi_cmd_lba(SCSICommand *cmd) return lba; } -int scsi_req_parse(SCSIRequest *req, uint8_t *buf) +int scsi_req_parse(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf) { int rc; - if (req->dev->type == TYPE_TAPE) { - rc = scsi_req_stream_length(&req->cmd, req->dev, buf); + if (dev->type == TYPE_TAPE) { + rc = scsi_req_stream_length(cmd, dev, buf); } else { - rc = scsi_req_length(&req->cmd, req->dev, buf); + rc = scsi_req_length(cmd, dev, buf); } if (rc != 0) return rc; - assert(buf == req->cmd.buf); - scsi_cmd_xfer_mode(&req->cmd); - req->cmd.lba = scsi_cmd_lba(&req->cmd); - trace_scsi_req_parsed(req->dev->id, req->lun, req->tag, buf[0], - req->cmd.mode, req->cmd.xfer); - if (req->cmd.lba != -1) { - trace_scsi_req_parsed_lba(req->dev->id, req->lun, req->tag, buf[0], - req->cmd.lba); - } + memcpy(cmd->buf, buf, cmd->len); + scsi_cmd_xfer_mode(cmd); + cmd->lba = scsi_cmd_lba(cmd); return 0; } diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index ceb0e0a..64aa141 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -974,11 +974,6 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf) outbuf = (uint8_t *)r->iov.iov_base; DPRINTF("Command: lun=%d tag=0x%x data=0x%02x", req->lun, req->tag, buf[0]); - if (scsi_req_parse(&r->req, buf) != 0) { - BADF("Unsupported command length, command %x\n", command); - scsi_check_condition(r, SENSE_CODE(INVALID_OPCODE)); - return 0; - } #ifdef DEBUG_SCSI { int i; diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c index 699831a..e91c32e 100644 --- a/hw/scsi-generic.c +++ b/hw/scsi-generic.c @@ -84,10 +84,6 @@ static void scsi_command_complete(void *opaque, int ret) case -EDOM: status = TASK_SET_FULL; break; - case -EINVAL: - status = CHECK_CONDITION; - scsi_req_build_sense(&r->req, SENSE_CODE(INVALID_FIELD)); - break; case -ENOMEM: status = CHECK_CONDITION; scsi_req_build_sense(&r->req, SENSE_CODE(TARGET_FAILURE)); @@ -298,11 +294,6 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd) return 0; } - if (-1 == scsi_req_parse(&r->req, cmd)) { - BADF("Unsupported command length, command %x\n", cmd[0]); - scsi_command_complete(r, -EINVAL); - return 0; - } scsi_req_fixup(&r->req); DPRINTF("Command: lun=%d tag=0x%x len %zd data=0x%02x", lun, tag, diff --git a/hw/scsi.h b/hw/scsi.h index 5a1bbe2..dec814a 100644 --- a/hw/scsi.h +++ b/hw/scsi.h @@ -165,7 +165,6 @@ SCSIRequest *scsi_req_ref(SCSIRequest *req); void scsi_req_unref(SCSIRequest *req); void scsi_req_build_sense(SCSIRequest *req, SCSISense sense); -int scsi_req_parse(SCSIRequest *req, uint8_t *buf); void scsi_req_print(SCSIRequest *req); void scsi_req_continue(SCSIRequest *req); void scsi_req_data(SCSIRequest *req, int len);
Also introduce the first occurrence of "independent" SCSIReqOps, to handle invalid commands in common code. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/scsi-bus.c | 53 ++++++++++++++++++++++++++++++++++++++--------------- hw/scsi-disk.c | 5 ----- hw/scsi-generic.c | 9 --------- hw/scsi.h | 1 - 4 files changed, 38 insertions(+), 30 deletions(-)