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

[PATCH v3 0/3] 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 three patches in this series realize this
and also simplify the VPD code. Please consider these patches for kernel
v4.14.

Thanks,

Bart.

Changes between v2 and v3:
- Added a third patch that introduces rcu_swap_protected().
- Introduced scsi_update_vpd_page().
- Skip VPD buffer updating if querying VPD data fails.
- Fix a race condition in the VPD sysfs show method.

Changes between v1 and v2:
- Split the VPD rework into two patches.
- Introduced struct scsi_vpd and scsi_get_vpd_buf().

Bart Van Assche (3):
  Rework the code for caching Vital Product Data (VPD)
  Rework handling of scsi_device.vpd_pg8[03]
  scsi_transport_sas: Fix error handling in sas_smp_request()

 drivers/scsi/scsi.c               | 140 ++++++++++++++++----------------------
 drivers/scsi/scsi_lib.c           |  16 ++---
 drivers/scsi/scsi_sysfs.c         |  29 +++++---
 drivers/scsi/scsi_transport_sas.c |   6 +-
 include/scsi/scsi_device.h        |  18 +++--
 5 files changed, 106 insertions(+), 103 deletions(-)

-- 
2.14.1

[PATCH v3 1/3] rcu: Introduce rcu_swap_protected()

[Bart Van Assche]

A common pattern in RCU code is to assign a new value to an RCU
pointer after having read and stored the old value. Introduce a
macro for this pattern.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@kernel.org>
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>
---
 include/linux/rcupdate.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index f816fc72b51e..555815ce2e57 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -561,6 +561,26 @@ static inline void rcu_preempt_sleep_check(void) { }
  */
 #define rcu_pointer_handoff(p) (p)
 
+/**
+ * rcu_swap_protected() - swap an RCU and a regular pointer
+ * @rcu_ptr: RCU pointer
+ * @ptr: regular pointer
+ * @c: the conditions under which the dereference will take place
+ *
+ * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
+ * @c is the argument that is passed to the rcu_dereference_protected() call
+ * used to read that pointer.
+ */
+#define rcu_swap_protected(rcu_ptr, ptr, c) do {			\
+	typeof(ptr) __tmp;						\
+									\
+	BUILD_BUG_ON(!__same_type(typeof(rcu_ptr), typeof(*(ptr)) __rcu *)); \
+	BUILD_BUG_ON(!__same_type(typeof(ptr), typeof(*(rcu_ptr)) *));	\
+	__tmp = rcu_dereference_protected((rcu_ptr), (c));		\
+	rcu_assign_pointer((rcu_ptr), (ptr));				\
+	(ptr) = __tmp;							\
+} while (0)
+
 /**
  * rcu_read_lock() - mark the beginning of an RCU read-side critical section
  *
-- 
2.14.1

Re: [PATCH v3 1/3] rcu: Introduce rcu_swap_protected()

[Paul E. McKenney]

On Mon, Aug 28, 2017 at 01:46:13PM -0700, Bart Van Assche wrote:
> A common pattern in RCU code is to assign a new value to an RCU
> pointer after having read and stored the old value. Introduce a
> macro for this pattern.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> 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>
> ---
>  include/linux/rcupdate.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index f816fc72b51e..555815ce2e57 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -561,6 +561,26 @@ static inline void rcu_preempt_sleep_check(void) { }
>   */
>  #define rcu_pointer_handoff(p) (p)
> 
> +/**
> + * rcu_swap_protected() - swap an RCU and a regular pointer
> + * @rcu_ptr: RCU pointer
> + * @ptr: regular pointer
> + * @c: the conditions under which the dereference will take place
> + *
> + * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
> + * @c is the argument that is passed to the rcu_dereference_protected() call
> + * used to read that pointer.
> + */
> +#define rcu_swap_protected(rcu_ptr, ptr, c) do {			\
> +	typeof(ptr) __tmp;						\
> +									\
> +	BUILD_BUG_ON(!__same_type(typeof(rcu_ptr), typeof(*(ptr)) __rcu *)); \
> +	BUILD_BUG_ON(!__same_type(typeof(ptr), typeof(*(rcu_ptr)) *));	\

