Re: [PATCH] scsi_transport_srp: Fix shost to rport translation

[Bart Van Assche]

On Fri, 2018-04-13 at 08:11 +0200, Hannes Reinecke wrote:
> On Thu, 12 Apr 2018 13:32:07 -0600
> "Bart Van Assche"  wrote:
> > +static int find_child_rport(struct device *dev, void *data)
> > +{
> > +   struct device **child = data;
> > +
> > +   if (scsi_is_srp_rport(dev)) {
> > +   WARN_ON_ONCE(*child);
> > +   *child = dev;
> > +   }
> > +   return 0;
> > +}
> > +
> 
> Why not have 'static struct device *find_child_rport() ?

Hello Hannes,

The function device_for_each_child() expects to be passed a int (*)(struct 
device *,
void *) pointer. Is there perhaps another function for iterating over device 
children
that accepts a function that returns a pointer?

Thanks,

Bart.

Re: [PATCH] scsi_transport_srp: Fix shost to rport translation

[Hannes Reinecke]

On Thu, 12 Apr 2018 13:32:07 -0600
"Bart Van Assche"  wrote:

> Since an SRP remote port is attached as a child to shost->shost_gendev
> and as the only child, the translation from the shost pointer into an
> rport pointer must happen by looking up the shost child that is an
> rport. This patch fixes the following KASAN complaint:
> 
> BUG: KASAN: slab-out-of-bounds in srp_timed_out+0x57/0x110
> [scsi_transport_srp] Read of size 4 at addr 880035d3fcc0 by task
> kworker/1:0H/19
> 
> CPU: 1 PID: 19 Comm: kworker/1:0H Not tainted 4.16.0-rc3-dbg+ #1
> Workqueue: kblockd blk_mq_timeout_work
> Call Trace:
> dump_stack+0x85/0xc7
> print_address_description+0x65/0x270
> kasan_report+0x231/0x350
> srp_timed_out+0x57/0x110 [scsi_transport_srp]
> scsi_times_out+0xc7/0x3f0 [scsi_mod]
> blk_mq_terminate_expired+0xc2/0x140
> bt_iter+0xbc/0xd0
> blk_mq_queue_tag_busy_iter+0x1c7/0x350
> blk_mq_timeout_work+0x325/0x3f0
> process_one_work+0x441/0xa50
> worker_thread+0x76/0x6c0
> kthread+0x1b2/0x1d0
> ret_from_fork+0x24/0x30
> 
> Fixes: e68ca75200fe ("scsi_transport_srp: Reduce failover time")
> Signed-off-by: Bart Van Assche 
> Cc: Hannes Reinecke 
> Cc: Johannes Thumshirn 
> Cc: Jason Gunthorpe 
> Cc: Doug Ledford 
> Cc: Laurence Oberman 
> Cc: sta...@vger.kernel.org
> ---
>  drivers/scsi/scsi_transport_srp.c | 19 ++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_transport_srp.c
> b/drivers/scsi/scsi_transport_srp.c index 36f6190931bc..0d0515615847
> 100644 --- a/drivers/scsi/scsi_transport_srp.c
> +++ b/drivers/scsi/scsi_transport_srp.c
> @@ -51,6 +51,8 @@ struct srp_internal {
>   struct transport_container rport_attr_cont;
>  };
>  
> +static int scsi_is_srp_rport(const struct device *dev);
> +
>  #define to_srp_internal(tmpl) container_of(tmpl, struct
> srp_internal, t) 
>  #define  dev_to_rport(d) container_of(d, struct
> srp_rport, dev) @@ -60,9 +62,24 @@ static inline struct Scsi_Host
> *rport_to_shost(struct srp_rport *r) return
> dev_to_shost(r->dev.parent); }
>  
> +static int find_child_rport(struct device *dev, void *data)
> +{
> + struct device **child = data;
> +
> + if (scsi_is_srp_rport(dev)) {
> + WARN_ON_ONCE(*child);
> + *child = dev;
> + }
> + return 0;
> +}
> +
Huh?

Why not have 'static struct device *find_child_rport() ?

Cheers,

Hannes