loading scsi_debug with data integrity hits BUG_ON

loading scsi_debug with data integrity hits BUG_ON

[Akinobu Mita]

After commit 5837c80e870b ("bio-integrity: Fix bio_integrity_verify
segment start bug"), loading scsi_debug with data integrity enabled
(for example modprobe scsi_debug dif=1 dix=1) starts hitting BUG_ON()
at bio_integrity_verify():

        BUG_ON(total > bio->bi_integrity->bip_iter.bi_size);

Because bip_iter.bi_size has already been set to zero at this point
as a result of consuming bip_iter.

I can resolve this issue by removing the BUG_ON().  But I would like
to ask whether we should preserve an alternative assert, or not
before submitting the patch that simply removing the line.

Re: loading scsi_debug with data integrity hits BUG_ON

[Nicholas A. Bellinger]

Hi Akinobu,

On Wed, 2014-02-19 at 22:22 +0900, Akinobu Mita wrote:
> After commit 5837c80e870b ("bio-integrity: Fix bio_integrity_verify
> segment start bug"), loading scsi_debug with data integrity enabled
> (for example modprobe scsi_debug dif=1 dix=1) starts hitting BUG_ON()
> at bio_integrity_verify():
> 
>         BUG_ON(total > bio->bi_integrity->bip_iter.bi_size);
> 
> Because bip_iter.bi_size has already been set to zero at this point
> as a result of consuming bip_iter.
> 
> I can resolve this issue by removing the BUG_ON().  But I would like
> to ask whether we should preserve an alternative assert, or not
> before submitting the patch that simply removing the line.

So commit 5837c80e870b just allowed bio_integrity_verify() to actually
process protection information for READs again, which had been broken
since v3.10 code.

The bug your hitting is specific to v3.14-rc1, and was introduced with
the following change to support immutable biovecs:

commit d57a5f7c6605f15f3b5134837e68b448a7cea88e
Author: Kent Overstreet <kmo@daterainc.com>
Date:   Sat Nov 23 17:20:16 2013 -0800

    bio-integrity: Convert to bvec_iter

Given that there is no (easy) way to ascertain what the original value
of bio_integrity->bip_iter.bi_size was post bio_integrity_advance(),
dropping this BUG_ON() probably makes the most sense.

MKP + Jens, care to ACK + pickup..?

--nab

>From 847ab029fe0d5ed9b54a1f1507c46025f6c3f9e0 Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger <nab@linux-iscsi.org>
Date: Thu, 20 Feb 2014 00:52:01 +0000
Subject: [PATCH] bio-integrity: Drop bio_integrity_verify BUG_ON in post
 bip->bip_iter world

Given that bip->bip_iter.bi_size is decremented after bio_advance() ->
bio_integrity_advance() is called, the BUG_ON() in bio_integrity_verify()
ends up tripping in v3.14-rc1 code with the advent of immutable biovecs
in:

commit d57a5f7c6605f15f3b5134837e68b448a7cea88e
Author: Kent Overstreet <kmo@daterainc.com>
Date:   Sat Nov 23 17:20:16 2013 -0800

    bio-integrity: Convert to bvec_iter

Given that there is no easy way to ascertain the original bi_size
value, go ahead and drop this BUG_ON().

Reported-by: Sagi Grimberg <sagig@dev.mellanox.co.il>
Reported-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Kent Overstreet <kmo@daterainc.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 fs/bio-integrity.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
index 0bad24d..47ed160 100644
--- a/fs/bio-integrity.c
+++ b/fs/bio-integrity.c
@@ -476,7 +476,6 @@ static int bio_integrity_verify(struct bio *bio)
 		sector += sectors;
 		prot_buf += sectors * bi->tuple_size;
 		total += sectors * bi->tuple_size;
-		BUG_ON(total > bio->bi_integrity->bip_iter.bi_size);
 
 		kunmap_atomic(kaddr);
 	}
-- 
1.7.10.4

Re: loading scsi_debug with data integrity hits BUG_ON

[Akinobu Mita]

Thanks for the patch.  I have just one nitpicking.

> diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
> index 0bad24d..47ed160 100644
> --- a/fs/bio-integrity.c
> +++ b/fs/bio-integrity.c
> @@ -476,7 +476,6 @@ static int bio_integrity_verify(struct bio *bio)
>                 sector += sectors;
>                 prot_buf += sectors * bi->tuple_size;
>                 total += sectors * bi->tuple_size;
> -               BUG_ON(total > bio->bi_integrity->bip_iter.bi_size);

The variable 'total' is only used for the BUG_ON.  So it can be removed.

Re: loading scsi_debug with data integrity hits BUG_ON

[Nicholas A. Bellinger]

On Thu, 2014-02-20 at 22:44 +0900, Akinobu Mita wrote:
> Thanks for the patch.  I have just one nitpicking.
> 
> > diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
> > index 0bad24d..47ed160 100644
> > --- a/fs/bio-integrity.c
> > +++ b/fs/bio-integrity.c
> > @@ -476,7 +476,6 @@ static int bio_integrity_verify(struct bio *bio)
> >                 sector += sectors;
> >                 prot_buf += sectors * bi->tuple_size;
> >                 total += sectors * bi->tuple_size;
> > -               BUG_ON(total > bio->bi_integrity->bip_iter.bi_size);
> 
> The variable 'total' is only used for the BUG_ON.  So it can be removed.

Duh, yes of course..

MKP + Jens, any concerns wrt dropping this BUG_ON for v3.14-rc code..?

--nab

Re: loading scsi_debug with data integrity hits BUG_ON

[Martin K. Petersen]

>>>>> "nab" == Nicholas A Bellinger <nab@linux-iscsi.org> writes:

nab> Given that there is no (easy) way to ascertain what the original
nab> value of bio_integrity->bip_iter.bi_size was post
nab> bio_integrity_advance(), dropping this BUG_ON() probably makes the
nab> most sense.

nab> MKP + Jens, care to ACK + pickup..?

Please remove total as suggested by Akinobu.

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: loading scsi_debug with data integrity hits BUG_ON

[Nicholas A. Bellinger]

On Thu, 2014-02-20 at 14:16 -0500, Martin K. Petersen wrote:
> >>>>> "nab" == Nicholas A Bellinger <nab@linux-iscsi.org> writes:
> 
> nab> Given that there is no (easy) way to ascertain what the original
> nab> value of bio_integrity->bip_iter.bi_size was post
> nab> bio_integrity_advance(), dropping this BUG_ON() probably makes the
> nab> most sense.
> 
> nab> MKP + Jens, care to ACK + pickup..?
> 
> Please remove total as suggested by Akinobu.
> 
> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
> 

Thanks.  Here's an updated patch for Jen's to pickup.

--nab

>From 6bd5636cd46fe8e11de9bdecc26acac14a494f18 Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger <nab@linux-iscsi.org>
Date: Thu, 20 Feb 2014 00:52:01 +0000
Subject: [PATCH] bio-integrity: Drop bio_integrity_verify BUG_ON in post
 bip->bip_iter world

Given that bip->bip_iter.bi_size is decremented after bio_advance() ->
bio_integrity_advance() is called, the BUG_ON() in bio_integrity_verify()
ends up tripping in v3.14-rc1 code with the advent of immutable biovecs
in:

commit d57a5f7c6605f15f3b5134837e68b448a7cea88e
Author: Kent Overstreet <kmo@daterainc.com>
Date:   Sat Nov 23 17:20:16 2013 -0800

    bio-integrity: Convert to bvec_iter

Given that there is no easy way to ascertain the original bi_size
value, go ahead and drop this BUG_ON().

Reported-by: Sagi Grimberg <sagig@dev.mellanox.co.il>
Reported-by: Akinobu Mita <akinobu.mita@gmail.com>
Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Kent Overstreet <kmo@daterainc.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 fs/bio-integrity.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
index 0bad24d..76e0116 100644
--- a/fs/bio-integrity.c
+++ b/fs/bio-integrity.c
@@ -449,11 +449,10 @@ static int bio_integrity_verify(struct bio *bio)
 	struct blk_integrity_exchg bix;
 	struct bio_vec *bv;
 	sector_t sector = bio->bi_integrity->bip_iter.bi_sector;
-	unsigned int sectors, total, ret;
+	unsigned int sectors, ret = 0;
 	void *prot_buf = bio->bi_integrity->bip_buf;
 	int i;
 
-	ret = total = 0;
 	bix.disk_name = bio->bi_bdev->bd_disk->disk_name;
 	bix.sector_size = bi->sector_size;
 
@@ -475,8 +474,6 @@ static int bio_integrity_verify(struct bio *bio)
 		sectors = bv->bv_len / bi->sector_size;
 		sector += sectors;
 		prot_buf += sectors * bi->tuple_size;
-		total += sectors * bi->tuple_size;
-		BUG_ON(total > bio->bi_integrity->bip_iter.bi_size);
 
 		kunmap_atomic(kaddr);
 	}
