[PATCH 0/2] Rework handling of scsi_device.vpd_pg8[03]

[PATCH 0/2] Rework handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

Hello Martin,

The conclusion of a recent discussion about the handling of SCSI device VPD
data is that the code should be reworked such that that data is freed through
kfree_rcu() instead of kfree(). The two patches in this series realize this
and also simplify the VPD code somewhat. Please consider these patches for
kernel v4.14.

Thanks,

Bart.

Bart Van Assche (2):
  Introduce scsi_get_vpd_buf()
  Rework handling of scsi_device.vpd_pg8[03]

 drivers/scsi/scsi.c        | 116 +++++++++++++++++++++------------------------
 drivers/scsi/scsi_lib.c    |  16 +++----
 drivers/scsi/scsi_sysfs.c  |  26 +++++++---
 include/scsi/scsi_device.h |  18 +++++--
 4 files changed, 95 insertions(+), 81 deletions(-)

-- 
2.14.1

[PATCH 1/2] Introduce scsi_get_vpd_buf()

[Bart Van Assche]

This patch does not change any functionality.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Shane M Seymour <shane.seymour@hpe.com>
---
 drivers/scsi/scsi.c | 96 +++++++++++++++++++++++++----------------------------
 1 file changed, 45 insertions(+), 51 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 3d38c6d463b8..775f609f89e2 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -411,6 +411,41 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
 }
 EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
 
