Re: [PATCH resend] block_dev: don't test bdev->bd_contains when it is not stable.

[Jens Axboe]

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: stable@xxxxxxxxxxxxxxx (v2.6.35+)
> Signed-off-by: NeilBrown <neilb@xxxxxxxx>
> ---
> 
> 
> 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