-- 
1.7.10.4

Re: loading scsi_debug with data integrity hits BUG_ON

[Jens Axboe]

On Thu, Feb 20 2014, Nicholas A. Bellinger wrote:
> On Thu, 2014-02-20 at 14:16 -0500, Martin K. Petersen wrote:
> > >>>>> "nab" == Nicholas A Bellinger <nab@linux-iscsi.org> writes:
> > 
> > nab> Given that there is no (easy) way to ascertain what the original
> > nab> value of bio_integrity->bip_iter.bi_size was post
> > nab> bio_integrity_advance(), dropping this BUG_ON() probably makes the
> > nab> most sense.
> > 
> > nab> MKP + Jens, care to ACK + pickup..?
> > 
> > Please remove total as suggested by Akinobu.
> > 
> > Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
> > 
> 
> Thanks.  Here's an updated patch for Jen's to pickup.

Got it, thanks!

-- 
Jens Axboe

Re: loading scsi_debug with data integrity hits BUG_ON

[Muthu Kumar]

NAB,

On Thu, Feb 20, 2014 at 11:28 AM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
> On Thu, 2014-02-20 at 14:16 -0500, Martin K. Petersen wrote:
>> >>>>> "nab" == Nicholas A Bellinger <nab@linux-iscsi.org> writes:
>>
>> nab> Given that there is no (easy) way to ascertain what the original
>> nab> value of bio_integrity->bip_iter.bi_size was post
>> nab> bio_integrity_advance(), dropping this BUG_ON() probably makes the
>> nab> most sense.
>>
>> nab> MKP + Jens, care to ACK + pickup..?
>>
>> Please remove total as suggested by Akinobu.
>>
>> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
>>
>
> Thanks.  Here's an updated patch for Jen's to pickup.
>
> --nab
>
> From 6bd5636cd46fe8e11de9bdecc26acac14a494f18 Mon Sep 17 00:00:00 2001
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> Date: Thu, 20 Feb 2014 00:52:01 +0000
> Subject: [PATCH] bio-integrity: Drop bio_integrity_verify BUG_ON in post
>  bip->bip_iter world
>
> Given that bip->bip_iter.bi_size is decremented after bio_advance() ->
> bio_integrity_advance() is called, the BUG_ON() in bio_integrity_verify()
> ends up tripping in v3.14-rc1 code with the advent of immutable biovecs
> in:
>
> commit d57a5f7c6605f15f3b5134837e68b448a7cea88e
> Author: Kent Overstreet <kmo@daterainc.com>
> Date:   Sat Nov 23 17:20:16 2013 -0800
>
>     bio-integrity: Convert to bvec_iter
>
> Given that there is no easy way to ascertain the original bi_size
> value, go ahead and drop this BUG_ON().
>
> Reported-by: Sagi Grimberg <sagig@dev.mellanox.co.il>
> Reported-by: Akinobu Mita <akinobu.mita@gmail.com>
> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Kent Overstreet <kmo@daterainc.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>  fs/bio-integrity.c |    5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
> index 0bad24d..76e0116 100644
> --- a/fs/bio-integrity.c
> +++ b/fs/bio-integrity.c
> @@ -449,11 +449,10 @@ static int bio_integrity_verify(struct bio *bio)
>         struct blk_integrity_exchg bix;
>         struct bio_vec *bv;
>         sector_t sector = bio->bi_integrity->bip_iter.bi_sector;
> -       unsigned int sectors, total, ret;
> +       unsigned int sectors, ret = 0;
>         void *prot_buf = bio->bi_integrity->bip_buf;
>         int i;
>
> -       ret = total = 0;