Hmmm...

What kinds of bugs have these two BUILD_BUG_ON() instances have caught
that would not be caught by the assignments below?

> +	__tmp = rcu_dereference_protected((rcu_ptr), (c));		\
> +	rcu_assign_pointer((rcu_ptr), (ptr));				\
> +	(ptr) = __tmp;							\
> +} while (0)
> +

Could you please put this after rcu_assign_pointer() and before
rcu_access_pointer()?  That way the things that assign to RCU-protected
pointers are together.

							Thanx, Paul

>  /**
>   * rcu_read_lock() - mark the beginning of an RCU read-side critical section
>   *
> -- 
> 2.14.1
> 

Re: [PATCH v3 1/3] rcu: Introduce rcu_swap_protected()

[Bart Van Assche]

On Mon, 2017-08-28 at 14:26 -0700, Paul E. McKenney wrote:
> On Mon, Aug 28, 2017 at 01:46:13PM -0700, Bart Van Assche wrote:
> > A common pattern in RCU code is to assign a new value to an RCU
> > pointer after having read and stored the old value. Introduce a
> > macro for this pattern.
> > 
> > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> > Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > 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>
> > ---
> >  include/linux/rcupdate.h | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index f816fc72b51e..555815ce2e57 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -561,6 +561,26 @@ static inline void rcu_preempt_sleep_check(void) { }
> >   */
> >  #define rcu_pointer_handoff(p) (p)
> > 
> > +/**
> > + * rcu_swap_protected() - swap an RCU and a regular pointer
> > + * @rcu_ptr: RCU pointer
> > + * @ptr: regular pointer
> > + * @c: the conditions under which the dereference will take place
> > + *
> > + * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
> > + * @c is the argument that is passed to the rcu_dereference_protected() call
> > + * used to read that pointer.
> > + */
> > +#define rcu_swap_protected(rcu_ptr, ptr, c) do {			\
> > +	typeof(ptr) __tmp;						\
> > +									\
> > +	BUILD_BUG_ON(!__same_type(typeof(rcu_ptr), typeof(*(ptr)) __rcu *)); \
> > +	BUILD_BUG_ON(!__same_type(typeof(ptr), typeof(*(rcu_ptr)) *));	\
> 
> Hmmm...
> 
> What kinds of bugs have these two BUILD_BUG_ON() instances have caught
> that would not be caught by the assignments below?

Hello Paul,

These two BUILD_BUG_ON() statements can be left out. The purpose of these
statements is to complain as early as possible if the type of rcu_ptr and/or
ptr is incorrect. As we all know error messages that are triggered by macros
used inside a macro definition can be hard to read. My hope is that these
two BUILD_BUG_ON() macros will cause the compiler to report easier to read
diagnostic messages.

> > +	__tmp = rcu_dereference_protected((rcu_ptr), (c));		\
> > +	rcu_assign_pointer((rcu_ptr), (ptr));				\
> > +	(ptr) = __tmp;							\
> > +} while (0)
> > +
> 
> Could you please put this after rcu_assign_pointer() and before
> rcu_access_pointer()?  That way the things that assign to RCU-protected
> pointers are together.

Something like the patch below (compile-tested only)?

Thanks,

Bart.


[PATCH] rcu: Introduce rcu_swap_protected()
---
 include/linux/rcupdate.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index f816fc72b51e..8e920f0ecb07 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -407,6 +407,22 @@ static inline void rcu_preempt_sleep_check(void) { }
 	_r_a_p__v;							      \
 })
 
