| Message ID | 20221007193825.4058951-1-kbusch@meta.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCHv2] block: fix leaking minors of hidden disks | expand |
On Fri, Oct 07, 2022 at 12:38:25PM -0700, Keith Busch wrote: > From: Keith Busch <kbusch@kernel.org> > > The major/minor of a hidden gendisk is not propagated to the block > device. This is required to suppress it from being visible. For these > disks, we need to handle freeing the dynamic minor directly when it's > released since bdev_free_inode() won't be able to. > > Cc: Christoph Hellwig <hch@lst.de> > Reported-by: Daniel Wagner <dwagner@suse.de> > Signed-off-by: Keith Busch <kbusch@kernel.org> Thanks for the quick fix! Tested-by: Daniel Wagner <dwagner@suse.de> # ls -l /dev/nvme* crw------- 1 root root 10, 124 Oct 10 08:39 /dev/nvme-fabrics crw------- 1 root root 243, 0 Oct 10 08:38 /dev/nvme0 brw-rw---- 1 root disk 259, 0 Oct 10 08:38 /dev/nvme0n1 crw------- 1 root root 243, 2 Oct 10 08:41 /dev/nvme2 brw-rw---- 1 root disk 259, 2 Oct 10 08:41 /dev/nvme2n1 brw-rw---- 1 root disk 259, 3 Oct 10 08:41 /dev/nvme2n1p1 brw-rw---- 1 root disk 259, 5 Oct 10 08:41 /dev/nvme2n2 brw-rw---- 1 root disk 259, 7 Oct 10 08:41 /dev/nvme2n3 brw-rw---- 1 root disk 259, 9 Oct 10 08:41 /dev/nvme2n4 crw------- 1 root root 243, 3 Oct 10 08:41 /dev/nvme3 crw------- 1 root root 243, 4 Oct 10 08:41 /dev/nvme4 crw------- 1 root root 243, 5 Oct 10 08:41 /dev/nvme5 # nvme disconnect-all # nvme connect-all # ls -l /dev/nvme* crw------- 1 root root 10, 124 Oct 10 08:39 /dev/nvme-fabrics crw------- 1 root root 243, 0 Oct 10 08:38 /dev/nvme0 brw-rw---- 1 root disk 259, 0 Oct 10 08:38 /dev/nvme0n1 crw------- 1 root root 243, 2 Oct 10 08:41 /dev/nvme2 brw-rw---- 1 root disk 259, 2 Oct 10 08:41 /dev/nvme2n1 brw-rw---- 1 root disk 259, 3 Oct 10 08:41 /dev/nvme2n1p1 brw-rw---- 1 root disk 259, 5 Oct 10 08:41 /dev/nvme2n2 brw-rw---- 1 root disk 259, 7 Oct 10 08:41 /dev/nvme2n3 brw-rw---- 1 root disk 259, 9 Oct 10 08:41 /dev/nvme2n4 crw------- 1 root root 243, 3 Oct 10 08:41 /dev/nvme3 crw------- 1 root root 243, 4 Oct 10 08:41 /dev/nvme4 crw------- 1 root root 243, 5 Oct 10 08:41 /dev/nvme5
On 10/7/22 12:38, Keith Busch wrote: > From: Keith Busch <kbusch@kernel.org> > > The major/minor of a hidden gendisk is not propagated to the block > device. This is required to suppress it from being visible. For these > disks, we need to handle freeing the dynamic minor directly when it's > released since bdev_free_inode() won't be able to. > > Cc: Christoph Hellwig <hch@lst.de> > Reported-by: Daniel Wagner <dwagner@suse.de> > Signed-off-by: Keith Busch <kbusch@kernel.org> > --- > v1->v2: > > Actually check that the disk is hidden before assuming the minor needs > to be freed. > > block/genhd.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/block/genhd.c b/block/genhd.c > index 514395361d7c..0afcdbb7575c 100644 > --- a/block/genhd.c > +++ b/block/genhd.c > @@ -1166,6 +1166,8 @@ static void disk_release(struct device *dev) > if (test_bit(GD_ADDED, &disk->state) && disk->fops->free_disk) > disk->fops->free_disk(disk); > > + if ((disk->flags & GENHD_FL_HIDDEN) && disk->major == BLOCK_EXT_MAJOR) > + blk_free_ext_minor(disk->first_minor); > iput(disk->part0->bd_inode); /* frees the disk */ > } > Looks good. Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com> -ck
On Fri, Oct 07, 2022 at 12:38:25PM -0700, Keith Busch wrote: > From: Keith Busch <kbusch@kernel.org> > > The major/minor of a hidden gendisk is not propagated to the block > device. This is required to suppress it from being visible. For these > disks, we need to handle freeing the dynamic minor directly when it's > released since bdev_free_inode() won't be able to. This now leads to a different lifetime. I think the proper fix is the one below (untested): diff --git a/block/genhd.c b/block/genhd.c index d36fabf0abc1f..1752ce356484e 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -507,6 +507,8 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk, */ dev_set_uevent_suppress(ddev, 0); disk_uevent(disk, KOBJ_ADD); + } else { + disk->part0->bd_dev = ddev->devt; } disk_update_readahead(disk);
On Mon, Oct 10, 2022 at 12:36:26AM -0700, Christoph Hellwig wrote: > On Fri, Oct 07, 2022 at 12:38:25PM -0700, Keith Busch wrote: > > From: Keith Busch <kbusch@kernel.org> > > > > The major/minor of a hidden gendisk is not propagated to the block > > device. This is required to suppress it from being visible. For these > > disks, we need to handle freeing the dynamic minor directly when it's > > released since bdev_free_inode() won't be able to. > > This now leads to a different lifetime. I think the proper fix is > the one below (untested): > > diff --git a/block/genhd.c b/block/genhd.c > index d36fabf0abc1f..1752ce356484e 100644 > --- a/block/genhd.c > +++ b/block/genhd.c > @@ -507,6 +507,8 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk, > */ > dev_set_uevent_suppress(ddev, 0); > disk_uevent(disk, KOBJ_ADD); > + } else { > + disk->part0->bd_dev = ddev->devt; > } > > disk_update_readahead(disk); Doesn't give me the same consistent result as with Keith's version: # ls -l /dev/nvme* crw------- 1 root root 10, 124 Oct 10 10:00 /dev/nvme-fabrics crw------- 1 root root 243, 0 Oct 10 09:58 /dev/nvme0 brw-rw---- 1 root disk 259, 0 Oct 10 09:58 /dev/nvme0n1 crw------- 1 root root 243, 2 Oct 10 10:00 /dev/nvme2 brw-rw---- 1 root disk 259, 2 Oct 10 10:00 /dev/nvme2n1 brw-rw---- 1 root disk 259, 3 Oct 10 10:00 /dev/nvme2n1p1 brw-rw---- 1 root disk 259, 5 Oct 10 10:00 /dev/nvme2n2 brw-rw---- 1 root disk 259, 7 Oct 10 10:00 /dev/nvme2n3 brw-rw---- 1 root disk 259, 9 Oct 10 10:00 /dev/nvme2n4 crw------- 1 root root 243, 3 Oct 10 10:00 /dev/nvme3 crw------- 1 root root 243, 4 Oct 10 10:00 /dev/nvme4 crw------- 1 root root 243, 5 Oct 10 10:00 /dev/nvme5 # nvme disconnect-all # nvme connect-all # ls -l /dev/nvme* crw------- 1 root root 10, 124 Oct 10 10:00 /dev/nvme-fabrics crw------- 1 root root 243, 0 Oct 10 09:58 /dev/nvme0 brw-rw---- 1 root disk 259, 0 Oct 10 09:58 /dev/nvme0n1 crw------- 1 root root 243, 2 Oct 10 10:00 /dev/nvme2 brw-rw---- 1 root disk 259, 3 Oct 10 10:00 /dev/nvme2n1 brw-rw---- 1 root disk 259, 5 Oct 10 10:00 /dev/nvme2n1p1 brw-rw---- 1 root disk 259, 9 Oct 10 10:00 /dev/nvme2n2 brw-rw---- 1 root disk 259, 23 Oct 10 10:00 /dev/nvme2n3 brw-rw---- 1 root disk 259, 25 Oct 10 10:00 /dev/nvme2n4 crw------- 1 root root 243, 3 Oct 10 10:00 /dev/nvme3 crw------- 1 root root 243, 4 Oct 10 10:00 /dev/nvme4 crw------- 1 root root 243, 5 Oct 10 10:00 /dev/nvme5
On Mon, Oct 10, 2022 at 10:02:18AM +0200, Daniel Wagner wrote:
> Doesn't give me the same consistent result as with Keith's version:
That's because it is broken.. Try this version:
diff --git a/block/genhd.c b/block/genhd.c
index 514395361d7c5..2296ad422523a 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -507,6 +507,8 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
*/
dev_set_uevent_suppress(ddev, 0);
disk_uevent(disk, KOBJ_ADD);
+ } else {
+ disk->part0->bd_dev = MKDEV(disk->major, disk->first_minor);
}
disk_update_readahead(disk);
On Mon, Oct 10, 2022 at 10:19:02AM +0200, Christoph Hellwig wrote: > On Mon, Oct 10, 2022 at 10:02:18AM +0200, Daniel Wagner wrote: > > Doesn't give me the same consistent result as with Keith's version: > > That's because it is broken.. Try this version: This version works. Tested-by: Daniel Wagner <dwagner@suse.de> # ls -l /dev/nvme* crw------- 1 root root 10, 124 Oct 10 10:47 /dev/nvme-fabrics crw------- 1 root root 243, 0 Oct 10 10:46 /dev/nvme0 brw-rw---- 1 root disk 259, 0 Oct 10 10:46 /dev/nvme0n1 crw------- 1 root root 243, 2 Oct 10 10:48 /dev/nvme2 brw-rw---- 1 root disk 259, 2 Oct 10 10:48 /dev/nvme2n1 brw-rw---- 1 root disk 259, 3 Oct 10 10:48 /dev/nvme2n1p1 brw-rw---- 1 root disk 259, 5 Oct 10 10:48 /dev/nvme2n2 brw-rw---- 1 root disk 259, 7 Oct 10 10:48 /dev/nvme2n3 brw-rw---- 1 root disk 259, 9 Oct 10 10:48 /dev/nvme2n4 crw------- 1 root root 243, 3 Oct 10 10:48 /dev/nvme3 crw------- 1 root root 243, 4 Oct 10 10:48 /dev/nvme4 crw------- 1 root root 243, 5 Oct 10 10:48 /dev/nvme5 # nvme disconnect-all # nvme connect-all # ls -l /dev/nvme* crw------- 1 root root 10, 124 Oct 10 10:47 /dev/nvme-fabrics crw------- 1 root root 243, 0 Oct 10 10:46 /dev/nvme0 brw-rw---- 1 root disk 259, 0 Oct 10 10:46 /dev/nvme0n1 crw------- 1 root root 243, 2 Oct 10 10:48 /dev/nvme2 brw-rw---- 1 root disk 259, 2 Oct 10 10:48 /dev/nvme2n1 brw-rw---- 1 root disk 259, 3 Oct 10 10:48 /dev/nvme2n1p1 brw-rw---- 1 root disk 259, 5 Oct 10 10:48 /dev/nvme2n2 brw-rw---- 1 root disk 259, 7 Oct 10 10:48 /dev/nvme2n3 brw-rw---- 1 root disk 259, 9 Oct 10 10:48 /dev/nvme2n4 crw------- 1 root root 243, 3 Oct 10 10:48 /dev/nvme3 crw------- 1 root root 243, 4 Oct 10 10:48 /dev/nvme4 crw------- 1 root root 243, 5 Oct 10 10:48 /dev/nvme5
diff --git a/block/genhd.c b/block/genhd.c index 514395361d7c..0afcdbb7575c 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -1166,6 +1166,8 @@ static void disk_release(struct device *dev) if (test_bit(GD_ADDED, &disk->state) && disk->fops->free_disk) disk->fops->free_disk(disk); + if ((disk->flags & GENHD_FL_HIDDEN) && disk->major == BLOCK_EXT_MAJOR) + blk_free_ext_minor(disk->first_minor); iput(disk->part0->bd_inode); /* frees the disk */ }