+/**
+ * scsi_get_vpd_buf - Get Vital Product Data from a SCSI device
+ * @sdev: The device to ask
+ * @page: Which Vital Product Data to return
+ * @len: Upon success, the VPD length will be stored in *@len.
+ *
+ * Returns %NULL upon failure.
+ */
+static unsigned char *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page,
+				       int *len)
+{
+	unsigned char *vpd_buf;
+	int vpd_len = SCSI_VPD_PG_LEN, result;
+
+retry_pg:
+	vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
+	if (!vpd_buf)
+		return NULL;
+
+	result = scsi_vpd_inquiry(sdev, vpd_buf, page, vpd_len);
+	if (result < 0) {
+		kfree(vpd_buf);
+		return NULL;
+	}
+	if (result > vpd_len) {
+		vpd_len = result;
+		kfree(vpd_buf);
+		goto retry_pg;
+	}
+
+	*len = result;
+
+	return vpd_buf;
+}
+
 /**
  * scsi_attach_vpd - Attach Vital Product Data to a SCSI device structure
  * @sdev: The device to ask
@@ -422,8 +457,8 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
  */
 void scsi_attach_vpd(struct scsi_device *sdev)
 {
-	int result, i;
-	int vpd_len = SCSI_VPD_PG_LEN;
+	int i;
+	int vpd_len;
 	int pg80_supported = 0;
 	int pg83_supported = 0;
 	unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
@@ -431,51 +466,25 @@ void scsi_attach_vpd(struct scsi_device *sdev)
 	if (!scsi_device_supports_vpd(sdev))
 		return;
 
-retry_pg0:
-	vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
+	/* Ask for all the pages supported by this device */
+	vpd_buf = scsi_get_vpd_buf(sdev, 0, &vpd_len);
 	if (!vpd_buf)
 		return;
 
-	/* Ask for all the pages supported by this device */
-	result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
-	if (result < 0) {
-		kfree(vpd_buf);
-		return;
-	}
-	if (result > vpd_len) {
-		vpd_len = result;
-		kfree(vpd_buf);
-		goto retry_pg0;
-	}
-
-	for (i = 4; i < result; i++) {
+	for (i = 4; i < vpd_len; i++) {
 		if (vpd_buf[i] == 0x80)
 			pg80_supported = 1;
 		if (vpd_buf[i] == 0x83)
 			pg83_supported = 1;
 	}
 	kfree(vpd_buf);
-	vpd_len = SCSI_VPD_PG_LEN;
 
 	if (pg80_supported) {
-retry_pg80:
-		vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-		if (!vpd_buf)
-			return;
-
-		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
-		if (result < 0) {
-			kfree(vpd_buf);
-			return;
-		}
-		if (result > vpd_len) {
-			vpd_len = result;
-			kfree(vpd_buf);
-			goto retry_pg80;
-		}
+		vpd_buf = scsi_get_vpd_buf(sdev, 0x80, &vpd_len);
+
 		mutex_lock(&sdev->inquiry_mutex);
 		orig_vpd_buf = sdev->vpd_pg80;
-		sdev->vpd_pg80_len = result;
+		sdev->vpd_pg80_len = vpd_len;
 		rcu_assign_pointer(sdev->vpd_pg80, vpd_buf);
 		mutex_unlock(&sdev->inquiry_mutex);
 		synchronize_rcu();
@@ -483,28 +492,13 @@ void scsi_attach_vpd(struct scsi_device *sdev)
 			kfree(orig_vpd_buf);
 			orig_vpd_buf = NULL;
 		}
-		vpd_len = SCSI_VPD_PG_LEN;
 	}
 
 	if (pg83_supported) {
-retry_pg83:
-		vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-		if (!vpd_buf)
-			return;
-
-		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x83, vpd_len);
-		if (result < 0) {
-			kfree(vpd_buf);
-			return;
-		}
-		if (result > vpd_len) {
-			vpd_len = result;
-			kfree(vpd_buf);
-			goto retry_pg83;
-		}
+		vpd_buf = scsi_get_vpd_buf(sdev, 0x83, &vpd_len);
 		mutex_lock(&sdev->inquiry_mutex);
 		orig_vpd_buf = sdev->vpd_pg83;
-		sdev->vpd_pg83_len = result;
+		sdev->vpd_pg83_len = vpd_len;
 		rcu_assign_pointer(sdev->vpd_pg83, vpd_buf);
 		mutex_unlock(&sdev->inquiry_mutex);
 		synchronize_rcu();
-- 
2.14.1

[PATCH 2/2] Rework handling of scsi_device.vpd_pg8[03]

[Bart Van Assche]

Introduce struct scsi_vpd for the VPD page length, data and the
RCU head that will be used to free the VPD data. Use kfree_rcu()
instead of kfree() to free VPD data. Only annotate pointers that
are shared across threads with __rcu. Use rcu_dereference() when
dereferencing an RCU pointer. This patch suppresses about twenty
sparse complaints about the vpd_pg8[03] pointers.

Fixes: commit 09e2b0b14690 ("scsi: rescan VPD attributes")
Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Shane Seymour <shane.seymour@hpe.com>
---
 drivers/scsi/scsi.c        | 48 +++++++++++++++++++++-------------------------
 drivers/scsi/scsi_lib.c    | 16 ++++++++--------
 drivers/scsi/scsi_sysfs.c  | 26 +++++++++++++++++++------
 include/scsi/scsi_device.h | 18 +++++++++++++----
 4 files changed, 64 insertions(+), 44 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 775f609f89e2..1cf3aef0de8a 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -415,22 +415,20 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
  * scsi_get_vpd_buf - Get Vital Product Data from a SCSI device
  * @sdev: The device to ask
  * @page: Which Vital Product Data to return
- * @len: Upon success, the VPD length will be stored in *@len.
  *
  * Returns %NULL upon failure.
  */
-static unsigned char *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page,
-				       int *len)
+static struct scsi_vpd *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page)
 {
-	unsigned char *vpd_buf;
+	struct scsi_vpd *vpd_buf;
 	int vpd_len = SCSI_VPD_PG_LEN, result;
 
 retry_pg:
-	vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
+	vpd_buf = kmalloc(sizeof(*vpd_buf) + vpd_len, GFP_KERNEL);
 	if (!vpd_buf)
 		return NULL;
 
-	result = scsi_vpd_inquiry(sdev, vpd_buf, page, vpd_len);
+	result = scsi_vpd_inquiry(sdev, vpd_buf->data, page, vpd_len);
 	if (result < 0) {
 		kfree(vpd_buf);
 		return NULL;
@@ -441,7 +439,7 @@ static unsigned char *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page,
 		goto retry_pg;
 	}
 
-	*len = result;
+	vpd_buf->len = result;
 
 	return vpd_buf;
 }