+/**
+ * rcu_swap_protected() - swap an RCU and a regular pointer
+ * @rcu_ptr: RCU pointer
+ * @ptr: regular pointer
+ * @c: the conditions under which the dereference will take place
+ *
+ * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
+ * @c is the argument that is passed to the rcu_dereference_protected() call
+ * used to read that pointer.
+ */
+#define rcu_swap_protected(rcu_ptr, ptr, c) do {			\
+	typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c));	\
+	rcu_assign_pointer((rcu_ptr), (ptr));				\
+	(ptr) = __tmp;							\
+} while (0)
+
 /**
  * rcu_access_pointer() - fetch RCU pointer with no dereferencing
  * @p: The pointer to read

Re: [PATCH v3 1/3] rcu: Introduce rcu_swap_protected()

[Paul E. McKenney]

On Mon, Aug 28, 2017 at 09:39:18PM +0000, Bart Van Assche wrote:
> On Mon, 2017-08-28 at 14:26 -0700, Paul E. McKenney wrote:
> > On Mon, Aug 28, 2017 at 01:46:13PM -0700, Bart Van Assche wrote:
> > > A common pattern in RCU code is to assign a new value to an RCU
> > > pointer after having read and stored the old value. Introduce a
> > > macro for this pattern.
> > > 
> > > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> > > Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > Cc: Ingo Molnar <mingo@kernel.org>
> > > 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>
> > > ---
> > >  include/linux/rcupdate.h | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > > 
> > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > index f816fc72b51e..555815ce2e57 100644
> > > --- a/include/linux/rcupdate.h
> > > +++ b/include/linux/rcupdate.h
> > > @@ -561,6 +561,26 @@ static inline void rcu_preempt_sleep_check(void) { }
> > >   */
> > >  #define rcu_pointer_handoff(p) (p)
> > > 
> > > +/**
> > > + * rcu_swap_protected() - swap an RCU and a regular pointer
> > > + * @rcu_ptr: RCU pointer
> > > + * @ptr: regular pointer
> > > + * @c: the conditions under which the dereference will take place
> > > + *
> > > + * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
> > > + * @c is the argument that is passed to the rcu_dereference_protected() call
> > > + * used to read that pointer.
> > > + */
> > > +#define rcu_swap_protected(rcu_ptr, ptr, c) do {			\
> > > +	typeof(ptr) __tmp;						\
> > > +									\
> > > +	BUILD_BUG_ON(!__same_type(typeof(rcu_ptr), typeof(*(ptr)) __rcu *)); \
> > > +	BUILD_BUG_ON(!__same_type(typeof(ptr), typeof(*(rcu_ptr)) *));	\
> > 
> > Hmmm...
> > 
> > What kinds of bugs have these two BUILD_BUG_ON() instances have caught
> > that would not be caught by the assignments below?
> 
> Hello Paul,
> 
> These two BUILD_BUG_ON() statements can be left out. The purpose of these
> statements is to complain as early as possible if the type of rcu_ptr and/or
> ptr is incorrect. As we all know error messages that are triggered by macros
> used inside a macro definition can be hard to read. My hope is that these
> two BUILD_BUG_ON() macros will cause the compiler to report easier to read
> diagnostic messages.
> 
> > > +	__tmp = rcu_dereference_protected((rcu_ptr), (c));		\
> > > +	rcu_assign_pointer((rcu_ptr), (ptr));				\
> > > +	(ptr) = __tmp;							\
> > > +} while (0)
> > > +
> > 
> > Could you please put this after rcu_assign_pointer() and before
> > rcu_access_pointer()?  That way the things that assign to RCU-protected
> > pointers are together.
> 
> Something like the patch below (compile-tested only)?

Looks good!

I suspect that you would like to push this with your changes, so,
assuming 0day test robot and -next are OK with it:

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

I am not necessarily inalterably opposed to the extra BUILD_BUG_ON()
statements, and in fact I do like improved diagnostics, but those need to
go up via my tree as a separate patch.  That way, any unexpected gotchas
can be handled without risking your rcu_swap_protected() functionality.

							Thanx, Paul

