[PATCH] btrfs: Fix bug for misused dev_t when lookup in dev state hash table.

[PATCH] btrfs: Fix bug for misused dev_t when lookup in dev state hash table.

[Gu Jinxiang]

From: Gu JinXiang <gujx@cn.fujitsu.com>

Fix bug of commit 74d46992e0d9
("block: replace bi_bdev with a gendisk pointer and partitions index").

In this modify, use bio_dev(bio) to find dev state in function
__btrfsic_submit_bio. But when dev_state added to hashtable, it is using
dev_t of block_device.

Reproduce of this bug:
Use MOUNT_OPTIONS="-o check_int" when run btrfs/001 in xfstest.
Then there will be WARNING like below.
WARNING:
btrfs: attempt to write superblock which references block M @29523968 (sda7     /1111654400/2) which is never written!

Signed-off-by: Gu JinXiang <gujx@cn.fujitsu.com>
---
 fs/btrfs/check-integrity.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
index fb07e3c22b9a..02f9eb83173f 100644
--- a/fs/btrfs/check-integrity.c
+++ b/fs/btrfs/check-integrity.c
@@ -2803,7 +2803,7 @@ static void __btrfsic_submit_bio(struct bio *bio)
 	mutex_lock(&btrfsic_mutex);
 	/* since btrfsic_submit_bio() is also called before
 	 * btrfsic_mount(), this might return NULL */
