On Fri, 2017-08-25 at 17:49 +0200, Hannes Reinecke wrote:

On 08/23/2017 11:39 PM, Bart Van Assche wrote:

quoted

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.

quoted

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.