> Thanks,
> 
> Bart.
> 
> 
> [PATCH] rcu: Introduce rcu_swap_protected()
> ---
>  include/linux/rcupdate.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index f816fc72b51e..8e920f0ecb07 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -407,6 +407,22 @@ static inline void rcu_preempt_sleep_check(void) { }
>  	_r_a_p__v;							      \
>  })
>  
> +/**
> + * rcu_swap_protected() - swap an RCU and a regular pointer
> + * @rcu_ptr: RCU pointer
> + * @ptr: regular pointer
> + * @c: the conditions under which the dereference will take place
> + *
> + * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
> + * @c is the argument that is passed to the rcu_dereference_protected() call
> + * used to read that pointer.
> + */
> +#define rcu_swap_protected(rcu_ptr, ptr, c) do {			\
> +	typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c));	\
> +	rcu_assign_pointer((rcu_ptr), (ptr));				\
> +	(ptr) = __tmp;							\
> +} while (0)
> +
>  /**
>   * rcu_access_pointer() - fetch RCU pointer with no dereferencing
>   * @p: The pointer to read

[PATCH v3 2/3] Rework the code for caching Vital Product Data (VPD)

[Bart Van Assche]

Introduce the scsi_get_vpd_buf() and scsi_update_vpd_page()
functions. The only functional change in this patch is that if
updating page 0x80 fails that it is attempted to update page 0x83.

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 | 144 ++++++++++++++++++++++++----------------------------
 1 file changed, 66 insertions(+), 78 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 3d38c6d463b8..f3f4926a3e77 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -411,6 +411,63 @@ 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;
+}
+
+static void scsi_update_vpd_page(struct scsi_device *sdev, u8 page,
+				 unsigned char __rcu **sdev_vpd_buf,
+				 int *sdev_vpd_len)
+{
+	unsigned char *vpd_buf;
+	int len;
+
+	vpd_buf = scsi_get_vpd_buf(sdev, page, &len);
+	if (!vpd_buf)
+		return;
+
+	mutex_lock(&sdev->inquiry_mutex);
+	rcu_swap_protected(*sdev_vpd_buf, vpd_buf,
+			   lockdep_is_held(&sdev->inquiry_mutex));
+	*sdev_vpd_len = len;
+	mutex_unlock(&sdev->inquiry_mutex);
+
+	synchronize_rcu();
+
+	kfree(vpd_buf);
+}
+
 /**
  * scsi_attach_vpd - Attach Vital Product Data to a SCSI device structure
  * @sdev: The device to ask
@@ -422,95 +479,26 @@ 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 pg80_supported = 0;
-	int pg83_supported = 0;
-	unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
+	int i, vpd_len;
+	unsigned char *vpd_buf;
 
 	if (!scsi_device_supports_vpd(sdev))
 		return;
 
-retry_pg0:
-	vpd_buf = kmalloc(vpd_len, GFP_KERNEL);
-	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);
+	vpd_buf = scsi_get_vpd_buf(sdev, 0, &vpd_len);
+	if (!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;
+			scsi_update_vpd_page(sdev, 0x80, &sdev->vpd_pg80,
+					     &sdev->vpd_pg80_len);
 		if (vpd_buf[i] == 0x83)
-			pg83_supported = 1;
+			scsi_update_vpd_page(sdev, 0x83, &sdev->vpd_pg83,
+					     &sdev->vpd_pg83_len);
 	}
 	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;
-		}
-		mutex_lock(&sdev->inquiry_mutex);
-		orig_vpd_buf = sdev->vpd_pg80;
-		sdev->vpd_pg80_len = result;
-		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;
-		}
-		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;
-		}
-		mutex_lock(&sdev->inquiry_mutex);
-		orig_vpd_buf = sdev->vpd_pg83;
-		sdev->vpd_pg83_len = result;
-		rcu_assign_pointer(sdev->vpd_pg83, vpd_buf);
-		mutex_unlock(&sdev->inquiry_mutex);
-		synchronize_rcu();
-		if (orig_vpd_buf)
-			kfree(orig_vpd_buf);
-	}
 }
 
 /**
-- 
2.14.1

[PATCH v3 3/3] 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. Move the VPD buffer pointer
check inside the RCU read lock in the sysfs code. 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. This patch also fixes a race condition, namely that
updating of the VPD pointers and length variables in struct
scsi_device was not atomic with reference to the code reading
these variables. See also "Does the update code tolerate concurrent
accesses?" in Documentation/RCU/checklist.txt.

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        | 44 ++++++++++++++++++--------------------------
 drivers/scsi/scsi_lib.c    | 16 ++++++++--------
 drivers/scsi/scsi_sysfs.c  | 29 ++++++++++++++++++++---------
 include/scsi/scsi_device.h | 18 ++++++++++++++----
 4 files changed, 60 insertions(+), 47 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index f3f4926a3e77..d201ebcf4544 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,31 +439,27 @@ 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;
 }
 
 static void scsi_update_vpd_page(struct scsi_device *sdev, u8 page,
-				 unsigned char __rcu **sdev_vpd_buf,
-				 int *sdev_vpd_len)
+				 struct scsi_vpd __rcu **sdev_vpd_buf)
 {
-	unsigned char *vpd_buf;
-	int len;
+	struct scsi_vpd *vpd_buf;
 
-	vpd_buf = scsi_get_vpd_buf(sdev, page, &len);
+	vpd_buf = scsi_get_vpd_buf(sdev, page);
 	if (!vpd_buf)
 		return;
 
 	mutex_lock(&sdev->inquiry_mutex);
 	rcu_swap_protected(*sdev_vpd_buf, vpd_buf,
 			   lockdep_is_held(&sdev->inquiry_mutex));
-	*sdev_vpd_len = len;
 	mutex_unlock(&sdev->inquiry_mutex);
 
-	synchronize_rcu();
-
-	kfree(vpd_buf);
+	if (vpd_buf)
+		kfree_rcu(vpd_buf, rcu);
 }
 
 /**
@@ -479,24 +473,22 @@ static void scsi_update_vpd_page(struct scsi_device *sdev, u8 page,
  */
 void scsi_attach_vpd(struct scsi_device *sdev)
 {
-	int i, vpd_len;
-	unsigned char *vpd_buf;
+	int i;
+	struct scsi_vpd *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)
-			scsi_update_vpd_page(sdev, 0x80, &sdev->vpd_pg80,
-					     &sdev->vpd_pg80_len);
-		if (vpd_buf[i] == 0x83)
-			scsi_update_vpd_page(sdev, 0x83, &sdev->vpd_pg83,
-					     &sdev->vpd_pg83_len);
+	for (i = 4; i < vpd_buf->len; i++) {
+		if (vpd_buf->data[i] == 0x80)
+			scsi_update_vpd_page(sdev, 0x80, &sdev->vpd_pg80);
+		if (vpd_buf->data[i] == 0x83)
+			scsi_update_vpd_page(sdev, 0x83, &sdev->vpd_pg83);
 	}
 	kfree(vpd_buf);
 }
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 696d2eae0ba6..938a7e398cd4 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3272,8 +3272,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();
@@ -3304,8 +3304,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;
@@ -3421,8 +3421,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();
@@ -3432,8 +3432,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..bf53356f41f0 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 = NULL, *vpd_pg83 = NULL;
 	unsigned long flags;
 
 	sdev = container_of(work, struct scsi_device, ew.work);