Here you removed "ret = 0" as well. So if bio_for_each_segment_all()
is not entered, then we return junk from bio_integrity_verify().

Regards,
Muthu


>         bix.disk_name = bio->bi_bdev->bd_disk->disk_name;
>         bix.sector_size = bi->sector_size;
>
> @@ -475,8 +474,6 @@ static int bio_integrity_verify(struct bio *bio)
>                 sectors = bv->bv_len / bi->sector_size;
>                 sector += sectors;
>                 prot_buf += sectors * bi->tuple_size;
> -               total += sectors * bi->tuple_size;
> -               BUG_ON(total > bio->bi_integrity->bip_iter.bi_size);
>
>                 kunmap_atomic(kaddr);
>         }
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: loading scsi_debug with data integrity hits BUG_ON

[Jens Axboe]

On 2014-02-24 14:15, Muthu Kumar wrote:
> NAB,
>
> On Thu, Feb 20, 2014 at 11:28 AM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
>> On Thu, 2014-02-20 at 14:16 -0500, Martin K. Petersen wrote:
>>>>>>>> "nab" == Nicholas A Bellinger <nab@linux-iscsi.org> writes:
>>>
>>> nab> Given that there is no (easy) way to ascertain what the original
>>> nab> value of bio_integrity->bip_iter.bi_size was post
>>> nab> bio_integrity_advance(), dropping this BUG_ON() probably makes the
>>> nab> most sense.
>>>
>>> nab> MKP + Jens, care to ACK + pickup..?
>>>
>>> Please remove total as suggested by Akinobu.
>>>
>>> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
>>>
>>
>> Thanks.  Here's an updated patch for Jen's to pickup.
>>
>> --nab
>>
>>  From 6bd5636cd46fe8e11de9bdecc26acac14a494f18 Mon Sep 17 00:00:00 2001
>> From: Nicholas Bellinger <nab@linux-iscsi.org>
>> Date: Thu, 20 Feb 2014 00:52:01 +0000
>> Subject: [PATCH] bio-integrity: Drop bio_integrity_verify BUG_ON in post
>>   bip->bip_iter world
>>
>> Given that bip->bip_iter.bi_size is decremented after bio_advance() ->
>> bio_integrity_advance() is called, the BUG_ON() in bio_integrity_verify()
>> ends up tripping in v3.14-rc1 code with the advent of immutable biovecs
>> in:
>>
>> commit d57a5f7c6605f15f3b5134837e68b448a7cea88e
>> Author: Kent Overstreet <kmo@daterainc.com>
>> Date:   Sat Nov 23 17:20:16 2013 -0800
>>
>>      bio-integrity: Convert to bvec_iter
>>
>> Given that there is no easy way to ascertain the original bi_size
>> value, go ahead and drop this BUG_ON().
>>
>> Reported-by: Sagi Grimberg <sagig@dev.mellanox.co.il>
>> Reported-by: Akinobu Mita <akinobu.mita@gmail.com>
>> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
>> Cc: Jens Axboe <axboe@kernel.dk>
>> Cc: Kent Overstreet <kmo@daterainc.com>
>> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
>> ---
>>   fs/bio-integrity.c |    5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
>> index 0bad24d..76e0116 100644
>> --- a/fs/bio-integrity.c
>> +++ b/fs/bio-integrity.c
>> @@ -449,11 +449,10 @@ static int bio_integrity_verify(struct bio *bio)
>>          struct blk_integrity_exchg bix;
>>          struct bio_vec *bv;
>>          sector_t sector = bio->bi_integrity->bip_iter.bi_sector;
>> -       unsigned int sectors, total, ret;
>> +       unsigned int sectors, ret = 0;
>>          void *prot_buf = bio->bi_integrity->bip_buf;
>>          int i;
>>
>> -       ret = total = 0;
>
>
> Here you removed "ret = 0" as well. So if bio_for_each_segment_all()
> is not entered, then we return junk from bio_integrity_verify().

