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.
