Re: [Xen-devel] [PATCH] blkfront: don't access freed struct xenbus_device

[Daniel Stodden <daniel.stodden@xxxxxxxxxx>]

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@xxxxxxxxxx>
> 
> ---
>  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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel