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

Bart Van Assche
2017-08-25T16:27:17Z

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.