| Message ID | 20170316214048.20481-2-himanshu.madhani@cavium.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thu, 2017-03-16 at 14:40 -0700, Himanshu Madhani wrote: > +static int > +scsi_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag) > + > +{ > + struct Scsi_Host *shost = hctx->driver_data; > + struct request *req; > + struct scsi_cmnd *cmd; > + > + req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag); > + if (!req) > + return 0; > + > + cmd = blk_mq_rq_to_pdu(req); > + if (!cmd) > + return 0; > + > + if (shost->hostt->poll_queue) > + return(shost->hostt->poll_queue(shost, cmd)); > + else return 0; > +} What will happen if the request with tag 'tag' is completed after the block layer decided to call this function and before this function had the chance to call blk_mq_tag_to_rq()? Will this trigger a kernel crash? Also, please format patches properly. This is what checkpatch reports for this patch: ERROR: code indent should use tabs where possible #244: FILE: drivers/scsi/scsi_lib.c:2161: + struct Scsi_Host *shost = hctx->driver_data;$ WARNING: please, no spaces at the start of a line #244: FILE: drivers/scsi/scsi_lib.c:2161: + struct Scsi_Host *shost = hctx->driver_data;$ ERROR: code indent should use tabs where possible #245: FILE: drivers/scsi/scsi_lib.c:2162: + struct request *req;$ WARNING: please, no spaces at the start of a line #245: FILE: drivers/scsi/scsi_lib.c:2162: + struct request *req;$ ERROR: code indent should use tabs where possible #246: FILE: drivers/scsi/scsi_lib.c:2163: + struct scsi_cmnd *cmd;$ WARNING: please, no spaces at the start of a line #246: FILE: drivers/scsi/scsi_lib.c:2163: + struct scsi_cmnd *cmd;$ ERROR: code indent should use tabs where possible #248: FILE: drivers/scsi/scsi_lib.c:2165: + req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);$ WARNING: please, no spaces at the start of a line #248: FILE: drivers/scsi/scsi_lib.c:2165: + req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);$ ERROR: code indent should use tabs where possible #249: FILE: drivers/scsi/scsi_lib.c:2166: + if (!req)$ WARNING: please, no spaces at the start of a line #249: FILE: drivers/scsi/scsi_lib.c:2166: + if (!req)$ ERROR: code indent should use tabs where possible #250: FILE: drivers/scsi/scsi_lib.c:2167: + return 0;$ WARNING: please, no spaces at the start of a line #250: FILE: drivers/scsi/scsi_lib.c:2167: + return 0;$ ERROR: code indent should use tabs where possible #252: FILE: drivers/scsi/scsi_lib.c:2169: + cmd = blk_mq_rq_to_pdu(req);$ WARNING: please, no spaces at the start of a line #252: FILE: drivers/scsi/scsi_lib.c:2169: + cmd = blk_mq_rq_to_pdu(req);$ ERROR: return is not a function, parentheses are not required #257: FILE: drivers/scsi/scsi_lib.c:2174: + return(shost->hostt->poll_queue(shost, cmd)); ERROR: trailing statements should be on next line #258: FILE: drivers/scsi/scsi_lib.c:2175: + else return 0; WARNING: line over 80 characters #262: FILE: drivers/scsi/scsi_lib.c:2179: +__scsi_init_hctx(struct blk_mq_hw_ctx *hctx, struct Scsi_Host *shost, unsigned int hctx_idx) WARNING: Unnecessary space before function pointer name #306: FILE: include/scsi/scsi_host.h:139: + int (* poll_queue)(struct Scsi_Host *, struct scsi_cmnd *); ERROR: space prohibited after that '*' (ctx:BxW) #306: FILE: include/scsi/scsi_host.h:139: + int (* poll_queue)(struct Scsi_Host *, struct scsi_cmnd *); ^ Thanks, Bart.
> On Mar 16, 2017, at 3:27 PM, Bart Van Assche <bart.vanassche@sandisk.com> wrote: > > On Thu, 2017-03-16 at 14:40 -0700, Himanshu Madhani wrote: >> +static int >> +scsi_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag) >> + >> +{ >> + struct Scsi_Host *shost = hctx->driver_data; >> + struct request *req; >> + struct scsi_cmnd *cmd; >> + >> + req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag); >> + if (!req) >> + return 0; >> + >> + cmd = blk_mq_rq_to_pdu(req); >> + if (!cmd) >> + return 0; >> + >> + if (shost->hostt->poll_queue) >> + return(shost->hostt->poll_queue(shost, cmd)); >> + else return 0; >> +} > > What will happen if the request with tag 'tag' is completed after the block > layer decided to call this function and before this function had the chance > to call blk_mq_tag_to_rq()? Will this trigger a kernel crash? Also, please > format patches properly. This is what checkpatch reports for this patch: > > ERROR: code indent should use tabs where possible > #244: FILE: drivers/scsi/scsi_lib.c:2161: > + struct Scsi_Host *shost = hctx->driver_data;$ > > WARNING: please, no spaces at the start of a line > #244: FILE: drivers/scsi/scsi_lib.c:2161: > + struct Scsi_Host *shost = hctx->driver_data;$ > > ERROR: code indent should use tabs where possible > #245: FILE: drivers/scsi/scsi_lib.c:2162: > + struct request *req;$ > > WARNING: please, no spaces at the start of a line > #245: FILE: drivers/scsi/scsi_lib.c:2162: > + struct request *req;$ > > ERROR: code indent should use tabs where possible > #246: FILE: drivers/scsi/scsi_lib.c:2163: > + struct scsi_cmnd *cmd;$ > > WARNING: please, no spaces at the start of a line > #246: FILE: drivers/scsi/scsi_lib.c:2163: > + struct scsi_cmnd *cmd;$ > > ERROR: code indent should use tabs where possible > #248: FILE: drivers/scsi/scsi_lib.c:2165: > + req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);$ > > WARNING: please, no spaces at the start of a line > #248: FILE: drivers/scsi/scsi_lib.c:2165: > + req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);$ > > ERROR: code indent should use tabs where possible > #249: FILE: drivers/scsi/scsi_lib.c:2166: > + if (!req)$ > > WARNING: please, no spaces at the start of a line > #249: FILE: drivers/scsi/scsi_lib.c:2166: > + if (!req)$ > > ERROR: code indent should use tabs where possible > #250: FILE: drivers/scsi/scsi_lib.c:2167: > + return 0;$ > > WARNING: please, no spaces at the start of a line > #250: FILE: drivers/scsi/scsi_lib.c:2167: > + return 0;$ > > ERROR: code indent should use tabs where possible > #252: FILE: drivers/scsi/scsi_lib.c:2169: > + cmd = blk_mq_rq_to_pdu(req);$ > > WARNING: please, no spaces at the start of a line > #252: FILE: drivers/scsi/scsi_lib.c:2169: > + cmd = blk_mq_rq_to_pdu(req);$ > > ERROR: return is not a function, parentheses are not required > #257: FILE: drivers/scsi/scsi_lib.c:2174: > + return(shost->hostt->poll_queue(shost, cmd)); > > ERROR: trailing statements should be on next line > #258: FILE: drivers/scsi/scsi_lib.c:2175: > + else return 0; > > WARNING: line over 80 characters > #262: FILE: drivers/scsi/scsi_lib.c:2179: > +__scsi_init_hctx(struct blk_mq_hw_ctx *hctx, struct Scsi_Host *shost, unsigned int hctx_idx) > > WARNING: Unnecessary space before function pointer name > #306: FILE: include/scsi/scsi_host.h:139: > + int (* poll_queue)(struct Scsi_Host *, struct scsi_cmnd *); > > ERROR: space prohibited after that '*' (ctx:BxW) > #306: FILE: include/scsi/scsi_host.h:139: > + int (* poll_queue)(struct Scsi_Host *, struct scsi_cmnd *); > ^ > Thanks, > > Bart. We’ll take care of Check patch issue in updated RFC. Thanks, - Himanshu
Hi Bart, > On Mar 16, 2017, at 3:27 PM, Bart Van Assche <bart.vanassche@sandisk.com> wrote: > > On Thu, 2017-03-16 at 14:40 -0700, Himanshu Madhani wrote: >> +static int >> +scsi_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag) >> + >> +{ >> + struct Scsi_Host *shost = hctx->driver_data; >> + struct request *req; >> + struct scsi_cmnd *cmd; >> + >> + req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag); >> + if (!req) >> + return 0; >> + >> + cmd = blk_mq_rq_to_pdu(req); >> + if (!cmd) >> + return 0; >> + >> + if (shost->hostt->poll_queue) >> + return(shost->hostt->poll_queue(shost, cmd)); >> + else return 0; >> +} > > What will happen if the request with tag 'tag' is completed after the block > layer decided to call this function and before this function had the chance > to call blk_mq_tag_to_rq()? Will this trigger a kernel crash? Also, please > format patches properly. This is what checkpatch reports for this patch: > Tag is passed into here by rq->tag When tag goes to blk_mq_tag_to_rq it just indexes to the rqs array using tag: struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, unsigned int tag) { if (tag < tags->nr_tags) { prefetch(tags->rqs[tag]); return tags->rqs[tag]; } return NULL; So if tag is invalid(too large), it returns NULL. Every step it validates it has a valid * before continuing. Worse case we poll for a completion that has already completed. We don’t think this will result into NULL pointer crash. > ERROR: code indent should use tabs where possible > #244: FILE: drivers/scsi/scsi_lib.c:2161: > + struct Scsi_Host *shost = hctx->driver_data;$ > > WARNING: please, no spaces at the start of a line > #244: FILE: drivers/scsi/scsi_lib.c:2161: > + struct Scsi_Host *shost = hctx->driver_data;$ > > ERROR: code indent should use tabs where possible > #245: FILE: drivers/scsi/scsi_lib.c:2162: > + struct request *req;$ > > WARNING: please, no spaces at the start of a line > #245: FILE: drivers/scsi/scsi_lib.c:2162: > + struct request *req;$ > > ERROR: code indent should use tabs where possible > #246: FILE: drivers/scsi/scsi_lib.c:2163: > + struct scsi_cmnd *cmd;$ > > WARNING: please, no spaces at the start of a line > #246: FILE: drivers/scsi/scsi_lib.c:2163: > + struct scsi_cmnd *cmd;$ > > ERROR: code indent should use tabs where possible > #248: FILE: drivers/scsi/scsi_lib.c:2165: > + req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);$ > > WARNING: please, no spaces at the start of a line > #248: FILE: drivers/scsi/scsi_lib.c:2165: > + req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);$ > > ERROR: code indent should use tabs where possible > #249: FILE: drivers/scsi/scsi_lib.c:2166: > + if (!req)$ > > WARNING: please, no spaces at the start of a line > #249: FILE: drivers/scsi/scsi_lib.c:2166: > + if (!req)$ > > ERROR: code indent should use tabs where possible > #250: FILE: drivers/scsi/scsi_lib.c:2167: > + return 0;$ > > WARNING: please, no spaces at the start of a line > #250: FILE: drivers/scsi/scsi_lib.c:2167: > + return 0;$ > > ERROR: code indent should use tabs where possible > #252: FILE: drivers/scsi/scsi_lib.c:2169: > + cmd = blk_mq_rq_to_pdu(req);$ > > WARNING: please, no spaces at the start of a line > #252: FILE: drivers/scsi/scsi_lib.c:2169: > + cmd = blk_mq_rq_to_pdu(req);$ > > ERROR: return is not a function, parentheses are not required > #257: FILE: drivers/scsi/scsi_lib.c:2174: > + return(shost->hostt->poll_queue(shost, cmd)); > > ERROR: trailing statements should be on next line > #258: FILE: drivers/scsi/scsi_lib.c:2175: > + else return 0; > > WARNING: line over 80 characters > #262: FILE: drivers/scsi/scsi_lib.c:2179: > +__scsi_init_hctx(struct blk_mq_hw_ctx *hctx, struct Scsi_Host *shost, unsigned int hctx_idx) > > WARNING: Unnecessary space before function pointer name > #306: FILE: include/scsi/scsi_host.h:139: > + int (* poll_queue)(struct Scsi_Host *, struct scsi_cmnd *); > > ERROR: space prohibited after that '*' (ctx:BxW) > #306: FILE: include/scsi/scsi_host.h:139: > + int (* poll_queue)(struct Scsi_Host *, struct scsi_cmnd *); > ^ > Thanks, > > Bart. Thanks, - Himanshu
On Fri, 2017-03-17 at 18:55 +0000, Madhani, Himanshu wrote: > On Mar 16, 2017, at 3:27 PM, Bart Van Assche <bart.vanassche@sandisk.com> wrote: > > On Thu, 2017-03-16 at 14:40 -0700, Himanshu Madhani wrote: > > > +static int > > > +scsi_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag) > > > + > > > +{ > > > + struct Scsi_Host *shost = hctx->driver_data; > > > + struct request *req; > > > + struct scsi_cmnd *cmd; > > > + > > > + req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag); > > > + if (!req) > > > + return 0; > > > + > > > + cmd = blk_mq_rq_to_pdu(req); > > > + if (!cmd) > > > + return 0; > > > + > > > + if (shost->hostt->poll_queue) > > > + return(shost->hostt->poll_queue(shost, cmd)); > > > + else return 0; > > > +} > > > > What will happen if the request with tag 'tag' is completed after the block > > layer decided to call this function and before this function had the chance > > to call blk_mq_tag_to_rq()? Will this trigger a kernel crash? Also, please > > format patches properly. This is what checkpatch reports for this patch: > > > > Tag is passed into here by rq->tag > > When tag goes to blk_mq_tag_to_rq it just indexes to the rqs array using tag: > > struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, unsigned int tag) > { > if (tag < tags->nr_tags) { > prefetch(tags->rqs[tag]); > return tags->rqs[tag]; > } > > return NULL; > > So if tag is invalid(too large), it returns NULL. Every step it validates it has a valid * before continuing. > > Worse case we poll for a completion that has already completed. > > We don’t think this will result into NULL pointer crash. Hello Himanshu, Have you noticed that the caller of scsi_poll(), namely blk_mq_poll(), does not use any kind of mechanism to prevent that the 'tag' argument is freed and reused while blk_mq_poll() is in progress? I think that a SCSI completion can occur while blk_mq_poll() is in progress. What will happen if a SCSI completion occurs concurrently with scsi_poll()? Will that trigger a use-after-free of the scsi_cmnd structure 'cmd' points at? If so, what will be the consequences? Because of this I'm not sure we should proceed with adding blk_mq_poll() support to the SCSI core. But if such support is added it's probably a much better choice to change the type of the second argument of .poll_queue() from struct scsi_cmnd * into int (the tag number). It is then the responsibility of SCSI LLD's to avoid that their .poll_queue() implementation triggers a use-after-free. Bart.
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 19125d72f322..eb01be039677 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -2154,6 +2154,43 @@ struct request_queue *scsi_alloc_queue(struct scsi_device *sdev) return q; } +static int +scsi_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag) + +{ + struct Scsi_Host *shost = hctx->driver_data; + struct request *req; + struct scsi_cmnd *cmd; + + req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag); + if (!req) + return 0; + + cmd = blk_mq_rq_to_pdu(req); + if (!cmd) + return 0; + + if (shost->hostt->poll_queue) + return(shost->hostt->poll_queue(shost, cmd)); + else return 0; +} + +static inline void +__scsi_init_hctx(struct blk_mq_hw_ctx *hctx, struct Scsi_Host *shost, unsigned int hctx_idx) +{ + hctx->driver_data = shost; +} + +static int +scsi_init_hctx(struct blk_mq_hw_ctx *hctx, void *data, unsigned int hctx_idx) +{ + struct Scsi_Host *shost = data; + + __scsi_init_hctx(hctx, shost, hctx_idx); + + return 0; +} + static struct blk_mq_ops scsi_mq_ops = { .queue_rq = scsi_queue_rq, .complete = scsi_softirq_done, @@ -2161,6 +2198,8 @@ static struct blk_mq_ops scsi_mq_ops = { .init_request = scsi_init_request, .exit_request = scsi_exit_request, .map_queues = scsi_map_queues, + .poll = scsi_poll, + .init_hctx = scsi_init_hctx, }; struct request_queue *scsi_mq_alloc_queue(struct scsi_device *sdev) diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index 3cd8c3bec638..e87e8a69a0df 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -127,6 +127,18 @@ struct scsi_host_template { int (* queuecommand)(struct Scsi_Host *, struct scsi_cmnd *); /* + * The poll_queue function allows the scsi layer to poll a + * LLDD for the completion of a specific scsi_cmnd (upon request + * from blk_mq). + * + * The LLDD returns 1 to indicate it completed the request or 0 + * otherwise. + * + * STATUS: OPTIONAL + */ + int (* poll_queue)(struct Scsi_Host *, struct scsi_cmnd *); + + /* * This is an error handling strategy routine. You don't need to * define one of these if you don't want to - there is a default * routine that is present that should work in most cases. For those