[PATCH 1/5] mtd: fix bdev exclusive open bugs in block2mtd::add_device()

[PATCH 1/5] mtd: fix bdev exclusive open bugs in block2mtd::add_device()

[Tejun Heo]

There are two bdev exclusive open bugs.

* open_bdev_exclusive() must not be called with NULL holder.  Use dev
  as the holder.

* open_by_devnum() doesn't open the bdev exclusively but
  block2mtd_free_device() always assumes it.  Explicitly claim the
  bdev.

The latter is rather clumsy but will be simplified with future
blkdev_get/put() cleanups.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: linux-mtd@lists.infradead.org
---
 drivers/mtd/devices/block2mtd.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
index 2cf0cc6..806a9ed 100644
--- a/drivers/mtd/devices/block2mtd.c
+++ b/drivers/mtd/devices/block2mtd.c
@@ -246,7 +246,7 @@ static struct block2mtd_dev *add_device(char *devname, int erase_size)
 		return NULL;
 
 	/* Get a handle on the device */
-	bdev = open_bdev_exclusive(devname, FMODE_READ|FMODE_WRITE, NULL);
+	bdev = open_bdev_exclusive(devname, FMODE_READ|FMODE_WRITE, dev);
 #ifndef MODULE
 	if (IS_ERR(bdev)) {
 
@@ -256,6 +256,15 @@ static struct block2mtd_dev *add_device(char *devname, int erase_size)
 		dev_t devt = name_to_dev_t(devname);
 		if (devt) {
 			bdev = open_by_devnum(devt, FMODE_WRITE | FMODE_READ);
+			if (!IS_ERR(bdev)) {
+				int ret;
+				ret = bd_claim(bdev, dev);
+				if (ret) {
+					blkdev_put(bdev,
+						   FMODE_READ | FMODE_WRITE);
+					bdev = ERR_PTR(ret);
+				}
+			}
 		}
 	}
 #endif
-- 
1.7.1

Re: [PATCH 1/5] mtd: fix bdev exclusive open bugs in block2mtd::add_device()

[Artem Bityutskiy]

Hi,

