bdev->bd_contains is not stable before calling __blkdev_get().
When __blkdev_get() is called on a parition with ->bd_openers == 0
it sets
bdev->bd_contains = bdev;
which is not correct for a partition.
After a call to __blkdev_get() succeeds, ->bd_openers will be > 0
and then ->bd_contains is stable.
When FMODE_EXCL is used, blkdev_get() calls
bd_start_claiming() -> bd_prepare_to_claim() -> bd_may_claim()
This call happens before __blkdev_get() is called, so ->bd_contains
is not stable. So bd_may_claim() cannot safely use ->bd_contains.
It currently tries to use it, and this can lead to a BUG_ON().
This happens when a whole device is already open with a bd_holder (in
use by dm in my particular example) and two threads race to open a
partition of that device for the first time, one opening with O_EXCL and
one without.
The thread that doesn't use O_EXCL gets through blkdev_get() to
__blkdev_get(), gains the ->bd_mutex, and sets bdev->bd_contains = bdev;
Immediately thereafter the other thread, using FMODE_EXCL, calls
bd_start_claiming() from blkdev_get(). This should fail because the
whole device has a holder, but because bdev->bd_contains == bdev
bd_may_claim() incorrectly reports success.
This thread continues and blocks on bd_mutex.
The first thread then sets bdev->bd_contains correctly and drops the mutex.
The thread using FMODE_EXCL then continues and when it calls bd_may_claim()
again in:
BUG_ON(!bd_may_claim(bdev, whole, holder));
The BUG_ON fires.
Fix this by removing the dependency on ->bd_contains in
bd_may_claim(). As bd_may_claim() has direct access to the whole
device, it can simply test if the target bdev is the whole device.
Fixes: 6b4517a7913a ("block: implement bd_claiming and claiming block")
Cc: [email protected] (v2.6.35+)
Signed-off-by: NeilBrown <[email protected]>
---
fs/block_dev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 05b553368bb4..9166b9f63d33 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -832,7 +832,7 @@ static bool bd_may_claim(struct block_device *bdev, struct block_device *whole,
return true; /* already a holder */
else if (bdev->bd_holder != NULL)
return false; /* held by someone else */
- else if (bdev->bd_contains == bdev)
+ else if (whole == bdev)
return true; /* is a whole device which isn't held */
else if (whole->bd_holder == bd_may_claim)
--
2.10.2
bdev->bd_contains is not stable before calling __blkdev_get().
When __blkdev_get() is called on a parition with ->bd_openers == 0
it sets
bdev->bd_contains = bdev;
which is not correct for a partition.
After a call to __blkdev_get() succeeds, ->bd_openers will be > 0
and then ->bd_contains is stable.
When FMODE_EXCL is used, blkdev_get() calls
bd_start_claiming() -> bd_prepare_to_claim() -> bd_may_claim()
This call happens before __blkdev_get() is called, so ->bd_contains
is not stable. So bd_may_claim() cannot safely use ->bd_contains.
It currently tries to use it, and this can lead to a BUG_ON().
This happens when a whole device is open with a bd_holder (in use by
dm in my particular example) and two threads race to open a partition
for the first time, one opening with O_EXCL and one without.
The thread that doesn't use O_EXCL gets through blkdev_get() to
__blkdev_get(), gains the ->bd_mutex, and sets bdev->bd_contains = bdev;
Immediately thereafter the other thread, using FMODE_EXCL, calls
bd_start_claiming from blkdev_get(). This should fail because the
whole device has a holder, but because bdev->bd_contains == bdev
bd_may_claim() incorrectly reports success.
This thread continues and blocks on bd_mutex.
The first thread then sets bdev->bd_contains correctly and drops the mutex.
The thread using FMODE_EXCL then continues and when it calls bd_may_claim()
again in:
BUG_ON(!bd_may_claim(bdev, whole, holder));
The BUG_ON fires.
Fix this by removing the dependency on ->bd_contains in
bd_may_claim(). As bd_may_claim() have direct access to the whole
device, it can simply test if the target bdev is the whole device.
Fixes: 6b4517a7913a ("block: implement bd_claiming and claiming block")
Cc: [email protected] (v2.6.35+)
Signed-off-by: NeilBrown <[email protected]>
---
No reply after 2 weeks, and this didn't appear in -next, so I'm adding a
couple more addresses - hopefully someone can take it.
Thanks,
NeilBrown
fs/block_dev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 05b553368bb4..9166b9f63d33 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -832,7 +832,7 @@ static bool bd_may_claim(struct block_device *bdev, struct block_device *whole,
return true; /* already a holder */
else if (bdev->bd_holder != NULL)
return false; /* held by someone else */
- else if (bdev->bd_contains == bdev)
+ else if (whole == bdev)
return true; /* is a whole device which isn't held */
else if (whole->bd_holder == bd_may_claim)
--
2.11.0
On 12/11/2016 05:29 PM, NeilBrown wrote:
>
> bdev->bd_contains is not stable before calling __blkdev_get().
> When __blkdev_get() is called on a parition with ->bd_openers == 0
> it sets
> bdev->bd_contains = bdev;
> which is not correct for a partition.
> After a call to __blkdev_get() succeeds, ->bd_openers will be > 0
> and then ->bd_contains is stable.
>
> When FMODE_EXCL is used, blkdev_get() calls
> bd_start_claiming() -> bd_prepare_to_claim() -> bd_may_claim()
>
> This call happens before __blkdev_get() is called, so ->bd_contains
> is not stable. So bd_may_claim() cannot safely use ->bd_contains.
> It currently tries to use it, and this can lead to a BUG_ON().
>
> This happens when a whole device is open with a bd_holder (in use by
> dm in my particular example) and two threads race to open a partition
> for the first time, one opening with O_EXCL and one without.
>
> The thread that doesn't use O_EXCL gets through blkdev_get() to
> __blkdev_get(), gains the ->bd_mutex, and sets bdev->bd_contains = bdev;
>
> Immediately thereafter the other thread, using FMODE_EXCL, calls
> bd_start_claiming from blkdev_get(). This should fail because the
> whole device has a holder, but because bdev->bd_contains == bdev
> bd_may_claim() incorrectly reports success.
>
> This thread continues and blocks on bd_mutex.
> The first thread then sets bdev->bd_contains correctly and drops the mutex.
> The thread using FMODE_EXCL then continues and when it calls bd_may_claim()
> again in:
> BUG_ON(!bd_may_claim(bdev, whole, holder));
> The BUG_ON fires.
>
> Fix this by removing the dependency on ->bd_contains in
> bd_may_claim(). As bd_may_claim() have direct access to the whole
> device, it can simply test if the target bdev is the whole device.
>
> Fixes: 6b4517a7913a ("block: implement bd_claiming and claiming block")
> Cc: [email protected] (v2.6.35+)
> Signed-off-by: NeilBrown <[email protected]>
> ---
>
>
> No reply after 2 weeks, and this didn't appear in -next, so I'm adding a
> couple more addresses - hopefully someone can take it.
> Thanks,
> NeilBrown
>
>
> fs/block_dev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 05b553368bb4..9166b9f63d33 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -832,7 +832,7 @@ static bool bd_may_claim(struct block_device *bdev, struct block_device *whole,
> return true; /* already a holder */
> else if (bdev->bd_holder != NULL)
> return false; /* held by someone else */
> - else if (bdev->bd_contains == bdev)
> + else if (whole == bdev)
> return true; /* is a whole device which isn't held */
>
> else if (whole->bd_holder == bd_may_claim)
>
Looks good to me, Neil. I'll get it applied, thanks.
--
Jens Axboe