Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]

[Bart Van Assche <Bart.VanAssche@wdc.com>]

On Fri, 2017-08-25 at 17:49 +0200, Hannes Reinecke wrote:
> On 08/23/2017 11:39 PM, Bart Van Assche wrote:
> > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> > index 3d38c6d463b8..5bb15e698969 100644
> > --- a/drivers/scsi/scsi.c
> > +++ b/drivers/scsi/scsi.c
> > @@ -426,7 +426,7 @@ void scsi_attach_vpd(struct scsi_device *sdev)
> >  	int vpd_len = SCSI_VPD_PG_LEN;
> >  	int pg80_supported = 0;
> >  	int pg83_supported = 0;
> > -	unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
> > +	unsigned char *vpd_buf, *orig_vpd_buf = NULL;
> >  
> >  	if (!scsi_device_supports_vpd(sdev))
> >  		return;
> 
> Why drop the __rcu annotation here?
> vpd_buf and orig_vpd_buf should always be accessed via rcu pointers.
> Did I misunderstood the meaning of the __rcu annotation?

Hello Hannes,

The __rcu annotation must only be used for pointers that are accessed from
multiple threads. The vpd_buf pointer is a local variable that is only used
from a single context. The family of rcu_dereference() functions expects an
__rcu-annotated pointer as argument and returns a regular pointer. See e.g.
the example in Documentation/RCU/whatisRCU.txt.

> > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> > index 5ed473a87589..cf8a2088a9ba 100644
> > --- a/drivers/scsi/scsi_sysfs.c
> > +++ b/drivers/scsi/scsi_sysfs.c
> > @@ -456,8 +456,11 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
> >  	/* NULL queue means the device can't be used */
> >  	sdev->request_queue = NULL;
> >  
> > -	kfree(sdev->vpd_pg83);
> > -	kfree(sdev->vpd_pg80);
> > +	mutex_lock(&sdev->inquiry_mutex);
> > +	kfree(rcu_dereference(sdev->vpd_pg83));
> > +	kfree(rcu_dereference(sdev->vpd_pg80));
> > +	mutex_unlock(&sdev->inquiry_mutex);
> > +
> >  	kfree(sdev->inquiry);
> >  	kfree(sdev);
> 
> As indicated by Shane, I think using 'kfree_rcu()' here might not be a
> bad idea. You never know ...

I will make that change. Thanks for the reviews!

Bart.