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

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

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)