All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Seymour, Shane M" <shane.seymour@hpe.com>
To: Bart Van Assche <Bart.VanAssche@wdc.com>, "hch@lst.de" <hch@lst.de>
Cc: "jejb@linux.vnet.ibm.com" <jejb@linux.vnet.ibm.com>,
	"linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
	"hare@suse.de" <hare@suse.de>,
	"jthumshirn@suse.de" <jthumshirn@suse.de>,
	"martin.petersen@oracle.com" <martin.petersen@oracle.com>
Subject: RE: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]
Date: Mon, 28 Aug 2017 02:02:29 +0000	[thread overview]
Message-ID: <AT5PR84MB0308DD95D77323F7FF9D9F55FD9E0@AT5PR84MB0308.NAMPRD84.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <1503691451.2680.16.camel@wdc.com>

> 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

  reply	other threads:[~2017-08-28  2:02 UTC|newest]

Thread overview: 65+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-23 21:39 [PATCH 00/19] SCSI patches for kernel v4.14 Bart Van Assche
2017-08-23 21:39 ` [PATCH 01/19] Remove an obsolete function declaration Bart Van Assche
2017-08-24  9:02   ` Christoph Hellwig
2017-08-23 21:39 ` [PATCH 02/19] Avoid sign extension of scsi_device.type Bart Van Assche
2017-08-24  6:10   ` Hannes Reinecke
2017-08-24  9:02   ` Christoph Hellwig
2017-08-23 21:39 ` [PATCH 03/19] Suppress gcc 7 fall-through warnings reported with W=1 Bart Van Assche
2017-08-24  9:03   ` Christoph Hellwig
2017-08-23 21:39 ` [PATCH 04/19] Convert a strncmp() call into a strcmp() call Bart Van Assche
2017-08-24  9:03   ` Christoph Hellwig
2017-08-25 15:43   ` Hannes Reinecke
2017-08-23 21:39 ` [PATCH 05/19] scsi_setup_fs_cmnd(): Call scsi_req_init() instead of open-coding it Bart Van Assche
2017-08-24  9:05   ` Christoph Hellwig
2017-08-24 16:17     ` Bart Van Assche
2017-08-23 21:39 ` [PATCH 06/19] Document which queue type a function is intended for Bart Van Assche
2017-08-24  9:05   ` Christoph Hellwig
2017-08-24 16:57     ` Bart Van Assche
2017-08-24 16:58       ` hch
2017-08-24 17:22         ` Bart Van Assche
2017-08-25 15:44   ` Hannes Reinecke
2017-08-23 21:39 ` [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03] Bart Van Assche
2017-08-24  9:07   ` Christoph Hellwig
2017-08-24 16:54     ` Bart Van Assche
2017-08-25  5:58       ` Seymour, Shane M
2017-08-25  6:59         ` hch
2017-08-25 20:04         ` Bart Van Assche
2017-08-28  2:02           ` Seymour, Shane M [this message]
2017-08-25 15:49   ` Hannes Reinecke
2017-08-25 16:26     ` Bart Van Assche
2017-08-23 21:39 ` [PATCH 08/19] Use blk_mq_rq_to_pdu() to convert a request to a SCSI command pointer Bart Van Assche
2017-08-24  9:07   ` Christoph Hellwig
2017-08-23 21:39 ` [PATCH 09/19] sd, sr: Convert two assignments into warning statements Bart Van Assche
2017-08-24  9:08   ` Christoph Hellwig
2017-08-23 21:40 ` [PATCH 10/19] sd: Fix indentation Bart Van Assche
2017-08-24  9:08   ` Christoph Hellwig
2017-08-25 15:50   ` Hannes Reinecke
2017-08-23 21:40 ` [PATCH 11/19] sd: Remove a useless comparison Bart Van Assche
2017-08-25 15:50   ` Hannes Reinecke
2017-08-23 21:40 ` [PATCH 12/19] sg: Fix type of last blk_trace_setup() argument Bart Van Assche
2017-08-24  9:08   ` Christoph Hellwig
2017-08-25 15:51   ` Hannes Reinecke
2017-08-23 21:40 ` [PATCH 13/19] libiscsi: Fix indentation Bart Van Assche
2017-08-24  9:08   ` Christoph Hellwig
2017-08-25 15:51   ` Hannes Reinecke
2017-08-23 21:40 ` [PATCH 14/19] libsas: Remove a set-but-not-used variable Bart Van Assche
2017-08-24  9:09   ` Christoph Hellwig
2017-08-25 15:51   ` Hannes Reinecke
2017-08-23 21:40 ` [PATCH 15/19] libsas: Annotate fall-through in a switch statement Bart Van Assche
2017-08-24  9:09   ` Christoph Hellwig
2017-08-25 15:52   ` Hannes Reinecke
2017-08-23 21:40 ` [PATCH 16/19] scsi_transport_sas, sas_tlr_supported(): Check kzalloc() return value Bart Van Assche
2017-08-24  9:09   ` Christoph Hellwig
2017-08-25 15:52   ` Hannes Reinecke
2017-08-23 21:40 ` [PATCH 17/19] scsi_transport_srp: Suppress a W=1 compiler warning Bart Van Assche
2017-08-24  9:11   ` Christoph Hellwig
2017-08-24 16:27     ` Bart Van Assche
2017-08-25 15:29       ` hch
2017-08-25 15:40         ` Bart Van Assche
2017-08-25 15:56           ` hch
2017-08-23 21:40 ` [PATCH 18/19] scsi_debug: Remove a set-but-not-used variable Bart Van Assche
2017-08-24  9:12   ` Christoph Hellwig
2017-08-25 15:53   ` Hannes Reinecke
2017-08-23 21:40 ` [PATCH 19/19] iscsi_tcp: " Bart Van Assche
2017-08-24  9:12   ` Christoph Hellwig
2017-08-25 15:53   ` Hannes Reinecke

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=AT5PR84MB0308DD95D77323F7FF9D9F55FD9E0@AT5PR84MB0308.NAMPRD84.PROD.OUTLOOK.COM \
    --to=shane.seymour@hpe.com \
    --cc=Bart.VanAssche@wdc.com \
    --cc=hare@suse.de \
    --cc=hch@lst.de \
    --cc=jejb@linux.vnet.ibm.com \
    --cc=jthumshirn@suse.de \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.