[PATCH] Fix blkback/blktap sysfs read bug.

[PATCH] Fix blkback/blktap sysfs read bug.

[Joe Jin]

Hi,

At backend driver blkback and blktap, when checking statistics information,
at the time vbd device remove, kernel will crash.

Below patch will fix it, please review and apply.

Signed-off-by: Joe Jin <joe.jin@oracle.com>

 blkback/xenbus.c |    5 ++++-
 blktap/xenbus.c  |    5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)
---

diff -r 6061d5615522 drivers/xen/blkback/xenbus.c
--- a/drivers/xen/blkback/xenbus.c	Fri Jan 08 13:07:17 2010 +0000
+++ b/drivers/xen/blkback/xenbus.c	Tue Jan 19 17:37:32 2010 +0800
@@ -104,10 +104,13 @@
 				   struct device_attribute *attr,	\
 				   char *buf)				\
 	{								\
+		ssize_t ret = -ENODEV;					\
 		struct xenbus_device *dev = to_xenbus_device(_dev);	\
 		struct backend_info *be = dev->dev.driver_data;		\
 									\
-		return sprintf(buf, format, ##args);			\
+		if (be && be->blkif)					\
+			ret = sprintf(buf, format, ##args);		\
+		return ret;						\
 	}								\
 	static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL)
 
diff -r 6061d5615522 drivers/xen/blktap/xenbus.c
--- a/drivers/xen/blktap/xenbus.c	Fri Jan 08 13:07:17 2010 +0000
+++ b/drivers/xen/blktap/xenbus.c	Tue Jan 19 17:37:32 2010 +0800
@@ -122,10 +122,13 @@
 				   struct device_attribute *attr,	\
 				   char *buf)				\
 	{								\
+		ssize_t ret = -ENODEV;					\
 		struct xenbus_device *dev = to_xenbus_device(_dev);	\
 		struct backend_info *be = dev->dev.driver_data;		\
 									\
-		return sprintf(buf, format, ##args);			\
+		if (be && be->blkif)					\
+			ret = sprintf(buf, format, ##args);		\
+		return ret;						\
 	}								\
 	static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL)

Re: [PATCH] Fix blkback/blktap sysfs read bug.

[Jan Beulich]

>>> "Joe Jin" <joe.jin@oracle.com> 19.01.10 10:52 >>>
>At backend driver blkback and blktap, when checking statistics information,
>at the time vbd device remove, kernel will crash.
>
>Below patch will fix it, please review and apply.

This isn't a complete fix if I follow your analysis: There's still a race
between blk{back,tap}_remove() freeing be->blkif/be and the sysfs
code. dev->dev.driver_data (and possibly be->blkif) must be cleared
before freeing it (them).

Jan

Re: [PATCH] Fix blkback/blktap sysfs read bug.

[Joe Jin]

On 2010-01-19 10:25, Jan Beulich wrote:
> >>> "Joe Jin" <joe.jin@oracle.com> 19.01.10 10:52 >>>
> >At backend driver blkback and blktap, when checking statistics information,
> >at the time vbd device remove, kernel will crash.
> >
> >Below patch will fix it, please review and apply.
> 
> This isn't a complete fix if I follow your analysis: There's still a race
> between blk{back,tap}_remove() freeing be->blkif/be and the sysfs
> code. dev->dev.driver_data (and possibly be->blkif) must be cleared
> before freeing it (them).
> 

Thanks for you point.
Anyway, I think need to add some check at VBD_SHOW even cleared 
dev->dev.driver_data before free it, sysfs->fops() have been 
initialized when call open(), and later when read the file, call
trace will fall VBD_SHOW defined function(s), so it should crashed.

The checks may look like below?


diff -r 6061d5615522 drivers/xen/blkback/xenbus.c
--- a/drivers/xen/blkback/xenbus.c	Fri Jan 08 13:07:17 2010 +0000
+++ b/drivers/xen/blkback/xenbus.c	Tue Jan 19 19:30:32 2010 +0800
@@ -104,10 +104,13 @@
 				   struct device_attribute *attr,	\
 				   char *buf)				\
 	{								\
+		ssize_t ret = -ENODEV;					\
 		struct xenbus_device *dev = to_xenbus_device(_dev);	\
-		struct backend_info *be = dev->dev.driver_data;		\
+		struct backend_info *be;				\
 									\
-		return sprintf(buf, format, ##args);			\
+		if (dev && (be = dev->dev.driver_data) && be->blkif)	\
+			ret = sprintf(buf, format, ##args);		\
+		return ret;						\
 	}								\
 	static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL)
 
diff -r 6061d5615522 drivers/xen/blktap/xenbus.c
--- a/drivers/xen/blktap/xenbus.c	Fri Jan 08 13:07:17 2010 +0000
+++ b/drivers/xen/blktap/xenbus.c	Tue Jan 19 19:30:32 2010 +0800
@@ -122,10 +122,13 @@
 				   struct device_attribute *attr,	\
 				   char *buf)				\
 	{								\
+		ssize_t ret = -ENODEV;					\
 		struct xenbus_device *dev = to_xenbus_device(_dev);	\
-		struct backend_info *be = dev->dev.driver_data;		\
+		struct backend_info *be;				\
 									\
-		return sprintf(buf, format, ##args);			\
+		if (dev && (be = dev->dev.driver_data) && be->blkif)	\
+			ret = sprintf(buf, format, ##args);		\
+		return ret;						\
 	}								\
 	static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL)

Re: [PATCH] Fix blkback/blktap sysfs read bug.

[Jan Beulich]

>>> "Joe Jin" <joe.jin@oracle.com> 19.01.10 12:32 >>>
>On 2010-01-19 10:25, Jan Beulich wrote:
>> >>> "Joe Jin" <joe.jin@oracle.com> 19.01.10 10:52 >>>
>> >At backend driver blkback and blktap, when checking statistics information,
>> >at the time vbd device remove, kernel will crash.
>> >
>> >Below patch will fix it, please review and apply.
>> 
>> This isn't a complete fix if I follow your analysis: There's still a race
>> between blk{back,tap}_remove() freeing be->blkif/be and the sysfs
>> code. dev->dev.driver_data (and possibly be->blkif) must be cleared
>> before freeing it (them).
>> 
>
>Thanks for you point.
>Anyway, I think need to add some check at VBD_SHOW even cleared 
>dev->dev.driver_data before free it, sysfs->fops() have been 
>initialized when call open(), and later when read the file, call
>trace will fall VBD_SHOW defined function(s), so it should crashed.

I think I understand what you're saying. But then there's only one
solution - avoid freeing those two data structures from the .remove
handler.

>The checks may look like below?

Checking dev to be non-NULL is certainly pointless in any case.

But wait - how did you see the crash you're trying to fix occur in the
first place? blkback_remove() calls xenvbd_sysfs_delif(), so the sysfs
entries are gone by the time driver_data gets freed.

Jan

Re: [PATCH] Fix blkback/blktap sysfs read bug.

[Joe Jin]

On 2010-01-19 12:06, Jan Beulich wrote:
> >>> "Joe Jin" <joe.jin@oracle.com> 19.01.10 12:32 >>>
> >On 2010-01-19 10:25, Jan Beulich wrote:
> >> >>> "Joe Jin" <joe.jin@oracle.com> 19.01.10 10:52 >>>
> >> >At backend driver blkback and blktap, when checking statistics information,
> >> >at the time vbd device remove, kernel will crash.
> >> >
> >> >Below patch will fix it, please review and apply.
> >> 
> >> This isn't a complete fix if I follow your analysis: There's still a race
> >> between blk{back,tap}_remove() freeing be->blkif/be and the sysfs
> >> code. dev->dev.driver_data (and possibly be->blkif) must be cleared
> >> before freeing it (them).
> >> 
> >
> >Thanks for you point.
> >Anyway, I think need to add some check at VBD_SHOW even cleared 
> >dev->dev.driver_data before free it, sysfs->fops() have been 
> >initialized when call open(), and later when read the file, call
> >trace will fall VBD_SHOW defined function(s), so it should crashed.
> 
> I think I understand what you're saying. But then there's only one
> solution - avoid freeing those two data structures from the .remove
> handler.

locked blk{back,tap}_remove and VBD_SHOW? I did not found any refcnt help 
for this. but even add lock for this, I think still need the checks.

> 
> >The checks may look like below?
> 
> Checking dev to be non-NULL is certainly pointless in any case.
> 
> But wait - how did you see the crash you're trying to fix occur in the
> first place? blkback_remove() calls xenvbd_sysfs_delif(), so the sysfs
> entries are gone by the time driver_data gets freed.

When tested continous reboot VM testcase kernel panic. then I write 
a program as blow:
while (1) {
	open("/sys/devices/xen-backend/vbd-x-xxxx/statistics/rd_sect");
	usleep(100);
	read(fd, buff, BUFF_LEN);
}

running it, then reboot the vm, will hit the panic.

As I have menthioned, when open the file, kernel have initialized 
file handler, when call file operation, call the function.
At here, even blkback_remove() have delete sysfs entry, but did not
released sysfs->fops, that caused the panic.


Thanks,
Joe

Re: [PATCH] Fix blkback/blktap sysfs read bug.

[Jan Beulich]

>>> "Joe Jin" <joe.jin@oracle.com> 19.01.10 15:13 >>>
>When tested continous reboot VM testcase kernel panic. then I write 
>a program as blow:
>while (1) {
>	open("/sys/devices/xen-backend/vbd-x-xxxx/statistics/rd_sect");
>	usleep(100);
>	read(fd, buff, BUFF_LEN);
>}
>
>running it, then reboot the vm, will hit the panic.
>
>As I have menthioned, when open the file, kernel have initialized 
>file handler, when call file operation, call the function.
>At here, even blkback_remove() have delete sysfs entry, but did not
>released sysfs->fops, that caused the panic.

That would seem like a general sysfs problem then - I can't imagine
other modules do much to prevent this, though I'm sure there are
other modules which add/remove entries on the fly. Would require
some looking through the sources to see whether there is any
generally usable (and simple) solution to this.

But irrespective of that, I think the race remains even after your
patch - only the window gets smaller.

Jan

Re: [PATCH] Fix blkback/blktap sysfs read bug.

[Joe Jin]

On 2010-01-19 16:20, Jan Beulich wrote:
> >>> "Joe Jin" <joe.jin@oracle.com> 19.01.10 15:13 >>>
> >When tested continous reboot VM testcase kernel panic. then I write 
> >a program as blow:
> >while (1) {
> >	open("/sys/devices/xen-backend/vbd-x-xxxx/statistics/rd_sect");
> >	usleep(100);
> >	read(fd, buff, BUFF_LEN);
> >}
> >
> >running it, then reboot the vm, will hit the panic.
> >
> >As I have menthioned, when open the file, kernel have initialized 
> >file handler, when call file operation, call the function.
> >At here, even blkback_remove() have delete sysfs entry, but did not
> >released sysfs->fops, that caused the panic.
> 
> That would seem like a general sysfs problem then - I can't imagine
> other modules do much to prevent this, though I'm sure there are
> other modules which add/remove entries on the fly. Would require
> some looking through the sources to see whether there is any
> generally usable (and simple) solution to this.

sysfs did not provide lock to handle this, not sure if developer
think it is not necessary or they'd like to caller to handled it.

> 
> But irrespective of that, I think the race remains even after your
> patch - only the window gets smaller.

Add a rwlock will fix the race like below patch?


diff -r 6061d5615522 drivers/xen/blkback/xenbus.c
--- a/drivers/xen/blkback/xenbus.c	Fri Jan 08 13:07:17 2010 +0000
+++ b/drivers/xen/blkback/xenbus.c	Wed Jan 20 10:00:53 2010 +0800
@@ -26,6 +26,8 @@
 #define DPRINTK(fmt, args...)				\
 	pr_debug("blkback/xenbus (%s:%d) " fmt ".\n",	\
 		 __FUNCTION__, __LINE__, ##args)
+
+static rwlock_t sysfs_read_lock = RW_LOCK_UNLOCKED;
 
 struct backend_info
 {
@@ -104,10 +106,15 @@
 				   struct device_attribute *attr,	\
 				   char *buf)				\
 	{								\
+		ssize_t ret = -ENODEV;					\
 		struct xenbus_device *dev = to_xenbus_device(_dev);	\
-		struct backend_info *be = dev->dev.driver_data;		\
+		struct backend_info *be;				\
 									\
-		return sprintf(buf, format, ##args);			\
+		read_lock(&sysfs_read_lock);				\
+		if ((be = dev->dev.driver_data) && be->blkif)		\
+			ret = sprintf(buf, format, ##args);		\
+		read_unlock(&sysfs_read_lock);				\
+		return ret;						\
 	}								\
 	static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL)
 
@@ -173,6 +180,7 @@
 
 	DPRINTK("");
 
+	write_lock(&sysfs_read_lock);
 	if (be->major || be->minor)
 		xenvbd_sysfs_delif(dev);
 
@@ -191,6 +199,7 @@
 
 	kfree(be);
 	dev->dev.driver_data = NULL;
+	write_unlock(&sysfs_read_lock);
 	return 0;
 }
 
diff -r 6061d5615522 drivers/xen/blktap/xenbus.c
--- a/drivers/xen/blktap/xenbus.c	Fri Jan 08 13:07:17 2010 +0000
+++ b/drivers/xen/blktap/xenbus.c	Wed Jan 20 10:00:53 2010 +0800
@@ -50,6 +50,7 @@
 	int group_added;
 };
 
+static rwlock_t sysfs_read_lock = RW_LOCK_UNLOCKED;
 
 static void connect(struct backend_info *);
 static int connect_ring(struct backend_info *);
@@ -122,10 +123,15 @@
 				   struct device_attribute *attr,	\
 				   char *buf)				\
 	{								\
+		ssize_t ret = -ENODEV;					\
 		struct xenbus_device *dev = to_xenbus_device(_dev);	\
-		struct backend_info *be = dev->dev.driver_data;		\
+		struct backend_info *be;				\
 									\
-		return sprintf(buf, format, ##args);			\
+		read_lock(&sysfs_read_lock);				\
+		if (dev && (be = dev->dev.driver_data) && be->blkif)	\
+			ret = sprintf(buf, format, ##args);		\
+		read_unlock(&sysfs_read_lock);				\
+		return ret;						\
 	}								\
 	static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL)
 
@@ -170,6 +176,7 @@
 {
 	struct backend_info *be = dev->dev.driver_data;
 
+	write_lock(&sysfs_read_lock);
 	if (be->group_added)
 		xentap_sysfs_delif(be->dev);
 	if (be->backend_watch.node) {
@@ -187,6 +194,7 @@
 	}
 	kfree(be);
 	dev->dev.driver_data = NULL;
+	write_unlock(&sysfs_read_lock);
 	return 0;
 }

Re: [PATCH] Fix blkback/blktap sysfs read bug.

[Jan Beulich]

>>> "Joe Jin" <joe.jin@oracle.com> 20.01.10 03:06 >>>
>sysfs did not provide lock to handle this, not sure if developer
>think it is not necessary or they'd like to caller to handled it.

A lock is probably not the usual way to deal with this; ref-counting
would seem more common. Nevertheless I think adding a lock will
take care of the issue here.

>--- a/drivers/xen/blktap/xenbus.c	Fri Jan 08 13:07:17 2010 +0000
>+++ b/drivers/xen/blktap/xenbus.c	Wed Jan 20 10:00:53 2010 +0800
>...
>@@ -122,10 +123,15 @@
> 				   struct device_attribute *attr,	\
> 				   char *buf)				\
> 	{								\
>+		ssize_t ret = -ENODEV;					\
> 		struct xenbus_device *dev = to_xenbus_device(_dev);	\

The use of to_xenbus_device() here makes ...

>-		struct backend_info *be = dev->dev.driver_data;		\
>+		struct backend_info *be;				\
> 									\
>-		return sprintf(buf, format, ##args);			\
>+		read_lock(&sysfs_read_lock);				\
>+		if (dev && (be = dev->dev.driver_data) && be->blkif)	\

... the checking of dev here useless (and the blkback part of the patch
doesn't do the same).

>+			ret = sprintf(buf, format, ##args);		\
>+		read_unlock(&sysfs_read_lock);				\
>+		return ret;						\
> 	}								\
> 	static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL)
> 

And btw., in both cases with the lock added there's no need to check
both 'be' and 'be->blkif', since be->blkif can't be NULL when be is
non-NULL.

Jan

Re: [PATCH] Fix blkback/blktap sysfs read bug.

[Joe Jin]

On 2010-01-20 07:46, Jan Beulich wrote:
> >>> "Joe Jin" <joe.jin@oracle.com> 20.01.10 03:06 >>>
> >sysfs did not provide lock to handle this, not sure if developer
> >think it is not necessary or they'd like to caller to handled it.
> 
> A lock is probably not the usual way to deal with this; ref-counting
> would seem more common. Nevertheless I think adding a lock will
> take care of the issue here.

Add refcnt could not fix the race for file open is sysfs file operations
and will not touch xenbus/blk{back,tap} struct, so could not increase 
the refcnt.
The root cause is after open sysfs file, vbd was been removed by other.
For the lock type is read/write lock, so the lock almost will not slowdwon 
performance, isn't it?

> 
> >--- a/drivers/xen/blktap/xenbus.c	Fri Jan 08 13:07:17 2010 +0000
> >+++ b/drivers/xen/blktap/xenbus.c	Wed Jan 20 10:00:53 2010 +0800
> >...
> >@@ -122,10 +123,15 @@
> > 				   struct device_attribute *attr,	\
> > 				   char *buf)				\
> > 	{								\
> >+		ssize_t ret = -ENODEV;					\
> > 		struct xenbus_device *dev = to_xenbus_device(_dev);	\
> 
> The use of to_xenbus_device() here makes ...
> 
> >-		struct backend_info *be = dev->dev.driver_data;		\
> >+		struct backend_info *be;				\
> > 									\
> >-		return sprintf(buf, format, ##args);			\
> >+		read_lock(&sysfs_read_lock);				\
> >+		if (dev && (be = dev->dev.driver_data) && be->blkif)	\
> 
> ... the checking of dev here useless (and the blkback part of the patch
> doesn't do the same).

Sorry I forgot get rid of it :)

> 
> >+			ret = sprintf(buf, format, ##args);		\
> >+		read_unlock(&sysfs_read_lock);				\
> >+		return ret;						\
> > 	}								\
> > 	static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL)
> > 
> 
> And btw., in both cases with the lock added there's no need to check
> both 'be' and 'be->blkif', since be->blkif can't be NULL when be is
> non-NULL.

No we must check it, as I have methioned, root cause is vbd was been
freed after open sysfs file.


Below is the new patch, please review.


diff -r 0bec29c94ce9 drivers/xen/blkback/xenbus.c
--- a/drivers/xen/blkback/xenbus.c	Mon Jan 18 14:50:43 2010 +0000
+++ b/drivers/xen/blkback/xenbus.c	Wed Jan 20 18:49:50 2010 +0800
@@ -26,6 +26,8 @@
 #define DPRINTK(fmt, args...)				\
 	pr_debug("blkback/xenbus (%s:%d) " fmt ".\n",	\
 		 __FUNCTION__, __LINE__, ##args)
+
+static rwlock_t sysfs_read_lock = RW_LOCK_UNLOCKED;
 
 struct backend_info
 {
@@ -104,10 +106,19 @@
 				   struct device_attribute *attr,	\
 				   char *buf)				\
 	{								\
-		struct xenbus_device *dev = to_xenbus_device(_dev);	\
-		struct backend_info *be = dev->dev.driver_data;		\
+		ssize_t ret = -ENODEV;					\
+		struct xenbus_device *dev;				\
+		struct backend_info *be;				\
 									\
-		return sprintf(buf, format, ##args);			\
+		if (!get_device(_dev))					\
+			return ret;					\
+		dev = to_xenbus_device(_dev);				\
+		read_lock(&sysfs_read_lock);				\
+		if ((be = dev->dev.driver_data) && be->blkif)		\
+			ret = sprintf(buf, format, ##args);		\
+		read_unlock(&sysfs_read_lock);				\
+		put_device(_dev);					\
+		return ret;						\
 	}								\
 	static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL)
 
@@ -173,6 +184,7 @@
 
 	DPRINTK("");
 
+	write_lock(&sysfs_read_lock);
 	if (be->major || be->minor)
 		xenvbd_sysfs_delif(dev);
 
@@ -191,6 +203,7 @@
 
 	kfree(be);
 	dev->dev.driver_data = NULL;
+	write_unlock(&sysfs_read_lock);
 	return 0;
 }
 
diff -r 0bec29c94ce9 drivers/xen/blktap/xenbus.c
--- a/drivers/xen/blktap/xenbus.c	Mon Jan 18 14:50:43 2010 +0000
+++ b/drivers/xen/blktap/xenbus.c	Wed Jan 20 18:49:50 2010 +0800
@@ -50,6 +50,7 @@
 	int group_added;
 };
 
+static rwlock_t sysfs_read_lock = RW_LOCK_UNLOCKED;
 
 static void connect(struct backend_info *);
 static int connect_ring(struct backend_info *);
@@ -122,10 +123,19 @@
 				   struct device_attribute *attr,	\
 				   char *buf)				\
 	{								\
-		struct xenbus_device *dev = to_xenbus_device(_dev);	\
-		struct backend_info *be = dev->dev.driver_data;		\
+		ssize_t ret = -ENODEV;					\
+		struct xenbus_device *dev;				\
+		struct backend_info *be;				\
 									\
-		return sprintf(buf, format, ##args);			\
+		if (!get_device(_dev))					\
+			return ret;					\
+		dev = to_xenbus_device(_dev);				\
+		read_lock(&sysfs_read_lock);				\
+		if ((be = dev->dev.driver_data) && be->blkif)		\
+			ret = sprintf(buf, format, ##args);		\
+		read_unlock(&sysfs_read_lock);				\
+		put_device(_dev);					\
+		return ret;						\
 	}								\
 	static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL)
 
@@ -170,6 +180,7 @@
 {
 	struct backend_info *be = dev->dev.driver_data;
 
+	write_lock(&sysfs_read_lock);
 	if (be->group_added)
 		xentap_sysfs_delif(be->dev);
 	if (be->backend_watch.node) {
@@ -187,6 +198,7 @@
 	}
 	kfree(be);
 	dev->dev.driver_data = NULL;
+	write_unlock(&sysfs_read_lock);
 	return 0;
 }

Re: [PATCH] Fix blkback/blktap sysfs read bug.

[Jan Beulich]

>>> "Joe Jin" <joe.jin@oracle.com> 20.01.10 11:51 >>>
>On 2010-01-20 07:46, Jan Beulich wrote:
>> And btw., in both cases with the lock added there's no need to check
>> both 'be' and 'be->blkif', since be->blkif can't be NULL when be is
>> non-NULL.
>
>No we must check it, as I have methioned, root cause is vbd was been
>freed after open sysfs file.

Why? You're inside the locked region, so either 'be' is NULL, or both
'be' and 'be->blkif' are non-NULL.

Jan

Re: [PATCH] Fix blkback/blktap sysfs read bug.

[Joe Jin]

On 2010-01-20 11:06, Jan Beulich wrote:
> >>> "Joe Jin" <joe.jin@oracle.com> 20.01.10 11:51 >>>
> >On 2010-01-20 07:46, Jan Beulich wrote:
> >> And btw., in both cases with the lock added there's no need to check
> >> both 'be' and 'be->blkif', since be->blkif can't be NULL when be is
> >> non-NULL.
> >
> >No we must check it, as I have methioned, root cause is vbd was been
> >freed after open sysfs file.
> 
> Why? You're inside the locked region, so either 'be' is NULL, or both
> 'be' and 'be->blkif' are non-NULL.
> 

Sorry for misunstdrstood, your are right, attach new patch.


diff -r 0bec29c94ce9 drivers/xen/blkback/xenbus.c
--- a/drivers/xen/blkback/xenbus.c	Mon Jan 18 14:50:43 2010 +0000
+++ b/drivers/xen/blkback/xenbus.c	Wed Jan 20 19:44:32 2010 +0800
@@ -26,6 +26,8 @@
 #define DPRINTK(fmt, args...)				\
 	pr_debug("blkback/xenbus (%s:%d) " fmt ".\n",	\
 		 __FUNCTION__, __LINE__, ##args)
+
+static rwlock_t sysfs_read_lock = RW_LOCK_UNLOCKED;
 
 struct backend_info
 {
@@ -104,10 +106,19 @@
 				   struct device_attribute *attr,	\
 				   char *buf)				\
 	{								\
-		struct xenbus_device *dev = to_xenbus_device(_dev);	\
-		struct backend_info *be = dev->dev.driver_data;		\
+		ssize_t ret = -ENODEV;					\
+		struct xenbus_device *dev;				\
+		struct backend_info *be;				\
 									\
-		return sprintf(buf, format, ##args);			\
+		if (!get_device(_dev))					\
+			return ret;					\
+		dev = to_xenbus_device(_dev);				\
+		read_lock(&sysfs_read_lock);				\
+		if ((be = dev->dev.driver_data) != NULL)		\
+			ret = sprintf(buf, format, ##args);		\
+		read_unlock(&sysfs_read_lock);				\
+		put_device(_dev);					\
+		return ret;						\
 	}								\
 	static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL)
 
@@ -173,6 +184,7 @@
 
 	DPRINTK("");
 
+	write_lock(&sysfs_read_lock);
 	if (be->major || be->minor)
 		xenvbd_sysfs_delif(dev);
 
@@ -191,6 +203,7 @@
 
 	kfree(be);
 	dev->dev.driver_data = NULL;
+	write_unlock(&sysfs_read_lock);
 	return 0;
 }
 
diff -r 0bec29c94ce9 drivers/xen/blktap/xenbus.c
--- a/drivers/xen/blktap/xenbus.c	Mon Jan 18 14:50:43 2010 +0000
+++ b/drivers/xen/blktap/xenbus.c	Wed Jan 20 19:44:32 2010 +0800
@@ -50,6 +50,7 @@
 	int group_added;
 };
 
+static rwlock_t sysfs_read_lock = RW_LOCK_UNLOCKED;
 
 static void connect(struct backend_info *);
 static int connect_ring(struct backend_info *);
@@ -122,10 +123,19 @@
 				   struct device_attribute *attr,	\
 				   char *buf)				\
 	{								\
-		struct xenbus_device *dev = to_xenbus_device(_dev);	\
-		struct backend_info *be = dev->dev.driver_data;		\
+		ssize_t ret = -ENODEV;					\
+		struct xenbus_device *dev;				\
+		struct backend_info *be;				\
 									\
-		return sprintf(buf, format, ##args);			\
+		if (!get_device(_dev))					\
+			return ret;					\
+		dev = to_xenbus_device(_dev);				\
+		read_lock(&sysfs_read_lock);				\
+		if ((be = dev->dev.driver_data) != NULL)		\
+			ret = sprintf(buf, format, ##args);		\
+		read_unlock(&sysfs_read_lock);				\
+		put_device(_dev);					\
+		return ret;						\
 	}								\
 	static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL)
 
@@ -170,6 +180,7 @@
 {
 	struct backend_info *be = dev->dev.driver_data;
 
+	write_lock(&sysfs_read_lock);
 	if (be->group_added)
 		xentap_sysfs_delif(be->dev);
 	if (be->backend_watch.node) {
@@ -187,6 +198,7 @@
 	}
 	kfree(be);
 	dev->dev.driver_data = NULL;
+	write_unlock(&sysfs_read_lock);
 	return 0;
 }

Re: [PATCH] Fix blkback/blktap sysfs read bug.

[Keir Fraser]

On 20/01/2010 11:45, "Joe Jin" <joe.jin@oracle.com> wrote:

>> Why? You're inside the locked region, so either 'be' is NULL, or both
>> 'be' and 'be->blkif' are non-NULL.
>> 
> 
> Sorry for misunstdrstood, your are right, attach new patch.

I'm waiting for final resubmission with changeset comment and sign-off, and
for it to collect an ack from Jan, by the way.

 -- Keir

Re: [PATCH] Fix blkback/blktap sysfs read bug.

[Daniel Stodden]

Hey.

I think reverted a similar patch when moving past 2.6.18 a while ago,
but I don't remember the first kernel versions affected right now.

On Wed, 2010-01-20 at 06:45 -0500, Joe Jin wrote:

>  static void connect(struct backend_info *);
>  static int connect_ring(struct backend_info *);
> @@ -122,10 +123,19 @@
>  				   struct device_attribute *attr,	\
>  				   char *buf)				\
>  	{								\
> -		struct xenbus_device *dev = to_xenbus_device(_dev);	\
> -		struct backend_info *be = dev->dev.driver_data;		\
> +		ssize_t ret = -ENODEV;					\
> +		struct xenbus_device *dev;				\
> +		struct backend_info *be;				\


								\
> -		return sprintf(buf, format, ##args);			\
> +		if (!get_device(_dev))					\
> +			return ret;					\
> +		dev = to_xenbus_device(_dev);				\

I think this deadlocks here:
> +		read_lock(&sysfs_read_lock);				\
> +		if ((be = dev->dev.driver_data) != NULL)		\
> +			ret = sprintf(buf, format, ##args);		\
> +		read_unlock(&sysfs_read_lock);				\
> +		put_device(_dev);					\
> +		return ret;						\
>  	}								\
>  	static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL)
>  
> @@ -170,6 +180,7 @@
>  {
>  	struct backend_info *be = dev->dev.driver_data;
>  
> +	write_lock(&sysfs_read_lock);
>  	if (be->group_added)

.. and here:
>  		xentap_sysfs_delif(be->dev);
>  	if (be->backend_watch.node) {
> @@ -187,6 +198,7 @@
>  	}
>  	kfree(be);
>  	dev->dev.driver_data = NULL;
> +	write_unlock(&sysfs_read_lock);
>  	return 0;
>  }

Sysfs may refcount these nodes, blocking xentap_sysfs_delif().


Daniel

Re: [PATCH] Fix blkback/blktap sysfs read bug.

[Joe Jin]

On 2010-01-20 18:16, Daniel Stodden wrote:
> 
> Hey.
> 
> I think reverted a similar patch when moving past 2.6.18 a while ago,
> but I don't remember the first kernel versions affected right now.
> 
> On Wed, 2010-01-20 at 06:45 -0500, Joe Jin wrote:
> 
> >  static void connect(struct backend_info *);
> >  static int connect_ring(struct backend_info *);
> > @@ -122,10 +123,19 @@
> >  				   struct device_attribute *attr,	\
> >  				   char *buf)				\
> >  	{								\
> > -		struct xenbus_device *dev = to_xenbus_device(_dev);	\
> > -		struct backend_info *be = dev->dev.driver_data;		\
> > +		ssize_t ret = -ENODEV;					\
> > +		struct xenbus_device *dev;				\
> > +		struct backend_info *be;				\
> 
> 
> 								\
> > -		return sprintf(buf, format, ##args);			\
> > +		if (!get_device(_dev))					\
> > +			return ret;					\
> > +		dev = to_xenbus_device(_dev);				\
> 
> I think this deadlocks here:
> > +		read_lock(&sysfs_read_lock);				\
> > +		if ((be = dev->dev.driver_data) != NULL)		\
> > +			ret = sprintf(buf, format, ##args);		\
> > +		read_unlock(&sysfs_read_lock);				\
> > +		put_device(_dev);					\
> > +		return ret;						\
> >  	}								\
> >  	static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL)
> >  
> > @@ -170,6 +180,7 @@
> >  {
> >  	struct backend_info *be = dev->dev.driver_data;
> >  
> > +	write_lock(&sysfs_read_lock);
> >  	if (be->group_added)
> 
> .. and here:
> >  		xentap_sysfs_delif(be->dev);
> >  	if (be->backend_watch.node) {
> > @@ -187,6 +198,7 @@
> >  	}
> >  	kfree(be);
> >  	dev->dev.driver_data = NULL;
> > +	write_unlock(&sysfs_read_lock);
> >  	return 0;
> >  }
> 
> Sysfs may refcount these nodes, blocking xentap_sysfs_delif().

Would you like to give me some info about of blocking and deadlocks?

Checked xentap_sysfs_delif() -> sysfs_remove_group() and I did not found
blocking codes, maybe I lost something?

Thanks,
Joe

Re: [PATCH] Fix blkback/blktap sysfs read bug.

[Daniel Stodden]

On Wed, 2010-01-20 at 22:13 -0500, Joe Jin wrote:
> On 2010-01-20 18:16, Daniel Stodden wrote:
> > 
> > Hey.
> > 
> > I think reverted a similar patch when moving past 2.6.18 a while ago,
> > but I don't remember the first kernel versions affected right now.
> > 

> > Sysfs may refcount these nodes, blocking xentap_sysfs_delif().
> 
> Would you like to give me some info about of blocking and deadlocks?
> 
> Checked xentap_sysfs_delif() -> sysfs_remove_group() and I did not found
> blocking codes, maybe I lost something?
> 
> Thanks,
> Joe

Hi Joe.

Your patch will work okay on 2.6.18.

But collisions will deadlock after 2.6.23

Found an old stack trace:

[2009-07-08 06:15:08 UTC] INFO: task xb.00021.xvdd:30039 blocked for more than 120 seconds.
[2009-07-08 06:15:08 UTC] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[2009-07-08 06:15:08 UTC]        c7adfe0c 00000246 00000000 00000000 5b88fd4f 00000256 c7ade000 c7adfdc8 
[2009-07-08 06:15:08 UTC]        c0107c1b 38c984a4 00000256 c7adfddc ed629578 ed6293f0 ed629578 c16bdb00 
[2009-07-08 06:15:08 UTC]        00000000 eea0d500 c16bdb34 002dc05a 00000000 00000005 0024c31c e20a8ff0 
[2009-07-08 06:15:08 UTC] Call Trace:
[2009-07-08 06:15:08 UTC]  [<c0107c1b>] ? local_clock+0x3b/0x90
[2009-07-08 06:15:08 UTC]  [<c0344675>] schedule_timeout+0x75/0xc0
[2009-07-08 06:15:08 UTC]  [<c011ed81>] ? pick_next_task_fair+0x91/0xd0
[2009-07-08 06:15:08 UTC]  [<c03442c9>] wait_for_common+0xa9/0x1c0
[2009-07-08 06:15:08 UTC]  [<c0120d40>] ? default_wake_function+0x0/0x10
[2009-07-08 06:15:08 UTC]  [<c0344472>] wait_for_completion+0x12/0x20
[2009-07-08 06:15:08 UTC]  [<c01cf9e7>] sysfs_addrm_finish+0x1e7/0x230
[2009-07-08 06:15:08 UTC]  [<c01ce2e5>] sysfs_hash_and_remove+0x45/0x70
[2009-07-08 06:15:08 UTC]  [<c01d0adb>] remove_files+0x1b/0x30
[2009-07-08 06:15:08 UTC]  [<c01d0b26>] sysfs_remove_group+0x36/0xc0
[2009-07-08 06:15:08 UTC]  [<c01ae02f>] ? __blkdev_put+0x14f/0x160
[2009-07-08 06:15:08 UTC]  [<c02769fc>] xenvbd_sysfs_delif+0x2c/0x60
[2009-07-08 06:15:08 UTC]  [<c0276a76>] blkback_close+0x46/0x70
[2009-07-08 06:15:08 UTC]  [<c0275d33>] blkif_schedule+0x583/0x5b0
[2009-07-08 06:15:08 UTC]  [<c011ed81>] ? pick_next_task_fair+0x91/0xd0
[2009-07-08 06:15:08 UTC]  [<c013dda0>] ? autoremove_wake_function+0x0/0x50
[2009-07-08 06:15:08 UTC]  [<c02757b0>] ? blkif_schedule+0x0/0x5b0
[2009-07-08 06:15:08 UTC]  [<c013da42>] kthread+0x42/0x70
[2009-07-08 06:15:08 UTC]  [<c013da00>] ? kthread+0x0/0x70
[2009-07-08 06:15:08 UTC]  [<c010561b>] kernel_thread_helper+0x7/0x10

The reason is in sysfs_deactivate(), which will sync callers against any
remaining thread in .show()
- show() hangs on the lock
- the lock holder in sysfs_remove_group(), 
  waiting for show() to complete.

Pardon me -- I'm not entirely sure where/how these patches are currently
submitted and merged. I suppose yours are only for linux-2.6.18.hg, not
e.g. pvops? Then sorry for any confusion.

Daniel

Re: [PATCH] Fix blkback/blktap sysfs read bug.

[Joe Jin]

> Your patch will work okay on 2.6.18.
> 
> But collisions will deadlock after 2.6.23
> 
> Found an old stack trace:
> 
> [2009-07-08 06:15:08 UTC] INFO: task xb.00021.xvdd:30039 blocked for more than 120 seconds.
> [2009-07-08 06:15:08 UTC] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> [2009-07-08 06:15:08 UTC]        c7adfe0c 00000246 00000000 00000000 5b88fd4f 00000256 c7ade000 c7adfdc8 
> [2009-07-08 06:15:08 UTC]        c0107c1b 38c984a4 00000256 c7adfddc ed629578 ed6293f0 ed629578 c16bdb00 
> [2009-07-08 06:15:08 UTC]        00000000 eea0d500 c16bdb34 002dc05a 00000000 00000005 0024c31c e20a8ff0 
> [2009-07-08 06:15:08 UTC] Call Trace:
> [2009-07-08 06:15:08 UTC]  [<c0107c1b>] ? local_clock+0x3b/0x90
> [2009-07-08 06:15:08 UTC]  [<c0344675>] schedule_timeout+0x75/0xc0
> [2009-07-08 06:15:08 UTC]  [<c011ed81>] ? pick_next_task_fair+0x91/0xd0
> [2009-07-08 06:15:08 UTC]  [<c03442c9>] wait_for_common+0xa9/0x1c0
> [2009-07-08 06:15:08 UTC]  [<c0120d40>] ? default_wake_function+0x0/0x10
> [2009-07-08 06:15:08 UTC]  [<c0344472>] wait_for_completion+0x12/0x20
> [2009-07-08 06:15:08 UTC]  [<c01cf9e7>] sysfs_addrm_finish+0x1e7/0x230
> [2009-07-08 06:15:08 UTC]  [<c01ce2e5>] sysfs_hash_and_remove+0x45/0x70
> [2009-07-08 06:15:08 UTC]  [<c01d0adb>] remove_files+0x1b/0x30
> [2009-07-08 06:15:08 UTC]  [<c01d0b26>] sysfs_remove_group+0x36/0xc0
> [2009-07-08 06:15:08 UTC]  [<c01ae02f>] ? __blkdev_put+0x14f/0x160
> [2009-07-08 06:15:08 UTC]  [<c02769fc>] xenvbd_sysfs_delif+0x2c/0x60
> [2009-07-08 06:15:08 UTC]  [<c0276a76>] blkback_close+0x46/0x70
> [2009-07-08 06:15:08 UTC]  [<c0275d33>] blkif_schedule+0x583/0x5b0
> [2009-07-08 06:15:08 UTC]  [<c011ed81>] ? pick_next_task_fair+0x91/0xd0
> [2009-07-08 06:15:08 UTC]  [<c013dda0>] ? autoremove_wake_function+0x0/0x50
> [2009-07-08 06:15:08 UTC]  [<c02757b0>] ? blkif_schedule+0x0/0x5b0
> [2009-07-08 06:15:08 UTC]  [<c013da42>] kthread+0x42/0x70
> [2009-07-08 06:15:08 UTC]  [<c013da00>] ? kthread+0x0/0x70
> [2009-07-08 06:15:08 UTC]  [<c010561b>] kernel_thread_helper+0x7/0x10
> 
> The reason is in sysfs_deactivate(), which will sync callers against any
> remaining thread in .show()
> - show() hangs on the lock
> - the lock holder in sysfs_remove_group(), 
>   waiting for show() to complete.
> 
> Pardon me -- I'm not entirely sure where/how these patches are currently
> submitted and merged. I suppose yours are only for linux-2.6.18.hg, not
> e.g. pvops? Then sorry for any confusion.
> 

Daniel, 

Thanks a lot of your comments, it really help for me, yes my patch based 
linux-2.6.18.hg branch. As Jan have pointed out in previous email, it should 
be sysfs's issue, looked like later kernel sysfs have fixed the issue?

Thanks,
Joe

Re: [PATCH] Fix blkback/blktap sysfs read bug.

[Daniel Stodden]

On Thu, 2010-01-21 at 02:49 -0500, Joe Jin wrote:

> > The reason is in sysfs_deactivate(), which will sync callers against any
> > remaining thread in .show()
> > - show() hangs on the lock
> > - the lock holder in sysfs_remove_group(), 
> >   waiting for show() to complete.
> > 
> > Pardon me -- I'm not entirely sure where/how these patches are currently
> > submitted and merged. I suppose yours are only for linux-2.6.18.hg, not
> > e.g. pvops? Then sorry for any confusion.
> > 
> 
> Daniel, 
> 
> Thanks a lot of your comments, it really help for me, yes my patch based 
> linux-2.6.18.hg branch. As Jan have pointed out in previous email, it should 
> be sysfs's issue, looked like later kernel sysfs have fixed the issue?

Yes. Later sysfs won't need the locking. Ordering sysfs removal before
backend release will be sufficient.

Daniel