Re: [BUG] mm: bdi: export BDI attributes in sysfs

Re: [BUG] mm: bdi: export BDI attributes in sysfs

[Arthur Jones]

Hi Peter,  Andrew Morton asked that I add some
details and a lkml cc: to this bug report.

This problem showed up in a test I have which
uses the ipmitool to repeatedly cycle power on
3 SATA disks connected to an AHCI controller:

00:1f.2 SATA controller: Intel Corporation: Unknown device 2922 (rev 02)
(prog-)
        Subsystem: Tyan Computer: Unknown device 6631
        Flags: bus master, 66Mhz, medium devsel, latency 0, IRQ 511
        I/O ports at fc00 [size=8]
        I/O ports at f880 [size=4]
        I/O ports at f800 [size=8]
        I/O ports at f480 [size=4]
        I/O ports at f400 [size=32]
        Memory at fdfff800 (32-bit, non-prefetchable) [size=2K]
        Capabilities: [80] Message Signalled Interrupts: 64bit- Queue=0/4 Enabl+
        Capabilities: [70] Power Management version 3
        Capabilities: [a8] #12 [0010]
        Capabilities: [b0] #13 [0306]

About 1 in 4 times, when turning the power back
on for a drive, I get the BUG reported below.  The
drives are not mounted, just sitting there getting
power cycled (the test looks to make sure they are
being seen by the scsi subsystem and udev).

The ipmitool command which powers off disks 2 and 3 is:

ipmitool raw 0x06 0x52 0x07 0x36 0x00 0x01 0xfb
ipmitool raw 0x06 0x52 0x07 0x36 0x00 0x01 0xf7

The ipmitool command which powers on all disks is:

ipmitool raw 0x06 0x52 0x07 0x36 0x00 0x01 0xff

Though I expect these commands are not likely to
work anywhere but on the systems we have here...

Arthur

On Tue, May 13, 2008 at 12:04:35PM -0700, Arthur Jones wrote:
> Hi Peter,  When powering down then back up SATA drives
> on my box I found a BUG in the read_ahead_kb_show bdi
> device attribute code.  Somehow (I never tracked down
> why), the bdi is NULL occassionally in this code when
> running my test.  I'm not sure the right way to fix
> it, but the attached patch no longer causes the bug
> after many power cycles...
> 
> This code appeared in Linus' tree via:
> 
> commit cf0ca9fe5dd9e3693d935757a7b2fc50fc576554
> Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Date:   Wed Apr 30 00:54:32 2008 -0700
> 
>     mm: bdi: export BDI attributes in sysfs
>     
>     Provide a place in sysfs (/sys/class/bdi) for the backing_dev_info object.
>     This allows us to see and set the various BDI specific variables.
> 
>     [...]
> 
> The backtrace was:
> 
> BUG: unable to handle kernel NULL pointer dereference at
> 0000000000000000
> IP: [<ffffffff802694e3>] read_ahead_kb_show+0x1b/0x2d
> PGD df92c067 PUD df92b067 PMD 0 
> Oops: 0000 [1] SMP 
> CPU 0 
> Modules linked in: iptable_mangle ip_tables x_tables ipmi_devintf
> ipmi_watchdoge
> Pid: 12752, comm: udev Tainted: GF         2.6.26-rc2SMP #5
> RIP: 0010:[<ffffffff802694e3>]  [<ffffffff802694e3>]
> read_ahead_kb_show+0x1b/0xd
> RSP: 0018:ffff8100778a1e98  EFLAGS: 00010282
> RAX: 0000000000000000 RBX: ffffffff8066a4e0 RCX: 0000000000000000
> RDX: ffffffff805eb4cf RSI: 0000000000000fff RDI: ffff810099cbf000
> RBP: fffffffffffffffb R08: ffff810099cbf000 R09: 000000000009b25f
> R10: 0000000000000000 R11: 0000000000000002 R12: ffff810099dd5870
> R13: ffffffff80682ec0 R14: ffff8100778a1f50 R15: 0000000000000000
> FS:  00007fed77fff6e0(0000) GS:ffffffff806ca000(0000)
> knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> CR2: 0000000000000000 CR3: 00000000df929000 CR4: 00000000000006e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Process udev (pid: 12752, threadinfo ffff8100778a0000, task
> ffff81011e274800)
> Stack:  ffff81011fc02093 ffffffff803d88ea 0000000000000000
> ffff81011ddcc930
>  ffff81011e7c9780 ffffffff802cd2f9 0000000000000000 0000000000001000
>  000000000054bad0 ffff81009891a600 0000000000001000 000000000054bad0
> Call Trace:
>  [<ffffffff803d88ea>] ? dev_attr_show+0x1f/0x41
>  [<ffffffff802cd2f9>] ? sysfs_read_file+0xa5/0x128
>  [<ffffffff80286fca>] ? vfs_read+0xab/0x12e
>  [<ffffffff802872ce>] ? sys_read+0x45/0x6e
>  [<ffffffff8020b14b>] ? system_call_after_swapgs+0x7b/0x80
> 
> 
> Code: e5 48 89 45 00 5a 5b 5d 41 5c 4c 89 e8 41 5d c3 41 50 48 8b 87 e0
> 01 00 0 
> RIP  [<ffffffff802694e3>] read_ahead_kb_show+0x1b/0x2d
>  RSP <ffff8100778a1e98>
> CR2: 0000000000000000
> ---[ end trace 74e8b5443cce1870 ]---
> 
> Arthur

