On Tue, Jun 22, 2021 at 09:44:02AM -0700, Luis Chamberlain wrote:

On Tue, Jun 22, 2021 at 09:46:26AM +0200, Greg KH wrote:

quoted

On Mon, Jun 21, 2021 at 04:36:51PM -0700, Luis Chamberlain wrote:

quoted

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.

Great, then make it a 1/1 patch.  Putting it as patch 3 here means I can
not take it as our tools pull in the full series.

thanks,

greg k-h