Re: [PATCH] xen-blkfront: sanitize the removal state machine

2021-07-15 Thread Jens Axboe
On 7/15/21 8:17 AM, Christoph Hellwig wrote:
> xen-blkfront has a weird protocol where close message from the remote
> side can be delayed, and where hot removals are treated somewhat
> differently from regular removals, all leading to potential NULL
> pointer removals, and a del_gendisk from the block device release
> method, which will deadlock. Fix this by just performing normal hot
> removals even when the device is opened like all other Linux block
> drivers.

Applied, thanks.

-- 
Jens Axboe




[PATCH] xen-blkfront: sanitize the removal state machine

2021-07-15 Thread Christoph Hellwig
xen-blkfront has a weird protocol where close message from the remote
side can be delayed, and where hot removals are treated somewhat
differently from regular removals, all leading to potential NULL
pointer removals, and a del_gendisk from the block device release
method, which will deadlock. Fix this by just performing normal hot
removals even when the device is opened like all other Linux block
drivers.

Fixes: c76f48eb5c08 ("block: take bd_mutex around delete_partitions in 
del_gendisk")
Reported-by: Vitaly Kuznetsov 
Signed-off-by: Christoph Hellwig 
Tested-by: Vitaly Kuznetsov 
---
 drivers/block/xen-blkfront.c | 224 ---
 1 file changed, 26 insertions(+), 198 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 8d49f8fa98bb..d83fee21f6c5 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -502,34 +502,21 @@ static int blkif_getgeo(struct block_device *bd, struct 
hd_geometry *hg)
 static int blkif_ioctl(struct block_device *bdev, fmode_t mode,
   unsigned command, unsigned long argument)
 {
-   struct blkfront_info *info = bdev->bd_disk->private_data;
int i;
 
-   dev_dbg(>xbdev->dev, "command: 0x%x, argument: 0x%lx\n",
-   command, (long)argument);
-
switch (command) {
case CDROMMULTISESSION:
-   dev_dbg(>xbdev->dev, "FIXME: support multisession CDs 
later\n");
for (i = 0; i < sizeof(struct cdrom_multisession); i++)
if (put_user(0, (char __user *)(argument + i)))
return -EFAULT;
return 0;
-
-   case CDROM_GET_CAPABILITY: {
-   struct gendisk *gd = info->gd;
-   if (gd->flags & GENHD_FL_CD)
+   case CDROM_GET_CAPABILITY:
+   if (bdev->bd_disk->flags & GENHD_FL_CD)
return 0;
return -EINVAL;
-   }
-
default:
-   /*printk(KERN_ALERT "ioctl %08x not supported by Xen blkdev\n",
- command);*/
-   return -EINVAL; /* same return as native Linux */
+   return -EINVAL;
}
-
-   return 0;
 }
 
 static unsigned long blkif_ring_get_request(struct blkfront_ring_info *rinfo,
@@ -1177,36 +1164,6 @@ static int xlvbd_alloc_gendisk(blkif_sector_t capacity,
return err;
 }
 
-static void xlvbd_release_gendisk(struct blkfront_info *info)
-{
-   unsigned int minor, nr_minors, i;
-   struct blkfront_ring_info *rinfo;
-
-   if (info->rq == NULL)
-   return;
-
-   /* No more blkif_request(). */
-   blk_mq_stop_hw_queues(info->rq);
-
-   for_each_rinfo(info, rinfo, i) {
-   /* No more gnttab callback work. */
-   gnttab_cancel_free_callback(>callback);
-
-   /* Flush gnttab callback work. Must be done with no locks held. 
*/
-   flush_work(>work);
-   }
-
-   del_gendisk(info->gd);
-
-   minor = info->gd->first_minor;
-   nr_minors = info->gd->minors;
-   xlbd_release_minors(minor, nr_minors);
-
-   blk_cleanup_disk(info->gd);
-   info->gd = NULL;
-   blk_mq_free_tag_set(>tag_set);
-}
-
 /* Already hold rinfo->ring_lock. */
 static inline void kick_pending_request_queues_locked(struct 
blkfront_ring_info *rinfo)
 {
@@ -1756,12 +1713,6 @@ static int write_per_ring_nodes(struct 
xenbus_transaction xbt,
return err;
 }
 
-static void free_info(struct blkfront_info *info)
-{
-   list_del(>info_list);
-   kfree(info);
-}
-
 /* Common code used when first setting up, and when resuming. */
 static int talk_to_blkback(struct xenbus_device *dev,
   struct blkfront_info *info)
@@ -1880,13 +1831,6 @@ static int talk_to_blkback(struct xenbus_device *dev,
xenbus_dev_fatal(dev, err, "%s", message);
  destroy_blkring:
blkif_free(info, 0);
-
-   mutex_lock(_mutex);
-   free_info(info);
-   mutex_unlock(_mutex);
-
-   dev_set_drvdata(>dev, NULL);
-
return err;
 }
 
@@ -2126,38 +2070,26 @@ static int blkfront_resume(struct xenbus_device *dev)
 static void blkfront_closing(struct blkfront_info *info)
 {
struct xenbus_device *xbdev = info->xbdev;
-   struct block_device *bdev = NULL;
-
-   mutex_lock(>mutex);
+   struct blkfront_ring_info *rinfo;
+   unsigned int i;
 
-   if (xbdev->state == XenbusStateClosing) {
-   mutex_unlock(>mutex);
+   if (xbdev->state == XenbusStateClosing)
return;
-   }
 
-   if (info->gd)
-   bdev = bdgrab(info->gd->part0);
-
-   mutex_unlock(>mutex);
-
-   if (!bdev) {
-   xenbus_frontend_closed(xbdev);
-   return;
-   }
+   /* No more blkif_request(). */
+   blk_mq_stop_hw_queues(info->rq);
+   blk_set_queue_dying(info->rq);
+   set_capacity(info->gd, 0);
 
-