Re: [RFC PATCH] ide-floppy partitions

Re: [RFC PATCH] ide-floppy partitions

[Borislav Petkov]

On Wed, Oct 29, 2008 at 08:13:19AM +0100, Borislav Petkov wrote:
> Hi Tejun,
> 
> recent changes at 0762b8bde9729f10f8e6249809660ff2ec3ad735 and around
> break ide-floppy. Since it is a removable media drive and the partition
> scan during boot returns empty (no media in the drive), when you later
> put in a disk and try to mount it, mount returns saying
> 
> /dev/hdc4 is not a valid block device.
> 
> Which brings me to the other possible issue: Since having a hdc4
> partition as a single FAT16 partition on a ZIP drive is the "factory
> default" you could fabricate a case where you have a partition number> 1
> as the only partition on a hard drive too, i.e. no continuous
> partition numbering and the mount would theoretically fail there too
> since, for example, there's a check in disk_get_part() which does:
> 
> if (likely(partno < ptbl->len)) {
> 
> and in this case the check will fail if partno >= 1 while you have only
> one partition on the disk with a number higher than the partition table
> length and the above described failure will happen too. Anyways, this is
> just a hypothesis, but it happens with the ZIP drive here so other block
> devices should behave similarly.
> 
> Here's a patch that fixes the ide-floppy case a bit clumsily, I admit.
> 
> ---
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 88a776f..b798ea0 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1011,6 +1011,23 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
>  	disk = get_gendisk(bdev->bd_dev, &partno);
>  	if (!disk)
>  		goto out_unlock_kernel;
> +
> +	part = disk_get_part(disk, partno);
> +	if (!part) {
> +		struct block_device *whole;
> +
> +		mutex_lock_nested(&bdev->bd_mutex, for_part);
> +		whole = bdget_disk(disk, 0);
> +		ret = -ENOMEM;
> +		if (!whole)
> +			goto out_clear;
> +		ret = __blkdev_get(whole, mode, 1);
> +		if (ret)
> +			goto out_clear;
> +		bdev->bd_contains = whole;
> +		mutex_unlock(&bdev->bd_mutex);
> +	}
> +
>  	part = disk_get_part(disk, partno);
>  	if (!part)
>  		goto out_unlock_kernel;

Tejun, Jens,

can you guys please ACK/NACK this so that we can have an agreed upon solution
and so that I can stop applying it ontop of current git before testing any
further.

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: [RFC PATCH] ide-floppy partitions

[Tejun Heo]

Borislav Petkov wrote:
> can you guys please ACK/NACK this so that we can have an agreed upon solution
> and so that I can stop applying it ontop of current git before testing any
> further.

Ah... right.  I missed removable devices.  I think it would be better to
just expand the table rather than recursing into __blkdev_get() again.
I'll try to come up with cleaner solution.  Also, I don't think the
discontinuous partition would be a problem.  ptbl is expanded to the
highest numbered partition not the number of partitions.  I'll test that
too.  Thanks.

-- 
tejun

Re: [RFC PATCH] ide-floppy partitions

[Borislav Petkov]

Hi

On Mon, Nov 3, 2008 at 4:37 PM, Tejun Heo <tj@kernel.org> wrote:
> Borislav Petkov wrote:
>> can you guys please ACK/NACK this so that we can have an agreed upon solution
>> and so that I can stop applying it ontop of current git before testing any
>> further.
>
> Ah... right.  I missed removable devices.  I think it would be better to
> just expand the table rather than recursing into __blkdev_get() again.
> I'll try to come up with cleaner solution.

Sounds good. It should simply rescan partitions upon media change before it does
disk_get_part so that the ptbl is updated.

> Also, I don't think the
> discontinuous partition would be a problem.  ptbl is expanded to the
> highest numbered partition not the number of partitions.  I'll test that
> too.  Thanks.

You don't have to. I already did that with a harddrive in a external case over
usb and it works since upon connection the usb core rescans partitions and ptbl
is valid then.

Thanks.

-- 
Regards/Gruss,
Boris

Subject: [PATCH] block: fix __blkdev_get() for removable devices

[Tejun Heo]

Commit 0762b8bde9729f10f8e6249809660ff2ec3ad735 moved disk_get_part()
in front of recursive get on the whole disk, which caused removable
devices to try disk_get_part() before rescanning after a new media is
inserted, which might fail legit open attempts or give the old
partition.

This patch fixes the problem by moving disk_get_part() after
__blkdev_get() on the whole disk.

This problem was spotted by Borislav Petkov.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: <Borislav Petkov> petkovbb@googlemail.com
---
Borislav, can you please verify this patch?  Thanks.

 fs/block_dev.c |   23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 88a776f..db831ef 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -986,7 +986,6 @@ static int __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part);
 static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 {
 	struct gendisk *disk;
-	struct hd_struct *part = NULL;
 	int ret;
 	int partno;
 	int perm = 0;
@@ -1004,24 +1003,25 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 		return ret;
 	}
 