@@ -458,52 +456,50 @@ static unsigned char *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page,
 void scsi_attach_vpd(struct scsi_device *sdev)
 {
 	int i;
-	int vpd_len;
 	int pg80_supported = 0;
 	int pg83_supported = 0;
-	unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
+	struct scsi_vpd *vpd_buf, *orig_vpd_buf;
 
 	if (!scsi_device_supports_vpd(sdev))
 		return;
 
 	/* Ask for all the pages supported by this device */
-	vpd_buf = scsi_get_vpd_buf(sdev, 0, &vpd_len);
+	vpd_buf = scsi_get_vpd_buf(sdev, 0);
 	if (!vpd_buf)
 		return;
 
-	for (i = 4; i < vpd_len; i++) {
-		if (vpd_buf[i] == 0x80)
+	for (i = 4; i < vpd_buf->len; i++) {
+		if (vpd_buf->data[i] == 0x80)
 			pg80_supported = 1;
-		if (vpd_buf[i] == 0x83)
+		if (vpd_buf->data[i] == 0x83)
 			pg83_supported = 1;
 	}
 	kfree(vpd_buf);
 
 	if (pg80_supported) {
-		vpd_buf = scsi_get_vpd_buf(sdev, 0x80, &vpd_len);
+		vpd_buf = scsi_get_vpd_buf(sdev, 0x80);
 
 		mutex_lock(&sdev->inquiry_mutex);
-		orig_vpd_buf = sdev->vpd_pg80;
-		sdev->vpd_pg80_len = vpd_len;
+		orig_vpd_buf = rcu_dereference_protected(sdev->vpd_pg80,
+					lockdep_is_held(&sdev->inquiry_mutex));
 		rcu_assign_pointer(sdev->vpd_pg80, vpd_buf);
 		mutex_unlock(&sdev->inquiry_mutex);
-		synchronize_rcu();
-		if (orig_vpd_buf) {
-			kfree(orig_vpd_buf);
-			orig_vpd_buf = NULL;
-		}
+
+		if (orig_vpd_buf)
+			kfree_rcu(orig_vpd_buf, rcu);
 	}
 
 	if (pg83_supported) {
-		vpd_buf = scsi_get_vpd_buf(sdev, 0x83, &vpd_len);
+		vpd_buf = scsi_get_vpd_buf(sdev, 0x83);
+
 		mutex_lock(&sdev->inquiry_mutex);
-		orig_vpd_buf = sdev->vpd_pg83;
-		sdev->vpd_pg83_len = vpd_len;
+		orig_vpd_buf = rcu_dereference_protected(sdev->vpd_pg83,
+					lockdep_is_held(&sdev->inquiry_mutex));
 		rcu_assign_pointer(sdev->vpd_pg83, vpd_buf);
 		mutex_unlock(&sdev->inquiry_mutex);
-		synchronize_rcu();
+
 		if (orig_vpd_buf)
-			kfree(orig_vpd_buf);
+			kfree_rcu(orig_vpd_buf, rcu);
 	}
 }
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 813d2456ae93..3d6e50087600 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3276,8 +3276,8 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
 {
 	u8 cur_id_type = 0xff;
 	u8 cur_id_size = 0;
-	unsigned char *d, *cur_id_str;
-	unsigned char __rcu *vpd_pg83;
+	const unsigned char *d, *cur_id_str;
+	const struct scsi_vpd *vpd_pg83;
 	int id_size = -EINVAL;
 
 	rcu_read_lock();
@@ -3308,8 +3308,8 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
 	}
 
 	memset(id, 0, id_len);
-	d = vpd_pg83 + 4;
-	while (d < vpd_pg83 + sdev->vpd_pg83_len) {
+	d = vpd_pg83->data + 4;
+	while (d < vpd_pg83->data + vpd_pg83->len) {
 		/* Skip designators not referring to the LUN */
 		if ((d[1] & 0x30) != 0x00)
 			goto next_desig;
@@ -3425,8 +3425,8 @@ EXPORT_SYMBOL(scsi_vpd_lun_id);
  */
 int scsi_vpd_tpg_id(struct scsi_device *sdev, int *rel_id)
 {
-	unsigned char *d;
-	unsigned char __rcu *vpd_pg83;
+	const unsigned char *d;
+	const struct scsi_vpd *vpd_pg83;
 	int group_id = -EAGAIN, rel_port = -1;
 
 	rcu_read_lock();
@@ -3436,8 +3436,8 @@ int scsi_vpd_tpg_id(struct scsi_device *sdev, int *rel_id)
 		return -ENXIO;
 	}
 
-	d = sdev->vpd_pg83 + 4;
-	while (d < sdev->vpd_pg83 + sdev->vpd_pg83_len) {
+	d = vpd_pg83->data + 4;
+	while (d < vpd_pg83->data + vpd_pg83->len) {
 		switch (d[1] & 0xf) {
 		case 0x4:
 			/* Relative target port */
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 5ed473a87589..8a75e0f10eeb 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -428,6 +428,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	struct scsi_device *sdev;
 	struct device *parent;
 	struct list_head *this, *tmp;
+	struct scsi_vpd *vpd_pg80, *vpd_pg83;
 	unsigned long flags;
 
 	sdev = container_of(work, struct scsi_device, ew.work);
@@ -456,8 +457,19 @@ 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);
+	vpd_pg80 = rcu_dereference_protected(sdev->vpd_pg80,
+				lockdep_is_held(&sdev->inquiry_mutex));
+	rcu_assign_pointer(sdev->vpd_pg80, NULL);
+	vpd_pg83 = rcu_dereference_protected(sdev->vpd_pg83,
+				lockdep_is_held(&sdev->inquiry_mutex));
+	rcu_assign_pointer(sdev->vpd_pg83, NULL);
+	mutex_unlock(&sdev->inquiry_mutex);
+
+	if (vpd_pg83)
+		kfree_rcu(vpd_pg83, rcu);
+	if (vpd_pg80)
+		kfree_rcu(vpd_pg80, rcu);
 	kfree(sdev->inquiry);
 	kfree(sdev);
 
@@ -795,15 +807,17 @@ show_vpd_##_page(struct file *filp, struct kobject *kobj,	\
 {									\
 	struct device *dev = container_of(kobj, struct device, kobj);	\
 	struct scsi_device *sdev = to_scsi_device(dev);			\
+	struct scsi_vpd *vpd_page;					\
 	int ret;							\
+									\
 	if (!sdev->vpd_##_page)						\
 		return -EINVAL;						\
 	rcu_read_lock();						\
-	ret = memory_read_from_buffer(buf, count, &off,			\
-				      rcu_dereference(sdev->vpd_##_page), \
-				       sdev->vpd_##_page##_len);	\
+	vpd_page = rcu_dereference(sdev->vpd_##_page);			\
+	ret = memory_read_from_buffer(buf, count, &off, vpd_page->data, \
+				      vpd_page->len);			\
 	rcu_read_unlock();						\
-	return ret;						\
+	return ret;							\
 }									\
 static struct bin_attribute dev_attr_vpd_##_page = {		\
 	.attr =	{.name = __stringify(vpd_##_page), .mode = S_IRUGO },	\
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index f054f3f43c75..82e93ee94708 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -80,6 +80,18 @@ struct scsi_event {
 	 */
 };
 
+/**
+ * struct scsi_vpd - SCSI Vital Product Data
+ * @rcu: For kfree_rcu().
+ * @len: Length in bytes of @data.
+ * @data: VPD data as defined in various T10 SCSI standard documents.
+ */
+struct scsi_vpd {
+	struct rcu_head	rcu;
+	int		len;
+	unsigned char	data[];
+};
+
 struct scsi_device {
 	struct Scsi_Host *host;
 	struct request_queue *request_queue;
@@ -122,10 +134,8 @@ struct scsi_device {
 	const char * rev;		/* ... "nullnullnullnull" before scan */
 
 #define SCSI_VPD_PG_LEN                255
-	int vpd_pg83_len;
-	unsigned char __rcu *vpd_pg83;
-	int vpd_pg80_len;
-	unsigned char __rcu *vpd_pg80;
+	struct scsi_vpd __rcu *vpd_pg83;
+	struct scsi_vpd __rcu *vpd_pg80;
 	unsigned char current_tag;	/* current tag */
 	struct scsi_target      *sdev_target;   /* used only for single_lun */
 
-- 
2.14.1

RE: [PATCH 1/2] Introduce scsi_get_vpd_buf()

[Seymour, Shane M]

Reviewed-by: Shane Seymour <shane.seymour@hpe.com>

> -----Original Message-----
> From: Bart Van Assche [mailto:bart.vanassche@wdc.com]
> Sent: Saturday, August 26, 2017 6:36 AM
> To: Martin K . Petersen <martin.petersen@oracle.com>; James E . J .
> Bottomley <jejb@linux.vnet.ibm.com>
> Cc: linux-scsi@vger.kernel.org; Bart Van Assche <bart.vanassche@wdc.com>;
> Christoph Hellwig <hch@lst.de>; Hannes Reinecke <hare@suse.de>;
> Johannes Thumshirn <jthumshirn@suse.de>; Seymour, Shane M
> <shane.seymour@hpe.com>
> Subject: [PATCH 1/2] Introduce scsi_get_vpd_buf()
> 
> This patch does not change any functionality.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> Cc: Shane M Seymour <shane.seymour@hpe.com>
> ---
>  drivers/scsi/scsi.c | 96 +++++++++++++++++++++++++--------------------------
> --
>  1 file changed, 45 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 3d38c6d463b8..775f609f89e2 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -411,6 +411,41 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8
> page, unsigned char *buf,
>  }
>  EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
> 
> +/**
> + * scsi_get_vpd_buf - Get Vital Product Data from a SCSI device
> + * @sdev: The device to ask
> + * @page: Which Vital Product Data to return
> + * @len: Upon success, the VPD length will be stored in *@len.
> + *
> + * Returns %NULL upon failure.
> + */
> +static unsigned char *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page,
> +				       int *len)
> +{
> +	unsigned char *vpd_buf;
> +	int vpd_len = SCSI_VPD_PG_LEN, result;
> +
> +retry_pg:
> +	vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
> +	if (!vpd_buf)
> +		return NULL;
> +
> +	result = scsi_vpd_inquiry(sdev, vpd_buf, page, vpd_len);
> +	if (result < 0) {
> +		kfree(vpd_buf);
> +		return NULL;
> +	}
> +	if (result > vpd_len) {
> +		vpd_len = result;
> +		kfree(vpd_buf);
> +		goto retry_pg;
> +	}
> +
> +	*len = result;
> +
> +	return vpd_buf;
> +}
> +
>  /**
>   * scsi_attach_vpd - Attach Vital Product Data to a SCSI device structure
>   * @sdev: The device to ask
> @@ -422,8 +457,8 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
>   */
>  void scsi_attach_vpd(struct scsi_device *sdev)
>  {
> -	int result, i;
> -	int vpd_len = SCSI_VPD_PG_LEN;
> +	int i;
> +	int vpd_len;
>  	int pg80_supported = 0;
>  	int pg83_supported = 0;
>  	unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
> @@ -431,51 +466,25 @@ void scsi_attach_vpd(struct scsi_device *sdev)
>  	if (!scsi_device_supports_vpd(sdev))
>  		return;
> 
> -retry_pg0:
> -	vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
> +	/* Ask for all the pages supported by this device */
> +	vpd_buf = scsi_get_vpd_buf(sdev, 0, &vpd_len);
>  	if (!vpd_buf)
>  		return;
> 
> -	/* Ask for all the pages supported by this device */
> -	result = scsi_vpd_inquiry(sdev, vpd_buf, 0, vpd_len);
> -	if (result < 0) {
> -		kfree(vpd_buf);
> -		return;
> -	}
> -	if (result > vpd_len) {
> -		vpd_len = result;
> -		kfree(vpd_buf);
> -		goto retry_pg0;
> -	}
> -
> -	for (i = 4; i < result; i++) {
> +	for (i = 4; i < vpd_len; i++) {
>  		if (vpd_buf[i] == 0x80)
>  			pg80_supported = 1;
>  		if (vpd_buf[i] == 0x83)
>  			pg83_supported = 1;
>  	}
>  	kfree(vpd_buf);
> -	vpd_len = SCSI_VPD_PG_LEN;
> 
>  	if (pg80_supported) {
> -retry_pg80:
> -		vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
> -		if (!vpd_buf)
> -			return;
> -
> -		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
> -		if (result < 0) {
> -			kfree(vpd_buf);
> -			return;
> -		}
> -		if (result > vpd_len) {
> -			vpd_len = result;
> -			kfree(vpd_buf);
> -			goto retry_pg80;
> -		}
> +		vpd_buf = scsi_get_vpd_buf(sdev, 0x80, &vpd_len);
> +
>  		mutex_lock(&sdev->inquiry_mutex);
>  		orig_vpd_buf = sdev->vpd_pg80;
> -		sdev->vpd_pg80_len = result;
> +		sdev->vpd_pg80_len = vpd_len;
>  		rcu_assign_pointer(sdev->vpd_pg80, vpd_buf);
>  		mutex_unlock(&sdev->inquiry_mutex);
>  		synchronize_rcu();
> @@ -483,28 +492,13 @@ void scsi_attach_vpd(struct scsi_device *sdev)
>  			kfree(orig_vpd_buf);
>  			orig_vpd_buf = NULL;
>  		}
> -		vpd_len = SCSI_VPD_PG_LEN;
>  	}
> 
>  	if (pg83_supported) {
> -retry_pg83:
> -		vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
> -		if (!vpd_buf)
> -			return;
> -
> -		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x83, vpd_len);
> -		if (result < 0) {
> -			kfree(vpd_buf);
> -			return;
> -		}
> -		if (result > vpd_len) {
> -			vpd_len = result;
> -			kfree(vpd_buf);
> -			goto retry_pg83;
> -		}
> +		vpd_buf = scsi_get_vpd_buf(sdev, 0x83, &vpd_len);
>  		mutex_lock(&sdev->inquiry_mutex);
>  		orig_vpd_buf = sdev->vpd_pg83;
> -		sdev->vpd_pg83_len = result;
> +		sdev->vpd_pg83_len = vpd_len;
>  		rcu_assign_pointer(sdev->vpd_pg83, vpd_buf);
>  		mutex_unlock(&sdev->inquiry_mutex);
>  		synchronize_rcu();
> --
> 2.14.1

RE: [PATCH 2/2] Rework handling of scsi_device.vpd_pg8[03]

[Seymour, Shane M]

Hi Bart,

Comments inline below about the show_vpd_##_page macro.

> -----Original Message-----
> From: Bart Van Assche [mailto:bart.vanassche@wdc.com]
> Sent: Saturday, August 26, 2017 6:36 AM
> To: Martin K . Petersen <martin.petersen@oracle.com>; James E . J .
> Bottomley <jejb@linux.vnet.ibm.com>
> Cc: linux-scsi@vger.kernel.org; Bart Van Assche <bart.vanassche@wdc.com>;
> Christoph Hellwig <hch@lst.de>; Hannes Reinecke <hare@suse.de>;
> Johannes Thumshirn <jthumshirn@suse.de>; Seymour, Shane M
> <shane.seymour@hpe.com>
> Subject: [PATCH 2/2] Rework handling of scsi_device.vpd_pg8[03]
> 
> Introduce struct scsi_vpd for the VPD page length, data and the
> RCU head that will be used to free the VPD data. Use kfree_rcu()
> instead of kfree() to free VPD data. Only annotate pointers that
> are shared across threads with __rcu. Use rcu_dereference() when
> dereferencing an RCU pointer. This patch suppresses about twenty
> sparse complaints about the vpd_pg8[03] pointers.
> 
> Fixes: commit 09e2b0b14690 ("scsi: rescan VPD attributes")
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> Cc: Shane Seymour <shane.seymour@hpe.com>
> ---
>  drivers/scsi/scsi.c        | 48 +++++++++++++++++++++-------------------------
>  drivers/scsi/scsi_lib.c    | 16 ++++++++--------
>  drivers/scsi/scsi_sysfs.c  | 26 +++++++++++++++++++------
>  include/scsi/scsi_device.h | 18 +++++++++++++----
>  4 files changed, 64 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 775f609f89e2..1cf3aef0de8a 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -415,22 +415,20 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
>   * scsi_get_vpd_buf - Get Vital Product Data from a SCSI device
>   * @sdev: The device to ask
>   * @page: Which Vital Product Data to return
> - * @len: Upon success, the VPD length will be stored in *@len.
>   *
>   * Returns %NULL upon failure.
>   */
> -static unsigned char *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page,
> -				       int *len)
> +static struct scsi_vpd *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page)
>  {
> -	unsigned char *vpd_buf;
> +	struct scsi_vpd *vpd_buf;
>  	int vpd_len = SCSI_VPD_PG_LEN, result;
> 
>  retry_pg:
> -	vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
> +	vpd_buf = kmalloc(sizeof(*vpd_buf) + vpd_len, GFP_KERNEL);
>  	if (!vpd_buf)
>  		return NULL;
> 
> -	result = scsi_vpd_inquiry(sdev, vpd_buf, page, vpd_len);
> +	result = scsi_vpd_inquiry(sdev, vpd_buf->data, page, vpd_len);
>  	if (result < 0) {
>  		kfree(vpd_buf);
>  		return NULL;
> @@ -441,7 +439,7 @@ static unsigned char *scsi_get_vpd_buf(struct
> scsi_device *sdev, u8 page,
>  		goto retry_pg;
>  	}
> 
> -	*len = result;
> +	vpd_buf->len = result;
> 
>  	return vpd_buf;
>  }
> @@ -458,52 +456,50 @@ static unsigned char *scsi_get_vpd_buf(struct
> scsi_device *sdev, u8 page,
>  void scsi_attach_vpd(struct scsi_device *sdev)
>  {
>  	int i;
> -	int vpd_len;
>  	int pg80_supported = 0;
>  	int pg83_supported = 0;
> -	unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
> +	struct scsi_vpd *vpd_buf, *orig_vpd_buf;
> 
>  	if (!scsi_device_supports_vpd(sdev))
>  		return;
> 
>  	/* Ask for all the pages supported by this device */
> -	vpd_buf = scsi_get_vpd_buf(sdev, 0, &vpd_len);
> +	vpd_buf = scsi_get_vpd_buf(sdev, 0);
>  	if (!vpd_buf)
>  		return;
> 
> -	for (i = 4; i < vpd_len; i++) {
> -		if (vpd_buf[i] == 0x80)
> +	for (i = 4; i < vpd_buf->len; i++) {
> +		if (vpd_buf->data[i] == 0x80)
>  			pg80_supported = 1;
> -		if (vpd_buf[i] == 0x83)
> +		if (vpd_buf->data[i] == 0x83)
>  			pg83_supported = 1;
>  	}
>  	kfree(vpd_buf);
> 
>  	if (pg80_supported) {
> -		vpd_buf = scsi_get_vpd_buf(sdev, 0x80, &vpd_len);
> +		vpd_buf = scsi_get_vpd_buf(sdev, 0x80);
> 
>  		mutex_lock(&sdev->inquiry_mutex);
> -		orig_vpd_buf = sdev->vpd_pg80;
> -		sdev->vpd_pg80_len = vpd_len;
> +		orig_vpd_buf = rcu_dereference_protected(sdev-
> >vpd_pg80,
> +					lockdep_is_held(&sdev-
> >inquiry_mutex));
>  		rcu_assign_pointer(sdev->vpd_pg80, vpd_buf);
>  		mutex_unlock(&sdev->inquiry_mutex);
> -		synchronize_rcu();
> -		if (orig_vpd_buf) {
> -			kfree(orig_vpd_buf);
> -			orig_vpd_buf = NULL;
> -		}
> +
> +		if (orig_vpd_buf)
> +			kfree_rcu(orig_vpd_buf, rcu);
>  	}
> 
>  	if (pg83_supported) {
> -		vpd_buf = scsi_get_vpd_buf(sdev, 0x83, &vpd_len);
> +		vpd_buf = scsi_get_vpd_buf(sdev, 0x83);
> +
>  		mutex_lock(&sdev->inquiry_mutex);
> -		orig_vpd_buf = sdev->vpd_pg83;
> -		sdev->vpd_pg83_len = vpd_len;
> +		orig_vpd_buf = rcu_dereference_protected(sdev-
> >vpd_pg83,
> +					lockdep_is_held(&sdev-
> >inquiry_mutex));
>  		rcu_assign_pointer(sdev->vpd_pg83, vpd_buf);
>  		mutex_unlock(&sdev->inquiry_mutex);
> -		synchronize_rcu();
> +
>  		if (orig_vpd_buf)
> -			kfree(orig_vpd_buf);
> +			kfree_rcu(orig_vpd_buf, rcu);
>  	}
>  }
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 813d2456ae93..3d6e50087600 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -3276,8 +3276,8 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char
> *id, size_t id_len)
>  {
>  	u8 cur_id_type = 0xff;
>  	u8 cur_id_size = 0;
> -	unsigned char *d, *cur_id_str;
> -	unsigned char __rcu *vpd_pg83;
> +	const unsigned char *d, *cur_id_str;
> +	const struct scsi_vpd *vpd_pg83;
>  	int id_size = -EINVAL;
> 
>  	rcu_read_lock();
> @@ -3308,8 +3308,8 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char
> *id, size_t id_len)
>  	}
> 
>  	memset(id, 0, id_len);
> -	d = vpd_pg83 + 4;
> -	while (d < vpd_pg83 + sdev->vpd_pg83_len) {
> +	d = vpd_pg83->data + 4;
> +	while (d < vpd_pg83->data + vpd_pg83->len) {
>  		/* Skip designators not referring to the LUN */
>  		if ((d[1] & 0x30) != 0x00)
>  			goto next_desig;
> @@ -3425,8 +3425,8 @@ EXPORT_SYMBOL(scsi_vpd_lun_id);
>   */
>  int scsi_vpd_tpg_id(struct scsi_device *sdev, int *rel_id)
>  {
> -	unsigned char *d;
> -	unsigned char __rcu *vpd_pg83;
> +	const unsigned char *d;
> +	const struct scsi_vpd *vpd_pg83;
>  	int group_id = -EAGAIN, rel_port = -1;
> 
>  	rcu_read_lock();
> @@ -3436,8 +3436,8 @@ int scsi_vpd_tpg_id(struct scsi_device *sdev, int
> *rel_id)
>  		return -ENXIO;
>  	}
> 
> -	d = sdev->vpd_pg83 + 4;
> -	while (d < sdev->vpd_pg83 + sdev->vpd_pg83_len) {
> +	d = vpd_pg83->data + 4;
> +	while (d < vpd_pg83->data + vpd_pg83->len) {
>  		switch (d[1] & 0xf) {
>  		case 0x4:
>  			/* Relative target port */
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 5ed473a87589..8a75e0f10eeb 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -428,6 +428,7 @@ static void
> scsi_device_dev_release_usercontext(struct work_struct *work)
>  	struct scsi_device *sdev;
>  	struct device *parent;
>  	struct list_head *this, *tmp;
> +	struct scsi_vpd *vpd_pg80, *vpd_pg83;
>  	unsigned long flags;
> 
>  	sdev = container_of(work, struct scsi_device, ew.work);
> @@ -456,8 +457,19 @@ 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);
> +	vpd_pg80 = rcu_dereference_protected(sdev->vpd_pg80,
> +				lockdep_is_held(&sdev->inquiry_mutex));
> +	rcu_assign_pointer(sdev->vpd_pg80, NULL);
> +	vpd_pg83 = rcu_dereference_protected(sdev->vpd_pg83,
> +				lockdep_is_held(&sdev->inquiry_mutex));
> +	rcu_assign_pointer(sdev->vpd_pg83, NULL);
> +	mutex_unlock(&sdev->inquiry_mutex);
> +
> +	if (vpd_pg83)
> +		kfree_rcu(vpd_pg83, rcu);
> +	if (vpd_pg80)
> +		kfree_rcu(vpd_pg80, rcu);
>  	kfree(sdev->inquiry);
>  	kfree(sdev);
> 
> @@ -795,15 +807,17 @@ show_vpd_##_page(struct file *filp, struct kobject
> *kobj,	\
>  {									\
>  	struct device *dev = container_of(kobj, struct device, kobj);	\
>  	struct scsi_device *sdev = to_scsi_device(dev);
> 	\
> +	struct scsi_vpd *vpd_page;					\
>  	int ret;							\
> +									\

Can this check here:

>  	if (!sdev->vpd_##_page)
> 	\
>  		return -EINVAL;
> 	\
>  	rcu_read_lock();						\
> -	ret = memory_read_from_buffer(buf, count, &off,
> 	\
> -				      rcu_dereference(sdev->vpd_##_page), \
> -				       sdev->vpd_##_page##_len);	\
> +	vpd_page = rcu_dereference(sdev->vpd_##_page);

Be moved to here so memory_read_from_buffer is made conditional on if vpd_page is not NULL (initialize ret with -EINVAL)? Although it's been checked above there's no guarantee that vpd_page may now be set to. 

> 	\
> +	ret = memory_read_from_buffer(buf, count, &off, vpd_page->data,
> \
> +				      vpd_page->len);			\
>  	rcu_read_unlock();						\
> -	return ret;						\
> +	return ret;							\
>  }									\

That should make it look more like this:

	struct device *dev = container_of(kobj, struct device, kobj);	\
 	struct scsi_device *sdev = to_scsi_device(dev);			\
+	struct scsi_vpd *vpd_page;					\
-	int ret;							\
-	int ret = -EINVAL;						\
+									\
-	if (!sdev->vpd_##_page)						\
-		return -EINVAL;						\
 	rcu_read_lock();						\
-	ret = memory_read_from_buffer(buf, count, &off,			\
-				      rcu_dereference(sdev->vpd_##_page), \
-				       sdev->vpd_##_page##_len);	\
+	vpd_page = rcu_dereference(sdev->vpd_##_page);			\
+	if (vpd_page)							\
+		ret = memory_read_from_buffer(buf, count, &off, 	\
+			vpd_page->data, vpd_page->len);			\
 	rcu_read_unlock();						\
-	return ret;						\
+	return ret;							\

If you're happy to make a change like that feel free to add a reviewed-by line from me.



>  static struct bin_attribute dev_attr_vpd_##_page = {		\
>  	.attr =	{.name = __stringify(vpd_##_page), .mode = S_IRUGO },
> 	\
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index f054f3f43c75..82e93ee94708 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -80,6 +80,18 @@ struct scsi_event {
>  	 */
>  };
> 
> +/**
> + * struct scsi_vpd - SCSI Vital Product Data
> + * @rcu: For kfree_rcu().
> + * @len: Length in bytes of @data.
> + * @data: VPD data as defined in various T10 SCSI standard documents.
> + */
> +struct scsi_vpd {
> +	struct rcu_head	rcu;
> +	int		len;
> +	unsigned char	data[];
> +};
> +
>  struct scsi_device {
>  	struct Scsi_Host *host;
>  	struct request_queue *request_queue;
> @@ -122,10 +134,8 @@ struct scsi_device {
>  	const char * rev;		/* ... "nullnullnullnull" before scan */
> 
>  #define SCSI_VPD_PG_LEN                255
> -	int vpd_pg83_len;
> -	unsigned char __rcu *vpd_pg83;
> -	int vpd_pg80_len;
> -	unsigned char __rcu *vpd_pg80;
> +	struct scsi_vpd __rcu *vpd_pg83;
> +	struct scsi_vpd __rcu *vpd_pg80;
>  	unsigned char current_tag;	/* current tag */
>  	struct scsi_target      *sdev_target;   /* used only for single_lun */
> 
> --
> 2.14.1

Re: [PATCH 1/2] Introduce scsi_get_vpd_buf()

[Christoph Hellwig]

On Fri, Aug 25, 2017 at 01:36:00PM -0700, Bart Van Assche wrote:
>  
>  	if (pg80_supported) {
> -retry_pg80:
> -		vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
> -		if (!vpd_buf)
> -			return;
> -
> -		result = scsi_vpd_inquiry(sdev, vpd_buf, 0x80, vpd_len);
> -		if (result < 0) {
> -			kfree(vpd_buf);
> -			return;
> -		}
> -		if (result > vpd_len) {
> -			vpd_len = result;
> -			kfree(vpd_buf);
> -			goto retry_pg80;
> -		}
> +		vpd_buf = scsi_get_vpd_buf(sdev, 0x80, &vpd_len);
> +

The old code above did an early return when scsi_vpd_inquiry, so
contrary to the old description it does change behavior.

I think that needs to be fixed up.

Except for that this look slike a good cleanup.

Re: [PATCH 2/2] Rework handling of scsi_device.vpd_pg8[03]

[Christoph Hellwig]

This looks generally good.  But I wonder if there is a way to
factor the assining/clearing sequence into little helpers instead
of duplicating it?