RE: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]
> Hello Shane, > > You have either misinterpret my statement or the SCSI VPD handling code. If > you have a look at the SCSI VPD handling code you will see that an > rcu_read_lock() / > rcu_read_unlock() pair is sufficient to prevent that the VPD buffer > rcu_dereference() points at is being modified as long as the RCU read lock is > held, at least if > rcu_dereference() is only called once. The update side namely does not > modify the VPD buffer after the pointer to that buffer has been published. Hi Bart, I'm pretty sure I understood the code. My main point was that the only thing that RCU guarantees with code in between rcu_read_lock()/rcu_read_unlock() if you dereference an RCU pointer using rcu_dereference() is that the memory that the pointer returned points to won't be freed and will be valid regardless of if you get the new or old pointer. Anything related to the actual contents of what the pointer points to is code specific in regard to what happens to it. As Christoph pointed out (which I failed to consider fully) by doing a direct kfree in scsi_device_dev_release_usercontext() without a call to synchronize_rcu() if you had some code that that got one of the RCU pointers in a read-side critical section and for some reason scsi_device_dev_release_usercontext() got called you could now have an invalid pointer when RCU makes a guarantee that it must be valid. That's what I believe Christoph was discussing in his reply to my email. > Switching to kfree_rcu() requires more changes because all unsigned char > pointers to VPD data have to be converted into pointers to a structure that > contains the VPD data and the RCU head. Anyway, I will convert the kfree() > calls to RCU pointers into kfree_rcu() pointers. > Thanks, I appreciate you taking the comments onboard. I've been looking at the new patches today and should post something back by the end of my day today. Shane
Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]
On Fri, 2017-08-25 at 05:58 +, Seymour, Shane M wrote: > > From: Bart Van Assche [mailto:bart.vanass...@wdc.com] > > [ ... ] > > My understanding of the SCSI VPD code is as follows: > > * rcu_read_lock() / rcu_read_unlock() is used to prevent that another thread > > updates a VPD buffer while it is being read. > > My understanding is that it doesn't do that - you can update an RCU pointer > with rcu_assign_pointer() after someone has called rcu_read_lock() and before > they call rcu_read_unlock(). Hello Shane, You have either misinterpret my statement or the SCSI VPD handling code. If you have a look at the SCSI VPD handling code you will see that an rcu_read_lock() / rcu_read_unlock() pair is sufficient to prevent that the VPD buffer rcu_dereference() points at is being modified as long as the RCU read lock is held, at least if rcu_dereference() is only called once. The update side namely does not modify the VPD buffer after the pointer to that buffer has been published. > It is possible I may be being too picky (it's a personal failing sometimes) > but is it really that a large overhead to free the RCU pointers in a way that > RCU pointers are expected to work even if the pointers shouldn't be accessible > to anything? Switching to kfree_rcu() requires more changes because all unsigned char pointers to VPD data have to be converted into pointers to a structure that contains the VPD data and the RCU head. Anyway, I will convert the kfree() calls to RCU pointers into kfree_rcu() pointers. Bart.
Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]
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(>inquiry_mutex); > > + kfree(rcu_dereference(sdev->vpd_pg83)); > > + kfree(rcu_dereference(sdev->vpd_pg80)); > > + mutex_unlock(>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.
Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]
On 08/23/2017 11:39 PM, Bart Van Assche wrote:
> Only annotate pointers that are shared across threads with __rcu.
> Use rcu_dereference() when dereferencing an RCU pointer. Protect
> also the RCU pointer dereferences when freeing RCU pointers. 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
> Cc: Christoph Hellwig
> Cc: Hannes Reinecke
> Cc: Johannes Thumshirn
> Cc: Shane Seymour
> ---
> drivers/scsi/scsi.c | 6 +++---
> drivers/scsi/scsi_lib.c | 8
> drivers/scsi/scsi_sysfs.c | 7 +--
> 3 files changed, 12 insertions(+), 9 deletions(-)
>
> 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?
> @@ -474,7 +474,7 @@ void scsi_attach_vpd(struct scsi_device *sdev)
> goto retry_pg80;
> }
> mutex_lock(>inquiry_mutex);
> - orig_vpd_buf = sdev->vpd_pg80;
> + orig_vpd_buf = rcu_dereference(sdev->vpd_pg80);
> sdev->vpd_pg80_len = result;
> rcu_assign_pointer(sdev->vpd_pg80, vpd_buf);
> mutex_unlock(>inquiry_mutex);
Yeah, I wasn't quite sure if I need to use rcu_dereference here.
> @@ -503,7 +503,7 @@ void scsi_attach_vpd(struct scsi_device *sdev)
> goto retry_pg83;
> }
> mutex_lock(>inquiry_mutex);
> - orig_vpd_buf = sdev->vpd_pg83;
> + orig_vpd_buf = rcu_dereference(sdev->vpd_pg83);
> sdev->vpd_pg83_len = result;
> rcu_assign_pointer(sdev->vpd_pg83, vpd_buf);
> mutex_unlock(>inquiry_mutex);
Same here.
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 1905962fb992..2ca91d251c5f 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -3282,7 +3282,7 @@ 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;
> + unsigned char *vpd_pg83;
> int id_size = -EINVAL;
>
> rcu_read_lock();
> @@ -3431,7 +3431,7 @@ 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;
> + unsigned char *vpd_pg83;
> int group_id = -EAGAIN, rel_port = -1;
>
> rcu_read_lock();
> @@ -3441,8 +3441,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 + 4;
> + while (d < vpd_pg83 + sdev->vpd_pg83_len) {
> switch (d[1] & 0xf) {
> case 0x4:
> /* Relative target port */
Bah. Yeah, of course.
> 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(>inquiry_mutex);
> + kfree(rcu_dereference(sdev->vpd_pg83));
> + kfree(rcu_dereference(sdev->vpd_pg80));
> + mutex_unlock(>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 ...
Cheers,
Hannes
--
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]
On Fri, Aug 25, 2017 at 05:58:16AM +, Seymour, Shane M wrote: > > My understanding of the SCSI VPD code is as follows: > > * rcu_read_lock() / rcu_read_unlock() is used to prevent that another thread > > updates a VPD buffer while it is being read. > > My understanding is that it doesn't do that - you can update an RCU > pointer with rcu_assign_pointer() after someone has called > rcu_read_lock() and before they call rcu_read_unlock(). Indeed. > What rcu_read_lock() / rcu_read_unlock() do is mark a read-side critical > section when accessing an RCU data item. If you have 2 CPUs in a > read-side critical section and a 3rd CPU replacing the pointer using > rcu_assign_pointer() one CPU could potentially end up with the old pointer > and the other one with the new pointer or both old or both new (the only > guarantee you have is that the pointer won't be partially updated with > bits of old and the new pointer). Exactly. > To free the old pointer directly you have to call synchronize_rcu() > after which you can call kfree() or if you don't call synchronize_rcu() > you have to use a delayed freeing mechanism like kfree_rcu() (or call_rcu for the more general case) > so you can guarantee that the old pointer is still valid while used in a > read-side critical section. Using something like kfree_rcu() means that > you also don’t have to wait like I believe you can do if you call > synchronize_rcu() since you could be forced to wait for a RCU grace > period to end before you can call kfree(). Yes, call_rcu (including the kfree_rcu helper) schedules a delayed action after the grace period, synchronize_rcu synchronously waits for the end of the grace period. > > * All code that either updates or reads a VPD buffer holds a reference on > > the SCSI device that buffer is associated with. That is why I think it is > > not needed to use kfree_rcu() in scsi_device_dev_release_usercontext(). But a reference could be dropped during the grace period. We'll need to either wait for the grace period after NULLing out the vpd pointers or before freeing the allocations for them. Currently the only rcu synchronization in the scsi code is after assining the vpd pointers (in which case we'd only need it whe replacing the previous one, which should not happen in practice anyway), but I can't find anything in the free path.
RE: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]
> -Original Message- > From: Bart Van Assche [mailto:bart.vanass...@wdc.com] > Sent: Friday, August 25, 2017 2:54 AM > To: h...@lst.de > Cc: j...@linux.vnet.ibm.com; linux-scsi@vger.kernel.org; h...@suse.de; > jthumsh...@suse.de; martin.peter...@oracle.com; Seymour, Shane M > <shane.seym...@hpe.com> > Subject: Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03] > > On Thu, 2017-08-24 at 11:07 +0200, Christoph Hellwig wrote: > > On Wed, Aug 23, 2017 at 02:39:57PM -0700, Bart Van Assche wrote: > > > Only annotate pointers that are shared across threads with __rcu. > > > Use rcu_dereference() when dereferencing an RCU pointer. Protect > > > also the RCU pointer dereferences when freeing RCU pointers. This > > > patch suppresses about twenty sparse complaints about the > > > vpd_pg8[03] pointers. > > > > Shouldn't the kfrees be kfree_rcu? or where else is the rcu > > protection for them? > > Hello Christoph, > Hi Bart, > My understanding of the SCSI VPD code is as follows: > * rcu_read_lock() / rcu_read_unlock() is used to prevent that another thread > updates a VPD buffer while it is being read. My understanding is that it doesn't do that - you can update an RCU pointer with rcu_assign_pointer() after someone has called rcu_read_lock() and before they call rcu_read_unlock(). What rcu_read_lock() / rcu_read_unlock() do is mark a read-side critical section when accessing an RCU data item. If you have 2 CPUs in a read-side critical section and a 3rd CPU replacing the pointer using rcu_assign_pointer() one CPU could potentially end up with the old pointer and the other one with the new pointer or both old or both new (the only guarantee you have is that the pointer won't be partially updated with bits of old and the new pointer). To free the old pointer directly you have to call synchronize_rcu() after which you can call kfree() or if you don't call synchronize_rcu() you have to use a delayed freeing mechanism like kfree_rcu() so you can guarantee that the old pointer is still valid while used in a read-side critical section. Using something like kfree_rcu() means that you also don’t have to wait like I believe you can do if you call synchronize_rcu() since you could be forced to wait for a RCU grace period to end before you can call kfree(). So what they really do is ensure that someone updating the pointer can't free the old pointer (in case someone is using it) until everyone has left their read-side critical section (if the read-side critical section started before synchronize_rcu() was called). You'll find a good example here that uses synchronize_rcu(): https://www.kernel.org/doc/Documentation/RCU/whatisRCU.txt Search for " WHAT ARE SOME EXAMPLE USES OF CORE RCU API?" - it has a lot of other information about RCU. > * All code that either updates or reads a VPD buffer holds a reference on > the SCSI device that buffer is associated with. That is why I think it is > not needed to use kfree_rcu() in scsi_device_dev_release_usercontext(). The only counter argument I'd put up into why it shouldn't be done the way you want to is that if someone else sees that code and doesn't understand the context and can't guarantee similar to this situation where all references to the structure should have already been dropped and think that it's ok to directly kfree something returned from rcu_dereference() when it's something that they really shouldn't do. The only real difference is that kfree_rcu will return the memory for reuse when RCU processing gets done in softirq and what you're doing will do it immediately. It's easier to use kfree_rcu() from what I can see. It is possible I may be being too picky (it's a personal failing sometimes) but is it really that a large overhead to free the RCU pointers in a way that RCU pointers are expected to work even if the pointers shouldn't be accessible to anything? Shane > > Bart. >
Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]
On Thu, 2017-08-24 at 11:07 +0200, Christoph Hellwig wrote: > On Wed, Aug 23, 2017 at 02:39:57PM -0700, Bart Van Assche wrote: > > Only annotate pointers that are shared across threads with __rcu. > > Use rcu_dereference() when dereferencing an RCU pointer. Protect > > also the RCU pointer dereferences when freeing RCU pointers. This > > patch suppresses about twenty sparse complaints about the vpd_pg8[03] > > pointers. > > Shouldn't the kfrees be kfree_rcu? or where else is the rcu protection > for them? Hello Christoph, My understanding of the SCSI VPD code is as follows: * rcu_read_lock() / rcu_read_unlock() is used to prevent that another thread updates a VPD buffer while it is being read. * All code that either updates or reads a VPD buffer holds a reference on the SCSI device that buffer is associated with. That is why I think it is not needed to use kfree_rcu() in scsi_device_dev_release_usercontext(). Bart.
Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]
On Wed, Aug 23, 2017 at 02:39:57PM -0700, Bart Van Assche wrote: > Only annotate pointers that are shared across threads with __rcu. > Use rcu_dereference() when dereferencing an RCU pointer. Protect > also the RCU pointer dereferences when freeing RCU pointers. This > patch suppresses about twenty sparse complaints about the vpd_pg8[03] > pointers. Shouldn't the kfrees be kfree_rcu? or where else is the rcu protection for them?