@@ -456,8 +457,17 @@ 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);
+	rcu_swap_protected(sdev->vpd_pg80, vpd_pg80,
+			   lockdep_is_held(&sdev->inquiry_mutex));
+	rcu_swap_protected(sdev->vpd_pg83, vpd_pg83,
+			   lockdep_is_held(&sdev->inquiry_mutex));
+	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 +805,16 @@ 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);			\
-	int ret;							\
-	if (!sdev->vpd_##_page)						\
-		return -EINVAL;						\
+	struct scsi_vpd *vpd_page;					\
+	int ret = -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;							\
 }									\
 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

[PATCH v3 0/3] 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 three patches in this series realize this
and also simplify the VPD code. Please consider these patches for kernel
v4.14.

Thanks,

Bart.

Changes between v2 and v3:
- Added a third patch that introduces rcu_swap_protected().
- Introduced scsi_update_vpd_page().
- Skip VPD buffer updating if querying VPD data fails.
- Fix a race condition in the VPD sysfs show method.

Changes between v1 and v2:
- Split the VPD rework into two patches.
- Introduced struct scsi_vpd and scsi_get_vpd_buf().

Bart Van Assche (3):
  Rework the code for caching Vital Product Data (VPD)
  Rework handling of scsi_device.vpd_pg8[03]
  scsi_transport_sas: Fix error handling in sas_smp_request()

 drivers/scsi/scsi.c               | 140 ++++++++++++++++----------------------
 drivers/scsi/scsi_lib.c           |  16 ++---
 drivers/scsi/scsi_sysfs.c         |  29 +++++---
 drivers/scsi/scsi_transport_sas.c |   6 +-
 include/scsi/scsi_device.h        |  18 +++--
 5 files changed, 106 insertions(+), 103 deletions(-)

