From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Joe Jin" Subject: Re: [PATCH] Fix blkback/blktap sysfs read bug. Date: Tue, 19 Jan 2010 19:32:24 +0800 Message-ID: <20100119113224.GA21348@joejin-pc.cn.oracle.com> References: <20100119095250.GB20691@joejin-pc.cn.oracle.com> <4B5596B2020000780002AAF1@vpn.id2.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <4B5596B2020000780002AAF1@vpn.id2.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jan Beulich Cc: deepak.patel@oracle.com, Keir Fraser , xen-devel@lists.xensource.com, joe.jin@oracle.com, greg.marsden@oracle.com List-Id: xen-devel@lists.xenproject.org On 2010-01-19 10:25, Jan Beulich wrote: > >>> "Joe Jin" 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)