Re: [PATCH v3 3/3] drivers/base/core: refcount kobject and bus on device attribute read / store

From: Luis Chamberlain <mcgrof@kernel.org>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: minchan@kernel.org, jeyu@kernel.org, ngupta@vflare.org,
	sergey.senozhatsky.work@gmail.com, axboe@kernel.dk,
	mbenes@suse.com, jpoimboe@redhat.com, tglx@linutronix.de,
	keescook@chromium.org, jikos@kernel.org, rostedt@goodmis.org,
	peterz@infradead.org, linux-block@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/3] drivers/base/core: refcount kobject and bus on device attribute read / store
Date: Tue, 22 Jun 2021 09:44:02 -0700	[thread overview]
Message-ID: <20210622164402.d62je6pajcplkfuy@garbanzo> (raw)
In-Reply-To: <YNGVUk18pmTFZqAB@kroah.com>

On Tue, Jun 22, 2021 at 09:46:26AM +0200, Greg KH wrote:
> On Mon, Jun 21, 2021 at 04:36:51PM -0700, Luis Chamberlain wrote:
> > It's possible today to have a device attribute read or store
> > race against device removal. When this happens there is a small
> > chance that the derefence for the private data area of the driver
> > is NULL.
> > 
> > Let's consider the zram driver as an example. Its possible to run into
> > a race where a sysfs knob is being used, we get preempted, and a zram
> > device is removed before we complete use of the sysfs knob. This can happen
> > for instance on block devices, where for instance the zram block devices
> > just part of the private data of the block device.
> > 
> > For instance this can happen in the following two situations
> > as examples to illustrate this better:
> > 
> >         CPU 1                            CPU 2
> > destroy_devices
> > ...
> >                                  compact_store()
> >                                  zram = dev_to_zram(dev);
> > idr_for_each(zram_remove_cb
> >   zram_remove
> >   ...
> >   kfree(zram)
> >                                  down_read(&zram->init_lock);
> > 
> >         CPU 1                            CPU 2
> > hot_remove_store
> >                                  compact_store()
> >                                  zram = dev_to_zram(dev);
> >   zram_remove
> >     kfree(zram)
> >                                  down_read(&zram->init_lock);
> > 
> > To ensure the private data pointer is valid we could use bdget() / bdput()
> > in between access, however that would mean doing that in all sysfs
> > reads/stores on the driver. Instead a generic solution for all drivers
> > is to ensure the device kobject is still valid and also the bus, if
> > a bus is present.
> > 
> > This issue does not fix a known crash, however this race was
> > spotted by Minchan Kim through code inspection upon code review
> > of another zram patch.
> > 
> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > ---
> >  drivers/base/base.h |  2 ++
> >  drivers/base/bus.c  |  4 ++--
> >  drivers/base/core.c | 42 ++++++++++++++++++++++++++++++++++++++----
> >  3 files changed, 42 insertions(+), 6 deletions(-)
> 
> Please make this an independent patch of the zram mess  and I will be
> glad to consider it for the driver core tree then.

What do you mean by making it independent?

The patch does not depend on the zram changes, and so, this can
be merged separately as-is.

If you mean that I should not mention zram on the commit log, please
let me know. I however think a concrete example is useful.

Or do you just mean that I should resend this out as a new patch
without it being attached to the zram thread?

 Luis