-- 
2.14.1

[PATCH v3 1/3] rcu: Introduce rcu_swap_protected()

[Bart Van Assche]

A common pattern in RCU code is to assign a new value to an RCU
pointer after having read and stored the old value. Introduce a
macro for this pattern.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@kernel.org>
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>
---
 include/linux/rcupdate.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index f816fc72b51e..555815ce2e57 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -561,6 +561,26 @@ static inline void rcu_preempt_sleep_check(void) { }
  */
 #define rcu_pointer_handoff(p) (p)
 
+/**
+ * rcu_swap_protected() - swap an RCU and a regular pointer
+ * @rcu_ptr: RCU pointer
+ * @ptr: regular pointer
+ * @c: the conditions under which the dereference will take place
+ *
+ * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
+ * @c is the argument that is passed to the rcu_dereference_protected() call
+ * used to read that pointer.
+ */
+#define rcu_swap_protected(rcu_ptr, ptr, c) do {			\
+	typeof(ptr) __tmp;						\
+									\
+	BUILD_BUG_ON(!__same_type(typeof(rcu_ptr), typeof(*(ptr)) __rcu *)); \
+	BUILD_BUG_ON(!__same_type(typeof(ptr), typeof(*(rcu_ptr)) *));	\
+	__tmp = rcu_dereference_protected((rcu_ptr), (c));		\
+	rcu_assign_pointer((rcu_ptr), (ptr));				\
+	(ptr) = __tmp;							\
+} while (0)
+
 /**
  * rcu_read_lock() - mark the beginning of an RCU read-side critical section
  *
-- 
2.14.1

[PATCH v3 1/3] rcu: Introduce rcu_swap_protected()

[Bart Van Assche]

A common pattern in RCU code is to assign a new value to an RCU
pointer after having read and stored the old value. Introduce a
macro for this pattern.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@kernel.org>
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>
---
 include/linux/rcupdate.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index f816fc72b51e..555815ce2e57 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -561,6 +561,26 @@ static inline void rcu_preempt_sleep_check(void) { }
  */
 #define rcu_pointer_handoff(p) (p)
 