> mm: bdi: Check for NULL backing device
> 
> When powering down then powering up SATA drives,
> I found that the bdi would sometimes be NULL in
> the dev_get_drvdata(dev).  When showing device
> attributes, check for this condition to avoid
> a NULL pointer dereference.
> 
> Signed-off-by:  Arthur Jones <ajones@riverbed.com>
> 
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 7c4f9e0..cc9393f 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -109,7 +109,10 @@ static ssize_t name##_show(struct device *dev,				\
>  {									\
>  	struct backing_dev_info *bdi = dev_get_drvdata(dev);		\
>  									\
> -	return snprintf(page, PAGE_SIZE-1, "%lld\n", (long long)expr);	\
> +	if (!bdi)							\
> +		return snprintf(page, PAGE_SIZE - 1, "\n");		\
> +	else								\
> +		return snprintf(page, PAGE_SIZE-1, "%lld\n", (long long)expr);\
>  }
>  
>  BDI_SHOW(read_ahead_kb, K(bdi->ra_pages))

Re: [BUG] mm: bdi: export BDI attributes in sysfs

[Miklos Szeredi]

> About 1 in 4 times, when turning the power back
> on for a drive, I get the BUG reported below.  The
> drives are not mounted, just sitting there getting
> power cycled (the test looks to make sure they are
> being seen by the scsi subsystem and udev).

> > BUG: unable to handle kernel NULL pointer dereference at
> > 0000000000000000
> > IP: [<ffffffff802694e3>] read_ahead_kb_show+0x1b/0x2d

I suspect this is because there's a race between the show/store
functions and dev_set_drvdata() in bdi_register().

Could you try this patch to verify that this is indeed the problem?

This is not meant as a final solution, I'm sure Greg or Kay can help
find a better solution.

Thanks,
Miklos


Index: linux-2.6/mm/backing-dev.c
===================================================================
--- linux-2.6.orig/mm/backing-dev.c	2008-05-14 12:49:33.000000000 +0200
+++ linux-2.6/mm/backing-dev.c	2008-05-15 20:37:55.000000000 +0200
@@ -15,6 +15,7 @@ static struct class *bdi_class;
 #include <linux/seq_file.h>
 
 static struct dentry *bdi_debug_root;