On Mon, 2010-11-01 at 17:15 +0100, Tejun Heo wrote:
> +			if (!IS_ERR(bdev)) {
> +				int ret;
> +				ret = bd_claim(bdev, dev);
> +				if (ret) {
> +					blkdev_put(bdev,
> +						   FMODE_READ | FMODE_WRITE);

Would be a bit cleaner to define ea temporary variable:

fmode_t mode = FMODE_READ | FMODE_WRITE;

Would you to re-send with this little change please?

And ideally, 2 independent patches would be nicer because you fix 2
independent issues.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 1/5] mtd: fix bdev exclusive open bugs in block2mtd::add_device()

[Tejun Heo]

Hello,

On 11/13/2010 11:38 AM, Artem Bityutskiy wrote:
> On Mon, 2010-11-01 at 17:15 +0100, Tejun Heo wrote:
>> +			if (!IS_ERR(bdev)) {
>> +				int ret;
>> +				ret = bd_claim(bdev, dev);
>> +				if (ret) {
>> +					blkdev_put(bdev,
>> +						   FMODE_READ | FMODE_WRITE);
> 
> Would be a bit cleaner to define ea temporary variable:
> 
> fmode_t mode = FMODE_READ | FMODE_WRITE;
> 
> Would you to re-send with this little change please?

Yeap, sure.

> And ideally, 2 independent patches would be nicer because you fix 2
> independent issues.

Hmmm... not really.  The patch is small enough and splitting it won't
really buy as any better bisectability.  Splitting patches into
logical changes is a good thing but it's no religious dogma.  Let's
apply it to the point it actually helps.

Thank you.

-- 
tejun

Re: [PATCH 1/5] mtd: fix bdev exclusive open bugs in block2mtd::add_device()

[Artem Bityutskiy]

On Sat, 2010-11-13 at 11:42 +0100, Tejun Heo wrote:
> Hmmm... not really.  The patch is small enough and splitting it won't
> really buy as any better bisectability.  Splitting patches into
> logical changes is a good thing but it's no religious dogma.  Let's
> apply it to the point it actually helps.

Well, it is more about if your second fix is buggy, we could revert it
without reverting the first fix. But anyway, this is not a big deal, so
let's forget about this indeed.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

[PATCH UPDATED 1/5] mtd: fix bdev exclusive open bugs in block2mtd::add_device()

[Tejun Heo]

There are two bdev exclusive open bugs.

* open_bdev_exclusive() must not be called with NULL holder.  Use dev
  as the holder.

* open_by_devnum() doesn't open the bdev exclusively but
  block2mtd_free_device() always assumes it.  Explicitly claim the
  bdev.

The latter is rather clumsy but will be simplified with future
blkdev_get/put() cleanups.

- Updated to use local variable @mode to cache FMODE_* masks as
  suggested by Artem Bityutskiy.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: linux-mtd@lists.infradead.org
Cc: Artem Bityutskiy <dedekind1@gmail.com>
---

This change will cause two conflicts in the series.  I'm not reposting
them now as they're mostly trivial.  Git tree is fully updated.

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git cleanup-bd_claim

Jens, please let me know when/if you want the full series reposted.

Thanks.

 drivers/mtd/devices/block2mtd.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

Index: work/drivers/mtd/devices/block2mtd.c
===================================================================
--- work.orig/drivers/mtd/devices/block2mtd.c
+++ work/drivers/mtd/devices/block2mtd.c
@@ -234,6 +234,7 @@ static void block2mtd_free_device(struct
 /* FIXME: ensure that mtd->size % erase_size == 0 */
 static struct block2mtd_dev *add_device(char *devname, int erase_size)
 {
+	const fmode_t mode = FMODE_READ | FMODE_WRITE;
 	struct block_device *bdev;
 	struct block2mtd_dev *dev;
 	char *name;
@@ -246,7 +247,7 @@ static struct block2mtd_dev *add_device(
 		return NULL;

 	/* Get a handle on the device */
-	bdev = open_bdev_exclusive(devname, FMODE_READ|FMODE_WRITE, NULL);
+	bdev = open_bdev_exclusive(devname, mode, dev);
 #ifndef MODULE
 	if (IS_ERR(bdev)) {

@@ -255,7 +256,15 @@ static struct block2mtd_dev *add_device(

 		dev_t devt = name_to_dev_t(devname);
 		if (devt) {
-			bdev = open_by_devnum(devt, FMODE_WRITE | FMODE_READ);
+			bdev = open_by_devnum(devt, mode);
+			if (!IS_ERR(bdev)) {
+				int ret;
+				ret = bd_claim(bdev, dev);
+				if (ret) {
+					blkdev_put(bdev, mode);
+					bdev = ERR_PTR(ret);
+				}
+			}
 		}
 	}
 #endif

Re: [PATCH UPDATED 1/5] mtd: fix bdev exclusive open bugs in block2mtd::add_device()

[Artem Bityutskiy]

On Sat, 2010-11-13 at 11:59 +0100, Tejun Heo wrote:
> There are two bdev exclusive open bugs.
> 
> * open_bdev_exclusive() must not be called with NULL holder.  Use dev
>   as the holder.
> 
> * open_by_devnum() doesn't open the bdev exclusively but
>   block2mtd_free_device() always assumes it.  Explicitly claim the
>   bdev.
> 
> The latter is rather clumsy but will be simplified with future
> blkdev_get/put() cleanups.
> 
> - Updated to use local variable @mode to cache FMODE_* masks as
>   suggested by Artem Bityutskiy.

Do you want this patch to go in via Jens' tree with the rest of the
series, or as an independent patch via the mtd tree?

Anyway,

Acked-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

Thanks!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH UPDATED 1/5] mtd: fix bdev exclusive open bugs in block2mtd::add_device()

[Tejun Heo]

Hello,

On 11/13/2010 12:14 PM, Artem Bityutskiy wrote:
> On Sat, 2010-11-13 at 11:59 +0100, Tejun Heo wrote:
>> There are two bdev exclusive open bugs.
>>
>> * open_bdev_exclusive() must not be called with NULL holder.  Use dev
>>   as the holder.
>>
>> * open_by_devnum() doesn't open the bdev exclusively but
>>   block2mtd_free_device() always assumes it.  Explicitly claim the
>>   bdev.
>>
>> The latter is rather clumsy but will be simplified with future
>> blkdev_get/put() cleanups.
>>
>> - Updated to use local variable @mode to cache FMODE_* masks as
>>   suggested by Artem Bityutskiy.
> 
> Do you want this patch to go in via Jens' tree with the rest of the
> series, or as an independent patch via the mtd tree?

It probably would be easier if this is routed through Jens' tree as
it's necessary for later patches in the series, but this being a fix,
this should be merged during this cycle rather than the next one.
Jens, how do you want to proceed?

Thank you.

-- 
tejun