The ret = 0 was just moved up to the definition.

-- 
Jens Axboe

Re: loading scsi_debug with data integrity hits BUG_ON

[Muthu Kumar]

yep... silly me ;)

On Mon, Feb 24, 2014 at 2:37 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 2014-02-24 14:15, Muthu Kumar wrote:
>>
>> NAB,
>>
>> On Thu, Feb 20, 2014 at 11:28 AM, Nicholas A. Bellinger
>> <nab@linux-iscsi.org> wrote:
>>>
>>> On Thu, 2014-02-20 at 14:16 -0500, Martin K. Petersen wrote:
>>>>>>>>>
>>>>>>>>> "nab" == Nicholas A Bellinger <nab@linux-iscsi.org> writes:
>>>>
>>>>
>>>> nab> Given that there is no (easy) way to ascertain what the original
>>>> nab> value of bio_integrity->bip_iter.bi_size was post
>>>> nab> bio_integrity_advance(), dropping this BUG_ON() probably makes the
>>>> nab> most sense.
>>>>
>>>> nab> MKP + Jens, care to ACK + pickup..?
>>>>
>>>> Please remove total as suggested by Akinobu.
>>>>
>>>> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
>>>>
>>>
>>> Thanks.  Here's an updated patch for Jen's to pickup.
>>>
>>> --nab
>>>
>>>  From 6bd5636cd46fe8e11de9bdecc26acac14a494f18 Mon Sep 17 00:00:00 2001
>>> From: Nicholas Bellinger <nab@linux-iscsi.org>
>>> Date: Thu, 20 Feb 2014 00:52:01 +0000
>>> Subject: [PATCH] bio-integrity: Drop bio_integrity_verify BUG_ON in post
>>>   bip->bip_iter world
>>>
>>> Given that bip->bip_iter.bi_size is decremented after bio_advance() ->
>>> bio_integrity_advance() is called, the BUG_ON() in bio_integrity_verify()
>>> ends up tripping in v3.14-rc1 code with the advent of immutable biovecs
>>> in:
>>>
>>> commit d57a5f7c6605f15f3b5134837e68b448a7cea88e
>>> Author: Kent Overstreet <kmo@daterainc.com>
>>> Date:   Sat Nov 23 17:20:16 2013 -0800
>>>
>>>      bio-integrity: Convert to bvec_iter
>>>
>>> Given that there is no easy way to ascertain the original bi_size
>>> value, go ahead and drop this BUG_ON().
>>>
>>> Reported-by: Sagi Grimberg <sagig@dev.mellanox.co.il>
>>> Reported-by: Akinobu Mita <akinobu.mita@gmail.com>
>>> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
>>> Cc: Jens Axboe <axboe@kernel.dk>
>>> Cc: Kent Overstreet <kmo@daterainc.com>
>>> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
>>> ---
>>>   fs/bio-integrity.c |    5 +----
>>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
>>> index 0bad24d..76e0116 100644
>>> --- a/fs/bio-integrity.c
>>> +++ b/fs/bio-integrity.c
>>> @@ -449,11 +449,10 @@ static int bio_integrity_verify(struct bio *bio)
>>>          struct blk_integrity_exchg bix;
>>>          struct bio_vec *bv;
>>>          sector_t sector = bio->bi_integrity->bip_iter.bi_sector;
>>> -       unsigned int sectors, total, ret;
>>> +       unsigned int sectors, ret = 0;
>>>          void *prot_buf = bio->bi_integrity->bip_buf;
>>>          int i;
>>>
>>> -       ret = total = 0;
>>
>>
>>
>> Here you removed "ret = 0" as well. So if bio_for_each_segment_all()
>> is not entered, then we return junk from bio_integrity_verify().
>
>
> The ret = 0 was just moved up to the definition.
>
> --
> Jens Axboe
>