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

["Joe Jin" <joe.jin@xxxxxxxxxx>]

On 2010-01-19 16:20, Jan Beulich wrote:
> >>> "Joe Jin" <joe.jin@xxxxxxxxxx> 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;
 }
 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel