Jan Beulich
2010-Mar-01 10:00 UTC
[Xen-devel] [PATCH] blkfront: don''t access freed struct xenbus_device
Unfortunately commit eebc80638fe365a0386ed5ee57c7f0c6d5e56fb1 still
wasn''t quite right - there was a reference to freed memory left from
blkfront_closing().
Signed-off-by: Jan Beulich <jbeulich@novell.com>
---
drivers/block/xen-blkfront.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -981,13 +981,11 @@ static void blkfront_connect(struct blkf
* the backend. Once is this done, we can switch to Closed in
* acknowledgement.
*/
-static void blkfront_closing(struct xenbus_device *dev)
+static void blkfront_closing(struct blkfront_info *info)
{
- struct blkfront_info *info = dev_get_drvdata(&dev->dev);
unsigned int minor, nr_minors;
unsigned long flags;
- dev_dbg(&dev->dev, "blkfront_closing: %s removed\n",
dev->nodename);
if (info->rq == NULL)
goto out;
@@ -1013,7 +1011,8 @@ static void blkfront_closing(struct xenb
xlbd_release_minors(minor, nr_minors);
out:
- xenbus_frontend_closed(dev);
+ if (info->xbdev)
+ xenbus_frontend_closed(info->xbdev);
}
/**
@@ -1053,7 +1052,7 @@ static void backend_changed(struct xenbu
xenbus_dev_error(dev, -EBUSY,
"Device in use; refusing to close");
else
- blkfront_closing(dev);
+ blkfront_closing(info);
mutex_unlock(&bd->bd_mutex);
bdput(bd);
break;
@@ -1071,7 +1070,7 @@ static int blkfront_remove(struct xenbus
if(info->users == 0)
kfree(info);
else
- info->is_ready = -1;
+ info->xbdev = NULL;
return 0;
}
@@ -1080,14 +1079,14 @@ static int blkfront_is_ready(struct xenb
{
struct blkfront_info *info = dev_get_drvdata(&dev->dev);
- return info->is_ready > 0;
+ return info->is_ready && info->xbdev;
}
static int blkif_open(struct block_device *bdev, fmode_t mode)
{
struct blkfront_info *info = bdev->bd_disk->private_data;
- if(info->is_ready < 0)
+ if (!info->xbdev)
return -ENODEV;
info->users++;
return 0;
@@ -1102,13 +1101,13 @@ static int blkif_release(struct gendisk
have ignored this request initially, as the device was
still mounted. */
struct xenbus_device *dev = info->xbdev;
- enum xenbus_state state = xenbus_read_driver_state(dev->otherend);
- if(info->is_ready < 0) {
- blkfront_closing(dev);
+ if (!dev) {
+ blkfront_closing(info);
kfree(info);
- } else if (state == XenbusStateClosing && info->is_ready)
- blkfront_closing(dev);
+ } else if (xenbus_read_driver_state(dev->otherend)
+ == XenbusStateClosing && info->is_ready)
+ blkfront_closing(info);
}
return 0;
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Daniel Stodden
2010-Mar-02 03:37 UTC
Re: [Xen-devel] [PATCH] blkfront: don''t access freed struct xenbus_device
Hi Jan. I think clearing xbdev went exactly into the right direction, that''s much more intuitive to follow than the is_ready encoding. It does nothing preventing the race though. This code will e.g. free info under the feet of blkif_release, when it hits the CPU right after the info->users--. Another simple crasher is a blkif_open() just after blkfront_remove found users == 0. But I think there is going to a decent fix: We will just run the full XenbusStateClosing transition in blkfront_remove. Means: remove the disk node while holding the bd_mutex, if users == 0. This doesn''t take additional locks and is the right thing to do anyway: Don''t fail blkif_open based on xbdev, but prevent any blkif_open at all. Killing the device at this point also makes the desired queue flush happen. We also need to sync against ongoing queue runs before it''s safe to shut the interface. That''s all going to happen then. Also * xenbus_frontend_closed wants a transaction * blkif_ioctl is presently heading into disaster * etc. etc. Daniel On Mon, 2010-03-01 at 05:00 -0500, Jan Beulich wrote:> Unfortunately commit eebc80638fe365a0386ed5ee57c7f0c6d5e56fb1 still > wasn''t quite right - there was a reference to freed memory left from > blkfront_closing(). > > Signed-off-by: Jan Beulich <jbeulich@novell.com> > > --- > drivers/block/xen-blkfront.c | 25 ++++++++++++------------- > 1 file changed, 12 insertions(+), 13 deletions(-) > > --- a/drivers/block/xen-blkfront.c > +++ b/drivers/block/xen-blkfront.c > @@ -981,13 +981,11 @@ static void blkfront_connect(struct blkf > * the backend. Once is this done, we can switch to Closed in > * acknowledgement. > */ > -static void blkfront_closing(struct xenbus_device *dev) > +static void blkfront_closing(struct blkfront_info *info) > { > - struct blkfront_info *info = dev_get_drvdata(&dev->dev); > unsigned int minor, nr_minors; > unsigned long flags; > > - dev_dbg(&dev->dev, "blkfront_closing: %s removed\n", dev->nodename); > > if (info->rq == NULL) > goto out; > @@ -1013,7 +1011,8 @@ static void blkfront_closing(struct xenb > xlbd_release_minors(minor, nr_minors); > > out: > - xenbus_frontend_closed(dev); > + if (info->xbdev) > + xenbus_frontend_closed(info->xbdev); > } > > /** > @@ -1053,7 +1052,7 @@ static void backend_changed(struct xenbu > xenbus_dev_error(dev, -EBUSY, > "Device in use; refusing to close"); > else > - blkfront_closing(dev); > + blkfront_closing(info); > mutex_unlock(&bd->bd_mutex); > bdput(bd); > break; > @@ -1071,7 +1070,7 @@ static int blkfront_remove(struct xenbus > if(info->users == 0) > kfree(info); > else > - info->is_ready = -1; > + info->xbdev = NULL; > > return 0; > } > @@ -1080,14 +1079,14 @@ static int blkfront_is_ready(struct xenb > { > struct blkfront_info *info = dev_get_drvdata(&dev->dev); > > - return info->is_ready > 0; > + return info->is_ready && info->xbdev; > } > > static int blkif_open(struct block_device *bdev, fmode_t mode) > { > struct blkfront_info *info = bdev->bd_disk->private_data; > > - if(info->is_ready < 0) > + if (!info->xbdev) > return -ENODEV; > info->users++; > return 0; > @@ -1102,13 +1101,13 @@ static int blkif_release(struct gendisk > have ignored this request initially, as the device was > still mounted. */ > struct xenbus_device *dev = info->xbdev; > - enum xenbus_state state = xenbus_read_driver_state(dev->otherend); > > - if(info->is_ready < 0) { > - blkfront_closing(dev); > + if (!dev) { > + blkfront_closing(info); > kfree(info); > - } else if (state == XenbusStateClosing && info->is_ready) > - blkfront_closing(dev); > + } else if (xenbus_read_driver_state(dev->otherend) > + == XenbusStateClosing && info->is_ready) > + blkfront_closing(info); > } > return 0; > } > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel