| Message ID | 20190122185724.71598-1-bvanassche@acm.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix handling of bidi commands | expand |
On 2019-01-22 1:57 p.m., Bart Van Assche wrote: > Hi Martin, > > Recently Doug Gilbert reported that handling of bidi commands is broken > in the scsi-mq code. This patch series fixes that bug and also simplifies > bidi command handling. Please consider these patches for kernel v5.1. > > Thanks, > > Bart. > > Bart Van Assche (7): > Introduce the bidi_supported flag in the host template structure > Change scsi_cmnd.prot_sdb from a pointer into a regular member > Fix bidi handling > Introduce scsi_out_cmd() > Move several function definitions in <scsi/scsi_cmnd.h> > Introduce scsi_in_[sg]et_resid() and scsi_out_[sg]et_resid() > Move the resid member from struct scsi_data_buffer into struct > scsi_cmnd > > drivers/scsi/iscsi_tcp.c | 8 +--- > drivers/scsi/libiscsi.c | 12 ++--- > drivers/scsi/scsi_debug.c | 20 +++------ > drivers/scsi/scsi_lib.c | 70 ++++++++++++------------------ > drivers/scsi/sd.c | 4 +- > drivers/scsi/virtio_scsi.c | 4 +- > drivers/target/loopback/tcm_loop.c | 8 +--- > include/scsi/scsi_cmnd.h | 67 ++++++++++++++++++++-------- > include/scsi/scsi_host.h | 2 + > 9 files changed, 96 insertions(+), 99 deletions(-) > This patchset needs something like the following if UAS (USB Attached SCSI) is configured in your kernel. Beware of tabs/spaces/line_wraps as this is a cut and paste: diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 36742e8e7edc..24f3f95917a5 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -401,9 +401,9 @@ static void uas_data_cmplt(struct urb *urb) if (status != -ENOENT && status != -ECONNRESET && status != -ESHUTDOWN) uas_log_cmd_state(cmnd, "data cmplt err", status); /* error: no data transfered */ - sdb->resid = sdb->length; + scsi_in_set_resid(cmnd, sdb->length); } else { - sdb->resid = sdb->length - urb->actual_length; + scsi_in_set_resid(cmnd, sdb->length - urb->actual_length); } uas_try_complete(cmnd, __func__); out:
On Tue, 2019-01-22 at 18:30 -0500, Douglas Gilbert wrote: > This patchset needs something like the following if UAS (USB Attached > SCSI) is configured in your kernel. > > Beware of tabs/spaces/line_wraps as this is a cut and paste: > > diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c > index 36742e8e7edc..24f3f95917a5 100644 > --- a/drivers/usb/storage/uas.c > +++ b/drivers/usb/storage/uas.c > @@ -401,9 +401,9 @@ static void uas_data_cmplt(struct urb *urb) > if (status != -ENOENT && status != -ECONNRESET && status != > -ESHUTDOWN) > uas_log_cmd_state(cmnd, "data cmplt err", status); > /* error: no data transfered */ > - sdb->resid = sdb->length; > + scsi_in_set_resid(cmnd, sdb->length); > } else { > - sdb->resid = sdb->length - urb->actual_length; > + scsi_in_set_resid(cmnd, sdb->length - urb->actual_length); > } > uas_try_complete(cmnd, __func__); > out: Thanks Doug! I will fold a slightly modified version of this patch in. BTW, does this mean that you have been able to test this patch series? Bart.
On 2019-01-22 7:56 p.m., Bart Van Assche wrote: > On Tue, 2019-01-22 at 18:30 -0500, Douglas Gilbert wrote: >> This patchset needs something like the following if UAS (USB Attached >> SCSI) is configured in your kernel. >> >> Beware of tabs/spaces/line_wraps as this is a cut and paste: >> >> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c >> index 36742e8e7edc..24f3f95917a5 100644 >> --- a/drivers/usb/storage/uas.c >> +++ b/drivers/usb/storage/uas.c >> @@ -401,9 +401,9 @@ static void uas_data_cmplt(struct urb *urb) >> if (status != -ENOENT && status != -ECONNRESET && status != >> -ESHUTDOWN) >> uas_log_cmd_state(cmnd, "data cmplt err", status); >> /* error: no data transfered */ >> - sdb->resid = sdb->length; >> + scsi_in_set_resid(cmnd, sdb->length); >> } else { >> - sdb->resid = sdb->length - urb->actual_length; >> + scsi_in_set_resid(cmnd, sdb->length - urb->actual_length); >> } >> uas_try_complete(cmnd, __func__); >> out: > > Thanks Doug! I will fold a slightly modified version of this patch in. BTW, > does this mean that you have been able to test this patch series? Yes, but I didn't get far. With my sg v4 driver, scsi_debug and my sg_tst_bidi utility and this patchset after a short while I get a NULL pointer dereference in scsi_mq_prep_fn() inside the bidi conditional, probably the memset(). RIP ---> 0x2ce7 .... if (blk_bidi_rq(req)) { 2c9a: 49 8b 85 20 01 00 00 mov 0x120(%r13),%rax 2ca1: 48 85 c0 test %rax,%rax 2ca4: 74 60 je 2d06 <scsi_queue_rq+0x436> memset(&scsi_in_cmd(cmd)->sdb, 0, 2ca6: 48 c7 80 28 02 00 00 movq $0x0,0x228(%rax) 2cad: 00 00 00 00 2cb1: 48 c7 80 30 02 00 00 movq $0x0,0x230(%rax) 2cb8: 00 00 00 00 2cbc: 48 c7 80 38 02 00 00 movq $0x0,0x238(%rax) 2cc3: 00 00 00 00 2cc7: 49 8b 85 60 02 00 00 mov 0x260(%r13),%rax 2cce: 48 8b 90 20 01 00 00 mov 0x120(%rax),%rdx 2cd5: 48 8d 82 28 01 00 00 lea 0x128(%rdx),%rax 2cdc: 48 85 d2 test %rdx,%rdx 2cdf: 49 0f 44 c6 cmove %r14,%rax cmd->device->host->hostt->cmd_size; 2ce3: 48 8b 50 38 mov 0x38(%rax),%rdx 2ce7: 48 8b 12 mov (%rdx),%rdx <============== 2cea: 48 8b 92 98 00 00 00 mov 0x98(%rdx),%rdx 2cf1: 8b 92 30 01 00 00 mov 0x130(%rdx),%edx cmd->sdb.table.sgl = (void *)cmd + sizeof(struct scsi_cmnd) + 2cf7: 48 8d 94 10 b0 01 00 lea 0x1b0(%rax,%rdx,1),%rdx 2cfe: 00 2cff: 48 89 90 00 01 00 00 mov %rdx,0x100(%rax) blk_mq_start_request(req) ..... It might not be your patchset, but the location does look suspicious. Doug Gilbert
On Tue, 2019-01-22 at 23:49 -0500, Douglas Gilbert wrote: > On 2019-01-22 7:56 p.m., Bart Van Assche wrote: > > On Tue, 2019-01-22 at 18:30 -0500, Douglas Gilbert wrote: > > > This patchset needs something like the following if UAS (USB Attached > > > SCSI) is configured in your kernel. > > > > > > Beware of tabs/spaces/line_wraps as this is a cut and paste: > > > > > > diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c > > > index 36742e8e7edc..24f3f95917a5 100644 > > > --- a/drivers/usb/storage/uas.c > > > +++ b/drivers/usb/storage/uas.c > > > @@ -401,9 +401,9 @@ static void uas_data_cmplt(struct urb *urb) > > > if (status != -ENOENT && status != -ECONNRESET && status != > > > -ESHUTDOWN) > > > uas_log_cmd_state(cmnd, "data cmplt err", status); > > > /* error: no data transfered */ > > > - sdb->resid = sdb->length; > > > + scsi_in_set_resid(cmnd, sdb->length); > > > } else { > > > - sdb->resid = sdb->length - urb->actual_length; > > > + scsi_in_set_resid(cmnd, sdb->length - urb->actual_length); > > > } > > > uas_try_complete(cmnd, __func__); > > > out: > > > > Thanks Doug! I will fold a slightly modified version of this patch in. BTW, > > does this mean that you have been able to test this patch series? > > Yes, but I didn't get far. With my sg v4 driver, scsi_debug and my sg_tst_bidi > utility and this patchset after a short while I get a NULL pointer dereference > in scsi_mq_prep_fn() inside the bidi conditional, probably the memset(). > > RIP ---> 0x2ce7 > > .... > if (blk_bidi_rq(req)) { > 2c9a: 49 8b 85 20 01 00 00 mov 0x120(%r13),%rax > 2ca1: 48 85 c0 test %rax,%rax > 2ca4: 74 60 je 2d06 <scsi_queue_rq+0x436> > memset(&scsi_in_cmd(cmd)->sdb, 0, > 2ca6: 48 c7 80 28 02 00 00 movq $0x0,0x228(%rax) > 2cad: 00 00 00 00 > 2cb1: 48 c7 80 30 02 00 00 movq $0x0,0x230(%rax) > 2cb8: 00 00 00 00 > 2cbc: 48 c7 80 38 02 00 00 movq $0x0,0x238(%rax) > 2cc3: 00 00 00 00 > 2cc7: 49 8b 85 60 02 00 00 mov 0x260(%r13),%rax > 2cce: 48 8b 90 20 01 00 00 mov 0x120(%rax),%rdx > 2cd5: 48 8d 82 28 01 00 00 lea 0x128(%rdx),%rax > 2cdc: 48 85 d2 test %rdx,%rdx > 2cdf: 49 0f 44 c6 cmove %r14,%rax > cmd->device->host->hostt->cmd_size; > 2ce3: 48 8b 50 38 mov 0x38(%rax),%rdx > 2ce7: 48 8b 12 mov (%rdx),%rdx <============== > 2cea: 48 8b 92 98 00 00 00 mov 0x98(%rdx),%rdx > 2cf1: 8b 92 30 01 00 00 mov 0x130(%rdx),%edx > cmd->sdb.table.sgl = (void *)cmd + sizeof(struct scsi_cmnd) + > 2cf7: 48 8d 94 10 b0 01 00 lea 0x1b0(%rax,%rdx,1),%rdx > 2cfe: 00 > 2cff: 48 89 90 00 01 00 00 mov %rdx,0x100(%rax) > blk_mq_start_request(req) > ..... > > It might not be your patchset, but the location does look suspicious. Hi Doug, I think this means that the scsi_in_cmd(cmd)->device pointer is NULL. I will address this in the next version of this patch series. Thanks, Bart.
Hi Martin, Recently Doug Gilbert reported that handling of bidi commands is broken in the scsi-mq code. This patch series fixes that bug and also simplifies bidi command handling. Please consider these patches for kernel v5.1. Thanks, Bart. Bart Van Assche (7): Introduce the bidi_supported flag in the host template structure Change scsi_cmnd.prot_sdb from a pointer into a regular member Fix bidi handling Introduce scsi_out_cmd() Move several function definitions in <scsi/scsi_cmnd.h> Introduce scsi_in_[sg]et_resid() and scsi_out_[sg]et_resid() Move the resid member from struct scsi_data_buffer into struct scsi_cmnd drivers/scsi/iscsi_tcp.c | 8 +--- drivers/scsi/libiscsi.c | 12 ++--- drivers/scsi/scsi_debug.c | 20 +++------ drivers/scsi/scsi_lib.c | 70 ++++++++++++------------------ drivers/scsi/sd.c | 4 +- drivers/scsi/virtio_scsi.c | 4 +- drivers/target/loopback/tcm_loop.c | 8 +--- include/scsi/scsi_cmnd.h | 67 ++++++++++++++++++++-------- include/scsi/scsi_host.h | 2 + 9 files changed, 96 insertions(+), 99 deletions(-)