linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drivers/base/core: refcount kobject and bus on device attribute read / store
@ 2021-06-22 21:06 Luis Chamberlain
  2021-06-22 21:32 ` Greg KH
  2021-06-22 21:34 ` Greg KH
  0 siblings, 2 replies; 3+ messages in thread
From: Luis Chamberlain @ 2021-06-22 21:06 UTC (permalink / raw)
  To: gregkh, rafael
  Cc: jeyu, ngupta, sergey.senozhatsky.work, minchan, mcgrof, axboe,
	mbenes, jpoimboe, tglx, keescook, jikos, rostedt, peterz,
	linux-block, linux-kernel

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/bus.c  |  4 ++--
 drivers/base/core.c | 45 +++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 36d0c654ea61..21c80d7d6433 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -39,7 +39,7 @@ static struct kset *system_kset;
 static int __must_check bus_rescan_devices_helper(struct device *dev,
 						void *data);
 
-static struct bus_type *bus_get(struct bus_type *bus)
+struct bus_type *bus_get(struct bus_type *bus)
 {
 	if (bus) {
 		kset_get(&bus->p->subsys);
@@ -48,7 +48,7 @@ static struct bus_type *bus_get(struct bus_type *bus)
 	return NULL;
 }
 
-static void bus_put(struct bus_type *bus)
+void bus_put(struct bus_type *bus)
 {
 	if (bus)
 		kset_put(&bus->p->subsys);
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4a8bf8cda52b..109bbc5b6976 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2039,31 +2039,68 @@ EXPORT_SYMBOL(dev_driver_string);
 
 #define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
 
+struct bus_type *bus_get(struct bus_type *bus);
+void bus_put(struct bus_type *bus);
+
 static ssize_t dev_attr_show(struct kobject *kobj, struct attribute *attr,
 			     char *buf)
 {
-	struct device_attribute *dev_attr = to_dev_attr(attr);
-	struct device *dev = kobj_to_dev(kobj);
+	struct device_attribute *dev_attr;
+	struct device *dev;
+	struct bus_type *bus = NULL;
 	ssize_t ret = -EIO;
 
+	dev = get_device(kobj_to_dev(kobj));
+	if (!dev)
+		return ret;
+
+	if (dev->bus) {
+		bus = bus_get(dev->bus);
+		if (!bus)
+			goto out;
+	}
+
+	dev_attr = to_dev_attr(attr);
 	if (dev_attr->show)
 		ret = dev_attr->show(dev, dev_attr, buf);
 	if (ret >= (ssize_t)PAGE_SIZE) {
 		printk("dev_attr_show: %pS returned bad count\n",
 				dev_attr->show);
 	}
+
+	bus_put(bus);
+out:
+	put_device(dev);
+
 	return ret;
 }
 
 static ssize_t dev_attr_store(struct kobject *kobj, struct attribute *attr,
 			      const char *buf, size_t count)
 {
-	struct device_attribute *dev_attr = to_dev_attr(attr);
-	struct device *dev = kobj_to_dev(kobj);
+	struct device_attribute *dev_attr;
+	struct device *dev;
+	struct bus_type *bus = NULL;
 	ssize_t ret = -EIO;
 
+	dev = get_device(kobj_to_dev(kobj));
+	if (!dev)
+		return ret;
+
+	if (dev->bus) {
+		bus = bus_get(dev->bus);
+		if (!bus)
+			goto out;
+	}
+
+	dev_attr = to_dev_attr(attr);
 	if (dev_attr->store)
 		ret = dev_attr->store(dev, dev_attr, buf, count);
+
+	bus_put(bus);
+out:
+	put_device(dev);
+
 	return ret;
 }
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] drivers/base/core: refcount kobject and bus on device attribute read / store
  2021-06-22 21:06 [PATCH] drivers/base/core: refcount kobject and bus on device attribute read / store Luis Chamberlain
@ 2021-06-22 21:32 ` Greg KH
  2021-06-22 21:34 ` Greg KH
  1 sibling, 0 replies; 3+ messages in thread
From: Greg KH @ 2021-06-22 21:32 UTC (permalink / raw)
  To: Luis Chamberlain
  Cc: rafael, jeyu, ngupta, sergey.senozhatsky.work, minchan, axboe,
	mbenes, jpoimboe, tglx, keescook, jikos, rostedt, peterz,
	linux-block, linux-kernel

On Tue, Jun 22, 2021 at 02:06:59PM -0700, Luis Chamberlain wrote:
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2039,31 +2039,68 @@ EXPORT_SYMBOL(dev_driver_string);
>  
>  #define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
>  
> +struct bus_type *bus_get(struct bus_type *bus);
> +void bus_put(struct bus_type *bus);
> +

Didn't checkpatch complain about this?

We have a local .h file for stuff like this, can you please use it?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] drivers/base/core: refcount kobject and bus on device attribute read / store
  2021-06-22 21:06 [PATCH] drivers/base/core: refcount kobject and bus on device attribute read / store Luis Chamberlain
  2021-06-22 21:32 ` Greg KH
@ 2021-06-22 21:34 ` Greg KH
  1 sibling, 0 replies; 3+ messages in thread
From: Greg KH @ 2021-06-22 21:34 UTC (permalink / raw)
  To: Luis Chamberlain
  Cc: rafael, jeyu, ngupta, sergey.senozhatsky.work, minchan, axboe,
	mbenes, jpoimboe, tglx, keescook, jikos, rostedt, peterz,
	linux-block, linux-kernel

On Tue, Jun 22, 2021 at 02:06:59PM -0700, Luis Chamberlain wrote:
>  static ssize_t dev_attr_show(struct kobject *kobj, struct attribute *attr,
>  			     char *buf)
>  {
> -	struct device_attribute *dev_attr = to_dev_attr(attr);
> -	struct device *dev = kobj_to_dev(kobj);
> +	struct device_attribute *dev_attr;
> +	struct device *dev;
> +	struct bus_type *bus = NULL;
>  	ssize_t ret = -EIO;
>  
> +	dev = get_device(kobj_to_dev(kobj));
> +	if (!dev)
> +		return ret;

That check is impossible to ever hit, please recognize what things like
kobj_to_dev() really are doing when calling it.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-06-22 21:34 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-22 21:06 [PATCH] drivers/base/core: refcount kobject and bus on device attribute read / store Luis Chamberlain
2021-06-22 21:32 ` Greg KH
2021-06-22 21:34 ` Greg KH

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).