-	ret = -ENXIO;
-
 	lock_kernel();
 
+	ret = -ENXIO;
 	disk = get_gendisk(bdev->bd_dev, &partno);
 	if (!disk)
 		goto out_unlock_kernel;
-	part = disk_get_part(disk, partno);
-	if (!part)
-		goto out_unlock_kernel;
 
 	mutex_lock_nested(&bdev->bd_mutex, for_part);
 	if (!bdev->bd_openers) {
 		bdev->bd_disk = disk;
-		bdev->bd_part = part;
 		bdev->bd_contains = bdev;
 		if (!partno) {
 			struct backing_dev_info *bdi;
+
+			ret = -ENXIO;
+			bdev->bd_part = disk_get_part(disk, partno);
+			if (!bdev->bd_part)
+				goto out_clear;
+
 			if (disk->fops->open) {
 				ret = disk->fops->open(bdev, mode);
 				if (ret)
@@ -1049,18 +1049,17 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 			bdev->bd_contains = whole;
 			bdev->bd_inode->i_data.backing_dev_info =
 			   whole->bd_inode->i_data.backing_dev_info;
+			bdev->bd_part = disk_get_part(disk, partno);
 			if (!(disk->flags & GENHD_FL_UP) ||
-			    !part || !part->nr_sects) {
+			    !bdev->bd_part || !bdev->bd_part->nr_sects) {
 				ret = -ENXIO;
 				goto out_clear;
 			}
-			bd_set_size(bdev, (loff_t)part->nr_sects << 9);
+			bd_set_size(bdev, (loff_t)bdev->bd_part->nr_sects << 9);
 		}
 	} else {
-		disk_put_part(part);
 		put_disk(disk);
 		module_put(disk->fops->owner);
-		part = NULL;
 		disk = NULL;
 		if (bdev->bd_contains == bdev) {
 			if (bdev->bd_disk->fops->open) {
@@ -1080,6 +1079,7 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 	return 0;
 
  out_clear:
+	disk_put_part(bdev->bd_part);
 	bdev->bd_disk = NULL;
 	bdev->bd_part = NULL;
 	bdev->bd_inode->i_data.backing_dev_info = &default_backing_dev_info;
@@ -1091,7 +1091,6 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
  out_unlock_kernel:
 	unlock_kernel();
 
-	disk_put_part(part);
 	if (disk)
 		module_put(disk->fops->owner);
 	put_disk(disk);

Re: Subject: [PATCH] block: fix __blkdev_get() for removable devices

[Borislav Petkov]

On Tue, Nov 04, 2008 at 01:16:07PM +0900, Tejun Heo wrote:
> Commit 0762b8bde9729f10f8e6249809660ff2ec3ad735 moved disk_get_part()
> in front of recursive get on the whole disk, which caused removable
> devices to try disk_get_part() before rescanning after a new media is
> inserted, which might fail legit open attempts or give the old
> partition.
> 
> This patch fixes the problem by moving disk_get_part() after
> __blkdev_get() on the whole disk.
> 
> This problem was spotted by Borislav Petkov.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: <Borislav Petkov> petkovbb@googlemail.com
> ---
> Borislav, can you please verify this patch?  Thanks.

Yep, it works. Thanks.

Tested-by: Borislav Petkov <petkovbb@gmail.com>

-- 
Regards/Gruss,
    Boris.

Re: Subject: [PATCH] block: fix __blkdev_get() for removable devices

[Jens Axboe]

On Wed, Nov 05 2008, Borislav Petkov wrote:
> On Tue, Nov 04, 2008 at 01:16:07PM +0900, Tejun Heo wrote:
> > Commit 0762b8bde9729f10f8e6249809660ff2ec3ad735 moved disk_get_part()
> > in front of recursive get on the whole disk, which caused removable
> > devices to try disk_get_part() before rescanning after a new media is
> > inserted, which might fail legit open attempts or give the old
> > partition.
> > 
> > This patch fixes the problem by moving disk_get_part() after
> > __blkdev_get() on the whole disk.
> > 
> > This problem was spotted by Borislav Petkov.
> > 
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > Cc: <Borislav Petkov> petkovbb@googlemail.com
> > ---
> > Borislav, can you please verify this patch?  Thanks.
> 
> Yep, it works. Thanks.
> 
> Tested-by: Borislav Petkov <petkovbb@gmail.com>

I merged it and added your tested-by.

-- 
Jens Axboe