-	dev_state = btrfsic_dev_state_lookup(bio_dev(bio));
+	dev_state = btrfsic_dev_state_lookup(bio_dev(bio) + bio->bi_partno);
 	if (NULL != dev_state &&
 	    (bio_op(bio) == REQ_OP_WRITE) && bio_has_data(bio)) {
 		unsigned int i = 0;
-- 
2.13.5

Re: [PATCH] btrfs: Fix bug for misused dev_t when lookup in dev state hash table.

[Nikolay Borisov]

On 17.10.2017 14:34, Gu Jinxiang wrote:
> From: Gu JinXiang <gujx@cn.fujitsu.com>
> 
> Fix bug of commit 74d46992e0d9
> ("block: replace bi_bdev with a gendisk pointer and partitions index").
> 
> In this modify, use bio_dev(bio) to find dev state in function
> __btrfsic_submit_bio. But when dev_state added to hashtable, it is using
> dev_t of block_device.

This is rather incomprehensible. So bio_dev(bio) actually returns the
dev_t of the device to which this bio is submitted and the same dev_t
should be used when btrfsic_dev_state_hashtable_add is called? What am I
missing in here?


> 
> Reproduce of this bug:
> Use MOUNT_OPTIONS="-o check_int" when run btrfs/001 in xfstest.
> Then there will be WARNING like below.
> WARNING:
> btrfs: attempt to write superblock which references block M @29523968 (sda7     /1111654400/2) which is never written!
> 
> Signed-off-by: Gu JinXiang <gujx@cn.fujitsu.com>
> ---
>  fs/btrfs/check-integrity.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
> index fb07e3c22b9a..02f9eb83173f 100644
> --- a/fs/btrfs/check-integrity.c
> +++ b/fs/btrfs/check-integrity.c
> @@ -2803,7 +2803,7 @@ static void __btrfsic_submit_bio(struct bio *bio)
>  	mutex_lock(&btrfsic_mutex);
>  	/* since btrfsic_submit_bio() is also called before
>  	 * btrfsic_mount(), this might return NULL */
> -	dev_state = btrfsic_dev_state_lookup(bio_dev(bio));
> +	dev_state = btrfsic_dev_state_lookup(bio_dev(bio) + bio->bi_partno);

So this function looks up in btrfsic_dev_state_hashtable. And stuff in
this hashtable ias added via btrfsic_dev_state_hashtable_add function
which seems to be only using the dev_t (after your other patch is applied):

static void btrfsic_dev_state_hashtable_add(

                struct btrfsic_dev_state *ds,

                struct btrfsic_dev_state_hashtable *h)

{

        const unsigned int hashval =

            (((unsigned int)((uintptr_t)ds->bdev->bd_dev)) &

             (BTRFSIC_DEV2STATE_HASHTABLE_SIZE - 1));



        list_add(&ds->collision_resolving_node, h->table + hashval);

}


So how come your change is correct since you are passing the dev_t +
partition number?

>  	if (NULL != dev_state &&
>  	    (bio_op(bio) == REQ_OP_WRITE) && bio_has_data(bio)) {
>  		unsigned int i = 0;
> 

RE: [PATCH] btrfs: Fix bug for misused dev_t when lookup in dev state hash table.

[Gu, Jinxiang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3308 bytes --]

Hi，

> -----Original Message-----
> From: Nikolay Borisov [mailto:nborisov@suse.com]
> Sent: Tuesday, October 17, 2017 9:36 PM
> To: Gu, Jinxiang/顾 金香 <gujx@cn.fujitsu.com>; linux-btrfs@vger.kernel.org; hch@lst.de
> Subject: Re: [PATCH] btrfs: Fix bug for misused dev_t when lookup in dev state hash table.
> 
> 
> 
> On 17.10.2017 14:34, Gu Jinxiang wrote:
> > From: Gu JinXiang <gujx@cn.fujitsu.com>
> >
> > Fix bug of commit 74d46992e0d9
> > ("block: replace bi_bdev with a gendisk pointer and partitions index").
> >
> > In this modify, use bio_dev(bio) to find dev state in function
> > __btrfsic_submit_bio. But when dev_state added to hashtable, it is
> > using dev_t of block_device.
> 
> This is rather incomprehensible. So bio_dev(bio) actually returns the dev_t of the device to which this bio is submitted
> and the same dev_t should be used when btrfsic_dev_state_hashtable_add is called? What am I missing in here?
> 

bio_dev(bio) returns a dev_t of part0 which is different from dev_t in block_device(bd_dev).
bd_dev in block_device represents the exact partition.
block_device.bd_dev = bio->bi_partno (same as block_device.bd_partno) + bio_dev(bio).

When add a dev_state into hashtable it is using the exact partition's dev_t.
So when lookup it, it should also use the exact partition's dev_t.

> 
> >
> > Reproduce of this bug:
> > Use MOUNT_OPTIONS="-o check_int" when run btrfs/001 in xfstest.
> > Then there will be WARNING like below.
> > WARNING:
> > btrfs: attempt to write superblock which references block M @29523968 (sda7     /1111654400/2) which is never written!
> >
> > Signed-off-by: Gu JinXiang <gujx@cn.fujitsu.com>
> > ---
> >  fs/btrfs/check-integrity.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
> > index fb07e3c22b9a..02f9eb83173f 100644
> > --- a/fs/btrfs/check-integrity.c
> > +++ b/fs/btrfs/check-integrity.c
> > @@ -2803,7 +2803,7 @@ static void __btrfsic_submit_bio(struct bio *bio)
> >  	mutex_lock(&btrfsic_mutex);
> >  	/* since btrfsic_submit_bio() is also called before
> >  	 * btrfsic_mount(), this might return NULL */
> > -	dev_state = btrfsic_dev_state_lookup(bio_dev(bio));
> > +	dev_state = btrfsic_dev_state_lookup(bio_dev(bio) + bio->bi_partno);
> 
> So this function looks up in btrfsic_dev_state_hashtable. And stuff in this hashtable ias added via
> btrfsic_dev_state_hashtable_add function which seems to be only using the dev_t (after your other patch is applied):
> 
> static void btrfsic_dev_state_hashtable_add(
> 
>                 struct btrfsic_dev_state *ds,
> 
>                 struct btrfsic_dev_state_hashtable *h)
> 
> {
> 
>         const unsigned int hashval =
> 
>             (((unsigned int)((uintptr_t)ds->bdev->bd_dev)) &
> 
>              (BTRFSIC_DEV2STATE_HASHTABLE_SIZE - 1));
> 
> 
> 
>         list_add(&ds->collision_resolving_node, h->table + hashval);
> 
> }
> 
> 
> So how come your change is correct since you are passing the dev_t + partition number?
> 
> >  	if (NULL != dev_state &&
> >  	    (bio_op(bio) == REQ_OP_WRITE) && bio_has_data(bio)) {
> >  		unsigned int i = 0;
> >
> 



ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±ý»k~ÏâžØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&£ûàz¿äz¹Þ—ú+€Ê+zf£¢·hšˆ§~†­†Ûiÿÿïêÿ‘êçz_è®\x0fæj:+v‰¨þ)ߣøm

Re: [PATCH] btrfs: Fix bug for misused dev_t when lookup in dev state hash table.

[Nikolay Borisov]

On 18.10.2017 06:43, Gu, Jinxiang wrote:
> Hi，
> 
>> -----Original Message-----
>> From: Nikolay Borisov [mailto:nborisov@suse.com]
>> Sent: Tuesday, October 17, 2017 9:36 PM
>> To: Gu, Jinxiang/顾 金香 <gujx@cn.fujitsu.com>; linux-btrfs@vger.kernel.org; hch@lst.de
>> Subject: Re: [PATCH] btrfs: Fix bug for misused dev_t when lookup in dev state hash table.
>>
>>
>>
>> On 17.10.2017 14:34, Gu Jinxiang wrote:
>>> From: Gu JinXiang <gujx@cn.fujitsu.com>
>>>
>>> Fix bug of commit 74d46992e0d9
>>> ("block: replace bi_bdev with a gendisk pointer and partitions index").
>>>
>>> In this modify, use bio_dev(bio) to find dev state in function
>>> __btrfsic_submit_bio. But when dev_state added to hashtable, it is
>>> using dev_t of block_device.
>>
>> This is rather incomprehensible. So bio_dev(bio) actually returns the dev_t of the device to which this bio is submitted
>> and the same dev_t should be used when btrfsic_dev_state_hashtable_add is called? What am I missing in here?
>>
> 
> bio_dev(bio) returns a dev_t of part0 which is different from dev_t in block_device(bd_dev).
> bd_dev in block_device represents the exact partition.
> block_device.bd_dev = bio->bi_partno (same as block_device.bd_partno) + bio_dev(bio).
> 
> When add a dev_state into hashtable it is using the exact partition's dev_t.
> So when lookup it, it should also use the exact partition's dev_t.

Right, ok. Can you please put this explanation into the changelog of the
patch and resend

> 
>>
>>>
>>> Reproduce of this bug:
>>> Use MOUNT_OPTIONS="-o check_int" when run btrfs/001 in xfstest.
>>> Then there will be WARNING like below.
>>> WARNING:
>>> btrfs: attempt to write superblock which references block M @29523968 (sda7     /1111654400/2) which is never written!
>>>
>>> Signed-off-by: Gu JinXiang <gujx@cn.fujitsu.com>
>>> ---
>>>  fs/btrfs/check-integrity.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
>>> index fb07e3c22b9a..02f9eb83173f 100644
>>> --- a/fs/btrfs/check-integrity.c
>>> +++ b/fs/btrfs/check-integrity.c
>>> @@ -2803,7 +2803,7 @@ static void __btrfsic_submit_bio(struct bio *bio)
>>>  	mutex_lock(&btrfsic_mutex);
>>>  	/* since btrfsic_submit_bio() is also called before
>>>  	 * btrfsic_mount(), this might return NULL */
>>> -	dev_state = btrfsic_dev_state_lookup(bio_dev(bio));
>>> +	dev_state = btrfsic_dev_state_lookup(bio_dev(bio) + bio->bi_partno);
>>
>> So this function looks up in btrfsic_dev_state_hashtable. And stuff in this hashtable ias added via
>> btrfsic_dev_state_hashtable_add function which seems to be only using the dev_t (after your other patch is applied):
>>
>> static void btrfsic_dev_state_hashtable_add(
>>
>>                 struct btrfsic_dev_state *ds,
>>
>>                 struct btrfsic_dev_state_hashtable *h)
>>
>> {
>>
>>         const unsigned int hashval =
>>
>>             (((unsigned int)((uintptr_t)ds->bdev->bd_dev)) &
>>
>>              (BTRFSIC_DEV2STATE_HASHTABLE_SIZE - 1));
>>
>>
>>
>>         list_add(&ds->collision_resolving_node, h->table + hashval);
>>
>> }
>>
>>
>> So how come your change is correct since you are passing the dev_t + partition number?
>>
>>>  	if (NULL != dev_state &&
>>>  	    (bio_op(bio) == REQ_OP_WRITE) && bio_has_data(bio)) {
>>>  		unsigned int i = 0;
>>>
>>
> 
> 
>