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: sta...@vger.kernel.org (v2.6.35+)
> Signed-off-by: NeilBrown <ne...@suse.com>
> ---
>
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
