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

["Joe Jin" <joe.jin@oracle.com>]

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;
 }