+static DEFINE_MUTEX(bdi_dev_mutex);
 
 static void bdi_debug_init(void)
 {
@@ -84,11 +85,19 @@ static inline void bdi_debug_unregister(
 }
 #endif
 
+static struct backing_dev_info *dev_get_bdi(struct device *dev)
+{
+	mutex_lock(&bdi_dev_mutex);
+	mutex_unlock(&bdi_dev_mutex);
+
+	return dev_get_drvdata(dev);
+}
+
 static ssize_t read_ahead_kb_store(struct device *dev,
 				  struct device_attribute *attr,
 				  const char *buf, size_t count)
 {
-	struct backing_dev_info *bdi = dev_get_drvdata(dev);
+	struct backing_dev_info *bdi = dev_get_bdi(dev);
 	char *end;
 	unsigned long read_ahead_kb;
 	ssize_t ret = -EINVAL;
@@ -107,7 +116,7 @@ static ssize_t read_ahead_kb_store(struc
 static ssize_t name##_show(struct device *dev,				\
 			   struct device_attribute *attr, char *page)	\
 {									\
-	struct backing_dev_info *bdi = dev_get_drvdata(dev);		\
+	struct backing_dev_info *bdi = dev_get_bdi(dev);		\
 									\
 	return snprintf(page, PAGE_SIZE-1, "%lld\n", (long long)expr);	\
 }
@@ -117,7 +126,7 @@ BDI_SHOW(read_ahead_kb, K(bdi->ra_pages)
 static ssize_t min_ratio_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
-	struct backing_dev_info *bdi = dev_get_drvdata(dev);
+	struct backing_dev_info *bdi = dev_get_bdi(dev);
 	char *end;
 	unsigned int ratio;
 	ssize_t ret = -EINVAL;
@@ -135,7 +144,7 @@ BDI_SHOW(min_ratio, bdi->min_ratio)
 static ssize_t max_ratio_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
-	struct backing_dev_info *bdi = dev_get_drvdata(dev);
+	struct backing_dev_info *bdi = dev_get_bdi(dev);
 	char *end;
 	unsigned int ratio;
 	ssize_t ret = -EINVAL;
@@ -184,6 +193,7 @@ int bdi_register(struct backing_dev_info
 	if (!name)
 		return -ENOMEM;
 
+	mutex_lock(&bdi_dev_mutex);
 	dev = device_create(bdi_class, parent, MKDEV(0, 0), name);
 	if (IS_ERR(dev)) {
 		ret = PTR_ERR(dev);
@@ -195,6 +205,7 @@ int bdi_register(struct backing_dev_info
 	bdi_debug_register(bdi, name);
 
 exit:
+	mutex_unlock(&bdi_dev_mutex);
 	kfree(name);
 	return ret;
 }

Re: [BUG] mm: bdi: export BDI attributes in sysfs

[Linus Torvalds]

On Thu, 15 May 2008, Miklos Szeredi wrote:
> 
> This is not meant as a final solution, I'm sure Greg or Kay can help
> find a better solution.

Yeah, don't do this:

> +static struct backing_dev_info *dev_get_bdi(struct device *dev)
> +{
> +	mutex_lock(&bdi_dev_mutex);
> +	mutex_unlock(&bdi_dev_mutex);
> +
> +	return dev_get_drvdata(dev);
> +}

This kind of serialization can often hide bugs, and in some cases even 
make them go away (if the caller of the function means that the device is 
pinned and the tear-down cannot happen, for example), but it's really 
really bad form.

In order to use locking in a repeatable manner that is easy to think 
about, you really need to *keep* the lock until you've stopped using the 
data (or have dereferenced it into a stable form - eg maybe accessing the 
pointer itself needs locking, but some individual data read _off_ the 
pointer does not).

So the above kind of "get and release the lock" does obviously serialize 
wrt others that hold the lock, but it's still wrong.

>  static ssize_t read_ahead_kb_store(struct device *dev,
>  				  struct device_attribute *attr,
>  				  const char *buf, size_t count)
>  {
> -	struct backing_dev_info *bdi = dev_get_drvdata(dev);
> +	struct backing_dev_info *bdi = dev_get_bdi(dev);
>  	char *end;
>  	unsigned long read_ahead_kb;
>  	ssize_t ret = -EINVAL;

You should just get the lock in the routines that acually use this thing.

Or, if the "struct backing_dev_info *" pointer itself is stable, and it 
really is just the access from "struct device" that needs protection, then 
at the very least it should have been

	static struct backing_dev_info *dev_get_bdi(struct device *dev)
	{
		struct backing_dev_info *bdi;

		mutex_lock(&bdi_dev_mutex);
		bdi = dev_get_drvdata(dev);
		mutex_unlock(&bdi_dev_mutex);
		return bdi;
	}

which makes it clear that it's the "dev_get_drvdata()" that needs the 
locking, not the bdi pointer itself.

		Linus

Re: [BUG] mm: bdi: export BDI attributes in sysfs

[Miklos Szeredi]

> On Thu, 15 May 2008, Miklos Szeredi wrote:
> > 
> > This is not meant as a final solution, I'm sure Greg or Kay can help
> > find a better solution.
> 
> Yeah, don't do this:
> 
> > +static struct backing_dev_info *dev_get_bdi(struct device *dev)
> > +{
> > +	mutex_lock(&bdi_dev_mutex);
> > +	mutex_unlock(&bdi_dev_mutex);
> > +
> > +	return dev_get_drvdata(dev);
> > +}
> 
> This kind of serialization can often hide bugs, and in some cases even 
> make them go away (if the caller of the function means that the device is 
> pinned and the tear-down cannot happen, for example), but it's really 
> really bad form.

Yeah, I know.

> In order to use locking in a repeatable manner that is easy to think 
> about, you really need to *keep* the lock until you've stopped using the 
> data (or have dereferenced it into a stable form - eg maybe accessing the 
> pointer itself needs locking, but some individual data read _off_ the 
> pointer does not).
> 
> So the above kind of "get and release the lock" does obviously serialize 
> wrt others that hold the lock, but it's still wrong.
> 
> >  static ssize_t read_ahead_kb_store(struct device *dev,
> >  				  struct device_attribute *attr,
> >  				  const char *buf, size_t count)
> >  {
> > -	struct backing_dev_info *bdi = dev_get_drvdata(dev);
> > +	struct backing_dev_info *bdi = dev_get_bdi(dev);
> >  	char *end;
> >  	unsigned long read_ahead_kb;
> >  	ssize_t ret = -EINVAL;
> 
> You should just get the lock in the routines that acually use this thing.
> 
> Or, if the "struct backing_dev_info *" pointer itself is stable, and it 
> really is just the access from "struct device" that needs protection, then 
> at the very least it should have been

Actually nothing should need protection.  The only problem AFAICS is
that the device_create()/dev_set_drvdata() interface is racy: somebody
can come in after the device has been created but before drvdata has
been set, and then we are in trouble.

I'm quite sure this is not the only place in the kernel where this
would be an issue, that's why I expect the sysfs guys to have some
sort of alternative solution, that doesn't necessarily involve adding
a new mutex.

Miklos

Re: [BUG] mm: bdi: export BDI attributes in sysfs

[Linus Torvalds]

On Thu, 15 May 2008, Miklos Szeredi wrote:
> 
> Actually nothing should need protection.  The only problem AFAICS is
> that the device_create()/dev_set_drvdata() interface is racy: somebody
> can come in after the device has been created but before drvdata has
> been set, and then we are in trouble.

Well, I'm not sure that the locking should be at that level. Maybe the 
locking *should* be in the driver that does this. It may need to do other 
setup too, after all.

Of course, doing a device_create_drvdata() thing might be the right 
solution, at least part of the time. Greg?

		Linus

Re: [BUG] mm: bdi: export BDI attributes in sysfs

[Greg KH]

On Thu, May 15, 2008 at 12:54:57PM -0700, Linus Torvalds wrote:
> 
> 
> On Thu, 15 May 2008, Miklos Szeredi wrote:
> > 
> > Actually nothing should need protection.  The only problem AFAICS is
> > that the device_create()/dev_set_drvdata() interface is racy: somebody
> > can come in after the device has been created but before drvdata has
> > been set, and then we are in trouble.
> 
> Well, I'm not sure that the locking should be at that level. Maybe the 
> locking *should* be in the driver that does this. It may need to do other 
> setup too, after all.
> 
> Of course, doing a device_create_drvdata() thing might be the right 
> solution, at least part of the time. Greg?

Yes, you are right, let me go knock that together...

thanks,

greg k-h

Re: [BUG] mm: bdi: export BDI attributes in sysfs

[Greg KH]

On Thu, May 15, 2008 at 12:54:57PM -0700, Linus Torvalds wrote:
> 
> 
> On Thu, 15 May 2008, Miklos Szeredi wrote:
> > 
> > Actually nothing should need protection.  The only problem AFAICS is
> > that the device_create()/dev_set_drvdata() interface is racy: somebody
> > can come in after the device has been created but before drvdata has
> > been set, and then we are in trouble.
> 
> Well, I'm not sure that the locking should be at that level. Maybe the 
> locking *should* be in the driver that does this. It may need to do other 
> setup too, after all.
> 
> Of course, doing a device_create_drvdata() thing might be the right 
> solution, at least part of the time. Greg?

Here's a patch that is build tested only.

Can someone who can reproduce this let me know if it solves the problem?

thanks,

greg k-h

--------------------

Subject: Driver core: add device_create_vargs and device_create_drvdata

We want to have the drvdata field set properly when creating the device
as sysfs callbacks can assume it is present and it can race the later
setting of this field.

So, create two new functions, deviec_create_vargs() and
device_create_drvdata() that take this new field.

Also move the mm/backing-dev.c code to use it as it is showing this
problem today.

device_create_drvdata() will go away in 2.6.27 as the drvdata field will
just be moved to the device_create() call as it should be.

Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/base/core.c    |   85 +++++++++++++++++++++++++++++++++++++++++++++----
 include/linux/device.h |   12 ++++++
 mm/backing-dev.c       |   12 +-----
 3 files changed, 93 insertions(+), 16 deletions(-)

--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1163,11 +1163,13 @@ static void device_create_release(struct
 }
 
 /**
- * device_create - creates a device and registers it with sysfs
+ * device_create_vargs - creates a device and registers it with sysfs
  * @class: pointer to the struct class that this device should be registered to
  * @parent: pointer to the parent struct device of this new device, if any
  * @devt: the dev_t for the char device to be added
+ * @drvdata: the data to be added to the device for callbacks
  * @fmt: string for the device's name
+ * @args: va_list for the device's name
  *
  * This function can be used by char device classes.  A struct device
  * will be created in sysfs, registered to the specified class.
@@ -1183,10 +1185,10 @@ static void device_create_release(struct
  * Note: the struct class passed to this function must have previously
  * been created with a call to class_create().
  */
-struct device *device_create(struct class *class, struct device *parent,
-			     dev_t devt, const char *fmt, ...)
+struct device *device_create_vargs(struct class *class, struct device *parent,
+				   dev_t devt, void *drvdata, const char *fmt,
+				   va_list args)
 {
-	va_list args;
 	struct device *dev = NULL;
 	int retval = -ENODEV;
 
@@ -1203,10 +1205,9 @@ struct device *device_create(struct clas
 	dev->class = class;
 	dev->parent = parent;
 	dev->release = device_create_release;
+	dev_set_drvdata(dev, drvdata);
 
-	va_start(args, fmt);
 	vsnprintf(dev->bus_id, BUS_ID_SIZE, fmt, args);
-	va_end(args);
 	retval = device_register(dev);
 	if (retval)
 		goto error;
@@ -1217,6 +1218,78 @@ error:
 	kfree(dev);
 	return ERR_PTR(retval);
 }
+EXPORT_SYMBOL_GPL(device_create_vargs);
+
+/**
+ * device_create_drvdata - creates a device and registers it with sysfs
+ * @class: pointer to the struct class that this device should be registered to
+ * @parent: pointer to the parent struct device of this new device, if any
+ * @devt: the dev_t for the char device to be added
+ * @drvdata: the data to be added to the device for callbacks
+ * @fmt: string for the device's name
+ *
+ * This function can be used by char device classes.  A struct device
+ * will be created in sysfs, registered to the specified class.
+ *
+ * A "dev" file will be created, showing the dev_t for the device, if
+ * the dev_t is not 0,0.
+ * If a pointer to a parent struct device is passed in, the newly created
+ * struct device will be a child of that device in sysfs.
+ * The pointer to the struct device will be returned from the call.
+ * Any further sysfs files that might be required can be created using this
+ * pointer.
+ *
+ * Note: the struct class passed to this function must have previously
+ * been created with a call to class_create().
+ */
+struct device *device_create_drvdata(struct class *class,
+				     struct device *parent,
+				     dev_t devt,
+				     void *drvdata,
+				     const char *fmt, ...)
+{
+	va_list vargs;
+	struct device *dev;
+
+	va_start(vargs, fmt);
+	dev = device_create_vargs(class, parent, devt, drvdata, fmt, vargs);
+	va_end(vargs);
+	return dev;
+}
+EXPORT_SYMBOL_GPL(device_create_drvdata);
+
+/**
+ * device_create - creates a device and registers it with sysfs
+ * @class: pointer to the struct class that this device should be registered to
+ * @parent: pointer to the parent struct device of this new device, if any
+ * @devt: the dev_t for the char device to be added
+ * @fmt: string for the device's name
+ *
+ * This function can be used by char device classes.  A struct device
+ * will be created in sysfs, registered to the specified class.
+ *
+ * A "dev" file will be created, showing the dev_t for the device, if
+ * the dev_t is not 0,0.
+ * If a pointer to a parent struct device is passed in, the newly created
+ * struct device will be a child of that device in sysfs.
+ * The pointer to the struct device will be returned from the call.
+ * Any further sysfs files that might be required can be created using this
+ * pointer.
+ *
+ * Note: the struct class passed to this function must have previously
+ * been created with a call to class_create().
+ */
+struct device *device_create(struct class *class, struct device *parent,
+			     dev_t devt, const char *fmt, ...)
+{
+	va_list vargs;
+	struct device *dev;
+
+	va_start(vargs, fmt);
+	dev = device_create_vargs(class, parent, devt, NULL, fmt, vargs);
+	va_end(vargs);
+	return dev;
+}
 EXPORT_SYMBOL_GPL(device_create);
 
 static int __match_devt(struct device *dev, void *data)
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -456,9 +456,21 @@ extern int __must_check device_reprobe(s
 /*
  * Easy functions for dynamically creating devices on the fly
  */
+extern struct device *device_create_vargs(struct class *cls,
+					  struct device *parent,
+					  dev_t devt,
+					  void *drvdata,
+					  const char *fmt,
+					  va_list vargs);
 extern struct device *device_create(struct class *cls, struct device *parent,
 				    dev_t devt, const char *fmt, ...)
 				    __attribute__((format(printf, 4, 5)));
+extern struct device *device_create_drvdata(struct class *cls,
+					    struct device *parent,
+					    dev_t devt,
+					    void *drvdata,
+					    const char *fmt, ...)
+				    __attribute__((format(printf, 5, 6)));
 extern void device_destroy(struct class *cls, dev_t devt);
 
 /*
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -172,30 +172,22 @@ postcore_initcall(bdi_class_init);
 int bdi_register(struct backing_dev_info *bdi, struct device *parent,
 		const char *fmt, ...)
 {
-	char *name;
 	va_list args;
 	int ret = 0;
 	struct device *dev;
 
 	va_start(args, fmt);
-	name = kvasprintf(GFP_KERNEL, fmt, args);
+	dev = device_create_vargs(bdi_class, parent, MKDEV(0, 0), bdi, fmt, args);
 	va_end(args);
-
-	if (!name)
-		return -ENOMEM;
-
-	dev = device_create(bdi_class, parent, MKDEV(0, 0), name);
 	if (IS_ERR(dev)) {
 		ret = PTR_ERR(dev);
 		goto exit;
 	}
 
 	bdi->dev = dev;
-	dev_set_drvdata(bdi->dev, bdi);
-	bdi_debug_register(bdi, name);
+	bdi_debug_register(bdi, dev_name(dev));
 
 exit:
-	kfree(name);
 	return ret;
 }
 EXPORT_SYMBOL(bdi_register);

Re: [BUG] mm: bdi: export BDI attributes in sysfs

[Arthur Jones]

Hi Greg, ...

On Thu, May 15, 2008 at 02:02:54PM -0700, Greg KH wrote:
> On Thu, May 15, 2008 at 12:54:57PM -0700, Linus Torvalds wrote:
> >
> >
> > On Thu, 15 May 2008, Miklos Szeredi wrote:
> > >
> > > Actually nothing should need protection.  The only problem AFAICS is
> > > that the device_create()/dev_set_drvdata() interface is racy: somebody
> > > can come in after the device has been created but before drvdata has
> > > been set, and then we are in trouble.
> >
> > Well, I'm not sure that the locking should be at that level. Maybe the
> > locking *should* be in the driver that does this. It may need to do other
> > setup too, after all.
> >
> > Of course, doing a device_create_drvdata() thing might be the right
> > solution, at least part of the time. Greg?
> 
> Here's a patch that is build tested only.
> 
> Can someone who can reproduce this let me know if it solves the problem?

Yes, this patch survives many power cycles
without hitting the BUG...

Thanks for looking into this...

Arthur
> --------------------
> 
> Subject: Driver core: add device_create_vargs and device_create_drvdata
> 
> We want to have the drvdata field set properly when creating the device
> as sysfs callbacks can assume it is present and it can race the later
> setting of this field.
> 
> So, create two new functions, deviec_create_vargs() and
> device_create_drvdata() that take this new field.
> 
> Also move the mm/backing-dev.c code to use it as it is showing this
> problem today.
> 
> device_create_drvdata() will go away in 2.6.27 as the drvdata field will
> just be moved to the device_create() call as it should be.
> 
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> 
> ---
>  drivers/base/core.c    |   85 +++++++++++++++++++++++++++++++++++++++++++++----
>  include/linux/device.h |   12 ++++++
>  mm/backing-dev.c       |   12 +-----
>  3 files changed, 93 insertions(+), 16 deletions(-)
> 
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1163,11 +1163,13 @@ static void device_create_release(struct
>  }
> 
>  /**
> - * device_create - creates a device and registers it with sysfs
> + * device_create_vargs - creates a device and registers it with sysfs
>   * @class: pointer to the struct class that this device should be registered to
>   * @parent: pointer to the parent struct device of this new device, if any
>   * @devt: the dev_t for the char device to be added
> + * @drvdata: the data to be added to the device for callbacks
>   * @fmt: string for the device's name
> + * @args: va_list for the device's name
>   *
>   * This function can be used by char device classes.  A struct device
>   * will be created in sysfs, registered to the specified class.
> @@ -1183,10 +1185,10 @@ static void device_create_release(struct
>   * Note: the struct class passed to this function must have previously
>   * been created with a call to class_create().
>   */
> -struct device *device_create(struct class *class, struct device *parent,
> -                            dev_t devt, const char *fmt, ...)
> +struct device *device_create_vargs(struct class *class, struct device *parent,
> +                                  dev_t devt, void *drvdata, const char *fmt,
> +                                  va_list args)
>  {
> -       va_list args;
>         struct device *dev = NULL;
>         int retval = -ENODEV;
> 
> @@ -1203,10 +1205,9 @@ struct device *device_create(struct clas
>         dev->class = class;
>         dev->parent = parent;
>         dev->release = device_create_release;
> +       dev_set_drvdata(dev, drvdata);
> 
> -       va_start(args, fmt);
>         vsnprintf(dev->bus_id, BUS_ID_SIZE, fmt, args);
> -       va_end(args);
>         retval = device_register(dev);
>         if (retval)
>                 goto error;
> @@ -1217,6 +1218,78 @@ error:
>         kfree(dev);
>         return ERR_PTR(retval);
>  }
> +EXPORT_SYMBOL_GPL(device_create_vargs);
> +
> +/**
> + * device_create_drvdata - creates a device and registers it with sysfs
> + * @class: pointer to the struct class that this device should be registered to
> + * @parent: pointer to the parent struct device of this new device, if any
> + * @devt: the dev_t for the char device to be added
> + * @drvdata: the data to be added to the device for callbacks
> + * @fmt: string for the device's name
> + *
> + * This function can be used by char device classes.  A struct device
> + * will be created in sysfs, registered to the specified class.
> + *
> + * A "dev" file will be created, showing the dev_t for the device, if
> + * the dev_t is not 0,0.
> + * If a pointer to a parent struct device is passed in, the newly created
> + * struct device will be a child of that device in sysfs.
> + * The pointer to the struct device will be returned from the call.
> + * Any further sysfs files that might be required can be created using this
> + * pointer.
> + *
> + * Note: the struct class passed to this function must have previously
> + * been created with a call to class_create().
> + */
> +struct device *device_create_drvdata(struct class *class,
> +                                    struct device *parent,
> +                                    dev_t devt,
> +                                    void *drvdata,
> +                                    const char *fmt, ...)
> +{
> +       va_list vargs;
> +       struct device *dev;
> +
> +       va_start(vargs, fmt);
> +       dev = device_create_vargs(class, parent, devt, drvdata, fmt, vargs);
> +       va_end(vargs);
> +       return dev;
> +}
> +EXPORT_SYMBOL_GPL(device_create_drvdata);
> +
> +/**
> + * device_create - creates a device and registers it with sysfs
> + * @class: pointer to the struct class that this device should be registered to
> + * @parent: pointer to the parent struct device of this new device, if any
> + * @devt: the dev_t for the char device to be added
> + * @fmt: string for the device's name
> + *
> + * This function can be used by char device classes.  A struct device
> + * will be created in sysfs, registered to the specified class.
> + *
> + * A "dev" file will be created, showing the dev_t for the device, if
> + * the dev_t is not 0,0.
> + * If a pointer to a parent struct device is passed in, the newly created
> + * struct device will be a child of that device in sysfs.
> + * The pointer to the struct device will be returned from the call.
> + * Any further sysfs files that might be required can be created using this
> + * pointer.
> + *
> + * Note: the struct class passed to this function must have previously
> + * been created with a call to class_create().
> + */
> +struct device *device_create(struct class *class, struct device *parent,
> +                            dev_t devt, const char *fmt, ...)
> +{
> +       va_list vargs;
> +       struct device *dev;
> +
> +       va_start(vargs, fmt);
> +       dev = device_create_vargs(class, parent, devt, NULL, fmt, vargs);
> +       va_end(vargs);
> +       return dev;
> +}
>  EXPORT_SYMBOL_GPL(device_create);
> 
>  static int __match_devt(struct device *dev, void *data)
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -456,9 +456,21 @@ extern int __must_check device_reprobe(s
>  /*
>   * Easy functions for dynamically creating devices on the fly
>   */
> +extern struct device *device_create_vargs(struct class *cls,
> +                                         struct device *parent,
> +                                         dev_t devt,
> +                                         void *drvdata,
> +                                         const char *fmt,
> +                                         va_list vargs);
>  extern struct device *device_create(struct class *cls, struct device *parent,
>                                     dev_t devt, const char *fmt, ...)
>                                     __attribute__((format(printf, 4, 5)));
> +extern struct device *device_create_drvdata(struct class *cls,
> +                                           struct device *parent,
> +                                           dev_t devt,
> +                                           void *drvdata,
> +                                           const char *fmt, ...)
> +                                   __attribute__((format(printf, 5, 6)));
>  extern void device_destroy(struct class *cls, dev_t devt);
> 
>  /*
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -172,30 +172,22 @@ postcore_initcall(bdi_class_init);
>  int bdi_register(struct backing_dev_info *bdi, struct device *parent,
>                 const char *fmt, ...)
>  {
> -       char *name;
>         va_list args;
>         int ret = 0;
>         struct device *dev;
> 
>         va_start(args, fmt);
> -       name = kvasprintf(GFP_KERNEL, fmt, args);
> +       dev = device_create_vargs(bdi_class, parent, MKDEV(0, 0), bdi, fmt, args);
>         va_end(args);
> -
> -       if (!name)
> -               return -ENOMEM;
> -
> -       dev = device_create(bdi_class, parent, MKDEV(0, 0), name);
>         if (IS_ERR(dev)) {
>                 ret = PTR_ERR(dev);
>                 goto exit;
>         }
> 
>         bdi->dev = dev;
> -       dev_set_drvdata(bdi->dev, bdi);
> -       bdi_debug_register(bdi, name);
> +       bdi_debug_register(bdi, dev_name(dev));
> 
>  exit:
> -       kfree(name);
>         return ret;
>  }
>  EXPORT_SYMBOL(bdi_register);

Re: [BUG] mm: bdi: export BDI attributes in sysfs

[Greg KH]

On Thu, May 15, 2008 at 03:05:10PM -0700, Arthur Jones wrote:
> Hi Greg, ...
> 
> On Thu, May 15, 2008 at 02:02:54PM -0700, Greg KH wrote:
> > On Thu, May 15, 2008 at 12:54:57PM -0700, Linus Torvalds wrote:
> > >
> > >
> > > On Thu, 15 May 2008, Miklos Szeredi wrote:
> > > >
> > > > Actually nothing should need protection.  The only problem AFAICS is
> > > > that the device_create()/dev_set_drvdata() interface is racy: somebody
> > > > can come in after the device has been created but before drvdata has
> > > > been set, and then we are in trouble.
> > >
> > > Well, I'm not sure that the locking should be at that level. Maybe the
> > > locking *should* be in the driver that does this. It may need to do other
> > > setup too, after all.
> > >
> > > Of course, doing a device_create_drvdata() thing might be the right
> > > solution, at least part of the time. Greg?
> > 
> > Here's a patch that is build tested only.
> > 
> > Can someone who can reproduce this let me know if it solves the problem?
> 
> Yes, this patch survives many power cycles
> without hitting the BUG...
> 
> Thanks for looking into this...

Great!

Thanks for testing this, I'll clean it up a bit more and send it to
Linus later this evening.  Gotta go take the kids to swimming lessons...

thanks,

greg k-h

Re: [BUG] mm: bdi: export BDI attributes in sysfs

[Greg KH]

On Thu, May 15, 2008 at 03:05:10PM -0700, Arthur Jones wrote:
> Hi Greg, ...
> 
> On Thu, May 15, 2008 at 02:02:54PM -0700, Greg KH wrote:
> > On Thu, May 15, 2008 at 12:54:57PM -0700, Linus Torvalds wrote:
> > >
> > >
> > > On Thu, 15 May 2008, Miklos Szeredi wrote:
> > > >
> > > > Actually nothing should need protection.  The only problem AFAICS is
> > > > that the device_create()/dev_set_drvdata() interface is racy: somebody
> > > > can come in after the device has been created but before drvdata has
> > > > been set, and then we are in trouble.
> > >
> > > Well, I'm not sure that the locking should be at that level. Maybe the
> > > locking *should* be in the driver that does this. It may need to do other
> > > setup too, after all.
> > >
> > > Of course, doing a device_create_drvdata() thing might be the right
> > > solution, at least part of the time. Greg?
> > 
> > Here's a patch that is build tested only.
> > 
> > Can someone who can reproduce this let me know if it solves the problem?
> 
> Yes, this patch survives many power cycles
> without hitting the BUG...

Thanks again.

I've audited the whole tree and only found one other place with this
problem, the display driver class.  I've build up patches for this and
added it to my tree.  I'll let them take a spin in -next to verify I
didn't do anything stupid before sending them tomorrow to Linus.

thanks,

greg k-h

Re: [BUG] mm: bdi: export BDI attributes in sysfs

[Greg KH]

On Thu, May 15, 2008 at 09:27:50PM +0200, Miklos Szeredi wrote:
> > On Thu, 15 May 2008, Miklos Szeredi wrote:
> > > 
> > > This is not meant as a final solution, I'm sure Greg or Kay can help
> > > find a better solution.
> > 
> > Yeah, don't do this:
> > 
> > > +static struct backing_dev_info *dev_get_bdi(struct device *dev)
> > > +{
> > > +	mutex_lock(&bdi_dev_mutex);
> > > +	mutex_unlock(&bdi_dev_mutex);
> > > +
> > > +	return dev_get_drvdata(dev);
> > > +}
> > 
> > This kind of serialization can often hide bugs, and in some cases even 
> > make them go away (if the caller of the function means that the device is 
> > pinned and the tear-down cannot happen, for example), but it's really 
> > really bad form.
> 
> Yeah, I know.
> 
> > In order to use locking in a repeatable manner that is easy to think 
> > about, you really need to *keep* the lock until you've stopped using the 
> > data (or have dereferenced it into a stable form - eg maybe accessing the 
> > pointer itself needs locking, but some individual data read _off_ the 
> > pointer does not).
> > 
> > So the above kind of "get and release the lock" does obviously serialize 
> > wrt others that hold the lock, but it's still wrong.
> > 
> > >  static ssize_t read_ahead_kb_store(struct device *dev,
> > >  				  struct device_attribute *attr,
> > >  				  const char *buf, size_t count)
> > >  {
> > > -	struct backing_dev_info *bdi = dev_get_drvdata(dev);
> > > +	struct backing_dev_info *bdi = dev_get_bdi(dev);
> > >  	char *end;
> > >  	unsigned long read_ahead_kb;
> > >  	ssize_t ret = -EINVAL;
> > 
> > You should just get the lock in the routines that acually use this thing.
> > 
> > Or, if the "struct backing_dev_info *" pointer itself is stable, and it 
> > really is just the access from "struct device" that needs protection, then 
> > at the very least it should have been
> 
> Actually nothing should need protection.  The only problem AFAICS is
> that the device_create()/dev_set_drvdata() interface is racy: somebody
> can come in after the device has been created but before drvdata has
> been set, and then we are in trouble.

Then that needs to be fixed in the code that registered the device
itself.  The driver core knows nothing about this at all.  Is this
something in the block layer?

> I'm quite sure this is not the only place in the kernel where this
> would be an issue, that's why I expect the sysfs guys to have some
> sort of alternative solution, that doesn't necessarily involve adding
> a new mutex.

It should be fixed in the bus/subsystem that is creating the device, the
pointer must be set up before device_register() is called (or
device_add()).

thanks,

greg k-h

Re: [BUG] mm: bdi: export BDI attributes in sysfs

[Greg KH]

On Thu, May 15, 2008 at 01:37:48PM -0700, Greg KH wrote:
> On Thu, May 15, 2008 at 09:27:50PM +0200, Miklos Szeredi wrote:
> > > On Thu, 15 May 2008, Miklos Szeredi wrote:
> > > > 
> > > > This is not meant as a final solution, I'm sure Greg or Kay can help
> > > > find a better solution.
> > > 
> > > Yeah, don't do this:
> > > 
> > > > +static struct backing_dev_info *dev_get_bdi(struct device *dev)
> > > > +{
> > > > +	mutex_lock(&bdi_dev_mutex);
> > > > +	mutex_unlock(&bdi_dev_mutex);
> > > > +
> > > > +	return dev_get_drvdata(dev);
> > > > +}
> > > 
> > > This kind of serialization can often hide bugs, and in some cases even 
> > > make them go away (if the caller of the function means that the device is 
> > > pinned and the tear-down cannot happen, for example), but it's really 
> > > really bad form.
> > 
> > Yeah, I know.
> > 
> > > In order to use locking in a repeatable manner that is easy to think 
> > > about, you really need to *keep* the lock until you've stopped using the 
> > > data (or have dereferenced it into a stable form - eg maybe accessing the 
> > > pointer itself needs locking, but some individual data read _off_ the 
> > > pointer does not).
> > > 
> > > So the above kind of "get and release the lock" does obviously serialize 
> > > wrt others that hold the lock, but it's still wrong.
> > > 
> > > >  static ssize_t read_ahead_kb_store(struct device *dev,
> > > >  				  struct device_attribute *attr,
> > > >  				  const char *buf, size_t count)
> > > >  {
> > > > -	struct backing_dev_info *bdi = dev_get_drvdata(dev);
> > > > +	struct backing_dev_info *bdi = dev_get_bdi(dev);
> > > >  	char *end;
> > > >  	unsigned long read_ahead_kb;
> > > >  	ssize_t ret = -EINVAL;
> > > 
> > > You should just get the lock in the routines that acually use this thing.
> > > 
> > > Or, if the "struct backing_dev_info *" pointer itself is stable, and it 
> > > really is just the access from "struct device" that needs protection, then 
> > > at the very least it should have been
> > 
> > Actually nothing should need protection.  The only problem AFAICS is
> > that the device_create()/dev_set_drvdata() interface is racy: somebody
> > can come in after the device has been created but before drvdata has
> > been set, and then we are in trouble.
> 
> Then that needs to be fixed in the code that registered the device
> itself.  The driver core knows nothing about this at all.  Is this
> something in the block layer?
> 
> > I'm quite sure this is not the only place in the kernel where this
> > would be an issue, that's why I expect the sysfs guys to have some
> > sort of alternative solution, that doesn't necessarily involve adding
> > a new mutex.
> 
> It should be fixed in the bus/subsystem that is creating the device, the
> pointer must be set up before device_register() is called (or
> device_add()).

Oh nevermind, Linus is right.  We need to just add another parameter to
device_create() for this field.  For now I can make up a
device_create_drvdata() like Linus suggested.  Give me a few minutes...

thanks,

greg k-h

Re: [BUG] mm: bdi: export BDI attributes in sysfs

[Arthur Jones]

Hi Miklos,  ...

On Thu, May 15, 2008 at 11:53:02AM -0700, Miklos Szeredi wrote:
> > About 1 in 4 times, when turning the power back
> > on for a drive, I get the BUG reported below.  The
> > drives are not mounted, just sitting there getting
> > power cycled (the test looks to make sure they are
> > being seen by the scsi subsystem and udev).
> 
> > > BUG: unable to handle kernel NULL pointer dereference at
> > > 0000000000000000
> > > IP: [<ffffffff802694e3>] read_ahead_kb_show+0x1b/0x2d
> 
> I suspect this is because there's a race between the show/store
> functions and dev_set_drvdata() in bdi_register().
> 
> Could you try this patch to verify that this is indeed the problem?

Well, the patch had more conflicts than Obama at an
RNC fundraiser (spaces/tabs?) and didn't compile at first
-- but w/ some minor tweaks it finally succumbed and after
many power cycles I no longer see the problem.

Thanks for your help...

Arthur

> Index: linux-2.6/mm/backing-dev.c
> ===================================================================
> --- linux-2.6.orig/mm/backing-dev.c     2008-05-14 12:49:33.000000000 +0200
> +++ linux-2.6/mm/backing-dev.c  2008-05-15 20:37:55.000000000 +0200
> @@ -15,6 +15,7 @@ static struct class *bdi_class;
>  #include <linux/seq_file.h>
> 
>  static struct dentry *bdi_debug_root;
> +static DEFINE_MUTEX(bdi_dev_mutex);
> 
>  static void bdi_debug_init(void)
>  {
> @@ -84,11 +85,19 @@ static inline void bdi_debug_unregister(
>  }
>  #endif
> 
> +static struct backing_dev_info *dev_get_bdi(struct device *dev)
> +{
> +       mutex_lock(&bdi_dev_mutex);
> +       mutex_unlock(&bdi_dev_mutex);
> +
> +       return dev_get_drvdata(dev);
> +}
> +
>  static ssize_t read_ahead_kb_store(struct device *dev,
>                                   struct device_attribute *attr,
>                                   const char *buf, size_t count)
>  {
> -       struct backing_dev_info *bdi = dev_get_drvdata(dev);
> +       struct backing_dev_info *bdi = dev_get_bdi(dev);
>         char *end;
>         unsigned long read_ahead_kb;
>         ssize_t ret = -EINVAL;
> @@ -107,7 +116,7 @@ static ssize_t read_ahead_kb_store(struc
>  static ssize_t name##_show(struct device *dev,                         \
>                            struct device_attribute *attr, char *page)   \
>  {                                                                      \
> -       struct backing_dev_info *bdi = dev_get_drvdata(dev);            \
> +       struct backing_dev_info *bdi = dev_get_bdi(dev);                \
>                                                                         \
>         return snprintf(page, PAGE_SIZE-1, "%lld\n", (long long)expr);  \
>  }
> @@ -117,7 +126,7 @@ BDI_SHOW(read_ahead_kb, K(bdi->ra_pages)
>  static ssize_t min_ratio_store(struct device *dev,
>                 struct device_attribute *attr, const char *buf, size_t count)
>  {
> -       struct backing_dev_info *bdi = dev_get_drvdata(dev);
> +       struct backing_dev_info *bdi = dev_get_bdi(dev);
>         char *end;
>         unsigned int ratio;
>         ssize_t ret = -EINVAL;
> @@ -135,7 +144,7 @@ BDI_SHOW(min_ratio, bdi->min_ratio)
>  static ssize_t max_ratio_store(struct device *dev,
>                 struct device_attribute *attr, const char *buf, size_t count)
>  {
> -       struct backing_dev_info *bdi = dev_get_drvdata(dev);
> +       struct backing_dev_info *bdi = dev_get_bdi(dev);
>         char *end;
>         unsigned int ratio;
>         ssize_t ret = -EINVAL;
> @@ -184,6 +193,7 @@ int bdi_register(struct backing_dev_info
>         if (!name)
>                 return -ENOMEM;
> 
> +       mutex_lock(&bdi_dev_mutex);
>         dev = device_create(bdi_class, parent, MKDEV(0, 0), name);
>         if (IS_ERR(dev)) {
>                 ret = PTR_ERR(dev);
> @@ -195,6 +205,7 @@ int bdi_register(struct backing_dev_info
>         bdi_debug_register(bdi, name);
> 
>  exit:
> +       mutex_unlock(&bdi_dev_mutex);
>         kfree(name);
>         return ret;
>  }
>