From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Joe Jin" Subject: Re: [PATCH] Fix blkback/blktap sysfs read bug. Date: Wed, 20 Jan 2010 19:45:18 +0800 Message-ID: <20100120114518.GA10851@joejin-pc.cn.oracle.com> References: <20100119095250.GB20691@joejin-pc.cn.oracle.com> <4B5596B2020000780002AAF1@vpn.id2.novell.com> <20100119113224.GA21348@joejin-pc.cn.oracle.com> <4B55AE58020000780002AB39@vpn.id2.novell.com> <20100119141338.GA22249@joejin-pc.cn.oracle.com> <4B55E9E7020000780002AC17@vpn.id2.novell.com> <20100120020605.GA25697@joejin-pc.cn.oracle.com> <4B56C2DB020000780002AE45@vpn.id2.novell.com> <20100120105136.GA6801@joejin-pc.cn.oracle.com> <4B56F1B6020000780002AEFA@vpn.id2.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <4B56F1B6020000780002AEFA@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 , greg.marsden@oracle.com List-Id: xen-devel@lists.xenproject.org On 2010-01-20 11:06, Jan Beulich wrote: > >>> "Joe Jin" 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; }