+/**
+ * rcu_swap_protected() - swap an RCU and a regular pointer
+ * @rcu_ptr: RCU pointer
+ * @ptr: regular pointer
+ * @c: the conditions under which the dereference will take place
+ *
+ * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
+ * @c is the argument that is passed to the rcu_dereference_protected() call
+ * used to read that pointer.
+ */
+#define rcu_swap_protected(rcu_ptr, ptr, c) do {			\
+	typeof(ptr) __tmp;						\
+									\
+	BUILD_BUG_ON(!__same_type(typeof(rcu_ptr), typeof(*(ptr)) __rcu *)); \
+	BUILD_BUG_ON(!__same_type(typeof(ptr), typeof(*(rcu_ptr)) *));	\
+	__tmp = rcu_dereference_protected((rcu_ptr), (c));		\
+	rcu_assign_pointer((rcu_ptr), (ptr));				\
+	(ptr) = __tmp;							\
+} while (0)
+
 /**
  * rcu_read_lock() - mark the beginning of an RCU read-side critical section
  *
-- 
2.14.1

[PATCH v3 0/3] 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 three patches in this series realize this
and also simplify the VPD code. Please consider these patches for kernel
v4.14.

Thanks,

Bart.

Changes between v2 and v3:
- Added a third patch that introduces rcu_swap_protected().
- Introduced scsi_update_vpd_page().
- Skip VPD buffer updating if querying VPD data fails.
- Fix a race condition in the VPD sysfs show method.

Changes between v1 and v2:
- Split the VPD rework into two patches.
- Introduced struct scsi_vpd and scsi_get_vpd_buf().

Bart Van Assche (3):
  Rework the code for caching Vital Product Data (VPD)
  Rework handling of scsi_device.vpd_pg8[03]
  scsi_transport_sas: Fix error handling in sas_smp_request()

 drivers/scsi/scsi.c               | 140 ++++++++++++++++----------------------
 drivers/scsi/scsi_lib.c           |  16 ++---
 drivers/scsi/scsi_sysfs.c         |  29 +++++---
 drivers/scsi/scsi_transport_sas.c |   6 +-
 include/scsi/scsi_device.h        |  18 +++--
 5 files changed, 106 insertions(+), 103 deletions(-)

-- 
2.14.1

[PATCH v3 1/3] rcu: Introduce rcu_swap_protected()

[Bart Van Assche]

A common pattern in RCU code is to assign a new value to an RCU
pointer after having read and stored the old value. Introduce a
macro for this pattern.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@kernel.org>
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>
---
 include/linux/rcupdate.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index f816fc72b51e..555815ce2e57 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -561,6 +561,26 @@ static inline void rcu_preempt_sleep_check(void) { }
  */
 #define rcu_pointer_handoff(p) (p)
 
+/**
+ * rcu_swap_protected() - swap an RCU and a regular pointer
+ * @rcu_ptr: RCU pointer
+ * @ptr: regular pointer
+ * @c: the conditions under which the dereference will take place
+ *
+ * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
+ * @c is the argument that is passed to the rcu_dereference_protected() call
+ * used to read that pointer.
+ */
+#define rcu_swap_protected(rcu_ptr, ptr, c) do {			\
+	typeof(ptr) __tmp;						\
+									\
+	BUILD_BUG_ON(!__same_type(typeof(rcu_ptr), typeof(*(ptr)) __rcu *)); \
+	BUILD_BUG_ON(!__same_type(typeof(ptr), typeof(*(rcu_ptr)) *));	\
+	__tmp = rcu_dereference_protected((rcu_ptr), (c));		\
+	rcu_assign_pointer((rcu_ptr), (ptr));				\
+	(ptr) = __tmp;							\
+} while (0)
+
 /**
  * rcu_read_lock() - mark the beginning of an RCU read-side critical section
  *
-- 
2.14.1

[PATCH v3 1/3] rcu: Introduce rcu_swap_protected()

[Bart Van Assche]

A common pattern in RCU code is to assign a new value to an RCU
pointer after having read and stored the old value. Introduce a
macro for this pattern.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@kernel.org>
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>
---
 include/linux/rcupdate.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index f816fc72b51e..555815ce2e57 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -561,6 +561,26 @@ static inline void rcu_preempt_sleep_check(void) { }
  */
 #define rcu_pointer_handoff(p) (p)
 
+/**
+ * rcu_swap_protected() - swap an RCU and a regular pointer
+ * @rcu_ptr: RCU pointer
+ * @ptr: regular pointer
+ * @c: the conditions under which the dereference will take place
+ *
+ * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
+ * @c is the argument that is passed to the rcu_dereference_protected() call
+ * used to read that pointer.
+ */
+#define rcu_swap_protected(rcu_ptr, ptr, c) do {			\
+	typeof(ptr) __tmp;						\
+									\
+	BUILD_BUG_ON(!__same_type(typeof(rcu_ptr), typeof(*(ptr)) __rcu *)); \
+	BUILD_BUG_ON(!__same_type(typeof(ptr), typeof(*(rcu_ptr)) *));	\
+	__tmp = rcu_dereference_protected((rcu_ptr), (c));		\
+	rcu_assign_pointer((rcu_ptr), (ptr));				\
+	(ptr) = __tmp;							\
+} while (0)
+
 /**
  * rcu_read_lock() - mark the beginning of an RCU read-side critical section
  *
-- 
2.14.1