Re: [RFC 1/2] scsi: Provide mechanism for SCSI layer to poll for LLDD.

2017-03-17 Thread Bart Van Assche 
On Fri, 2017-03-17 at 18:55 +, Madhani, Himanshu wrote:
> On Mar 16, 2017, at 3:27 PM, Bart Van Assche  
> 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.

Re: [RFC 1/2] scsi: Provide mechanism for SCSI layer to poll for LLDD.

2017-03-17 Thread Madhani, Himanshu 
Hi Bart, 

> On Mar 16, 2017, at 3:27 PM, Bart Van Assche  
> 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

Re: [RFC 1/2] scsi: Provide mechanism for SCSI layer to poll for LLDD.

2017-03-16 Thread Bart Van Assche 
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.

Re: [RFC 1/2] scsi: Provide mechanism for SCSI layer to poll for LLDD.

2017-03-16 Thread Madhani, Himanshu 

> On Mar 16, 2017, at 3:27 PM, Bart Van Assche  
> 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

[RFC 1/2] scsi: Provide mechanism for SCSI layer to poll for LLDD.

2017-03-16 Thread Himanshu Madhani 
From: Darren Trapp 

Signed-off-by: Darren Trapp 
Signed-off-by: Himanshu Madhani 
---
 drivers/scsi/scsi_lib.c  | 39 +++
 include/scsi/scsi_host.h | 12 
 2 files changed, 51 insertions(+)

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
-- 
2.12.0