bio_integrity_verify() bug causing READ verify to be silently skipped

bio_integrity_verify() bug causing READ verify to be silently skipped

[Nicholas A. Bellinger]

Hi Martin & Co,

So after playing with the mainline DIF client against an initial WIP
target DIF support patch, I've started hitting a bug in
bio_integrity_verify() that causes READ verify logic to be silently
skipped for both WIP target and existing scsi_debug DIF code.

The issue is with the scsi_end_request() -> blk_end_request() completion
path, where eventually blk_update_request() -> req_bio_endio() ->
bio_advance() is called to increment bio->bi_idx to a non-zero value.

Given that bio_integrity_verify() is using bio_for_each_segment(), the
loop starts from the updated bio->bi_idx, and not a zero value, which
ends up skipping individual bio segment calls to bi->verify_fn().

The following patch changes bio_integrity_verify() to use
bio_for_each_segment_all() instead of bio_for_each_segment() to ensure
that the segment walk always starts from zero, regardless of the current
bio->bi_idx value after bio_advance().

Note this bug has been observed with v3.13-rc3 code, and I haven't yet
looked back to figure out when this bug was first introduced..  Any
ideas..?

Interestingly enough, the scsi-mq alpha code does not suffer from this
bug, as blk_end_request() is never called from scsi_mq_end_request() ->
blk_mq_end_io() completion path code.

Thank you,

--nab

>From 32242942edca095e8dd126cb1408f2842340773e Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger <nab@linux-iscsi.org>
Date: Tue, 24 Dec 2013 04:00:24 +0000
Subject: [PATCH] bio-integrity: Fix bio_integrity_verify segment start bug

This patch addresses a bug in bio_integrity_verify() code that has
been causing DIF READ verify operations to be silently skipped.

The issue is that bio->bi_idx will have been incremented within
bio_advance() code in the normal blk_update_request() ->
req_bio_endio() completion path, and bio_integrity_verify() is
using bio_for_each_segment() which starts the bio segment walk
at the current bio->bi_idx.

So instead use bio_for_each_segment_all() to always start the bio
segment walk from zero, regardless of the current bio->bi_idx
value after bio_advance() has been called.

Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 fs/bio-integrity.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
index fc60b31..f1d5cde 100644
--- a/fs/bio-integrity.c
+++ b/fs/bio-integrity.c
@@ -450,7 +450,7 @@ static int bio_integrity_verify(struct bio *bio)
 	bix.disk_name = bio->bi_bdev->bd_disk->disk_name;
 	bix.sector_size = bi->sector_size;
 
-	bio_for_each_segment(bv, bio, i) {
+	bio_for_each_segment_all(bv, bio, i) {
 		void *kaddr = kmap_atomic(bv->bv_page);
 		bix.data_buf = kaddr + bv->bv_offset;
 		bix.data_size = bv->bv_len;
-- 
1.7.10.4

Re: bio_integrity_verify() bug causing READ verify to be silently skipped

[Nicholas A. Bellinger]

On Mon, 2013-12-23 at 20:20 -0800, Nicholas A. Bellinger wrote:
> Hi Martin & Co,
> 
> So after playing with the mainline DIF client against an initial WIP
> target DIF support patch, I've started hitting a bug in
> bio_integrity_verify() that causes READ verify logic to be silently
> skipped for both WIP target and existing scsi_debug DIF code.
> 
> The issue is with the scsi_end_request() -> blk_end_request() completion
> path, where eventually blk_update_request() -> req_bio_endio() ->
> bio_advance() is called to increment bio->bi_idx to a non-zero value.
> 
> Given that bio_integrity_verify() is using bio_for_each_segment(), the
> loop starts from the updated bio->bi_idx, and not a zero value, which
> ends up skipping individual bio segment calls to bi->verify_fn().
> 
> The following patch changes bio_integrity_verify() to use
> bio_for_each_segment_all() instead of bio_for_each_segment() to ensure
> that the segment walk always starts from zero, regardless of the current
> bio->bi_idx value after bio_advance().
> 
> Note this bug has been observed with v3.13-rc3 code, and I haven't yet
> looked back to figure out when this bug was first introduced..  Any
> ideas..?

So the commit that introduced bio_advance() into req_bio_endio() code
was:

commit f79ea4161434b31e351658283b24e92c3e570142
Author: Kent Overstreet <koverstreet@google.com>
Date:   Thu Sep 20 16:38:30 2012 -0700

    block: Refactor blk_update_request()

So AFAICT this has been broken since v3.10..

(CC'ing kmo)

--nab

> 
> Interestingly enough, the scsi-mq alpha code does not suffer from this
> bug, as blk_end_request() is never called from scsi_mq_end_request() ->
> blk_mq_end_io() completion path code.
> 
> Thank you,
> 
> --nab
> 
> From 32242942edca095e8dd126cb1408f2842340773e Mon Sep 17 00:00:00 2001
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> Date: Tue, 24 Dec 2013 04:00:24 +0000
> Subject: [PATCH] bio-integrity: Fix bio_integrity_verify segment start bug
> 
> This patch addresses a bug in bio_integrity_verify() code that has
> been causing DIF READ verify operations to be silently skipped.
> 
> The issue is that bio->bi_idx will have been incremented within
> bio_advance() code in the normal blk_update_request() ->
> req_bio_endio() completion path, and bio_integrity_verify() is
> using bio_for_each_segment() which starts the bio segment walk
> at the current bio->bi_idx.
> 
> So instead use bio_for_each_segment_all() to always start the bio
> segment walk from zero, regardless of the current bio->bi_idx
> value after bio_advance() has been called.
> 
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>  fs/bio-integrity.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
> index fc60b31..f1d5cde 100644
> --- a/fs/bio-integrity.c
> +++ b/fs/bio-integrity.c
> @@ -450,7 +450,7 @@ static int bio_integrity_verify(struct bio *bio)
>  	bix.disk_name = bio->bi_bdev->bd_disk->disk_name;
>  	bix.sector_size = bi->sector_size;
>  
> -	bio_for_each_segment(bv, bio, i) {
> +	bio_for_each_segment_all(bv, bio, i) {
>  		void *kaddr = kmap_atomic(bv->bv_page);
>  		bix.data_buf = kaddr + bv->bv_offset;
>  		bix.data_size = bv->bv_len;

Re: bio_integrity_verify() bug causing READ verify to be silently skipped

[Martin K. Petersen]

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

nab> Given that bio_integrity_verify() is using bio_for_each_segment(),
nab> the loop starts from the updated bio->bi_idx, and not a zero value,
nab> which ends up skipping individual bio segment calls to
nab> bi->verify_fn().

That's botched. Verify is meant to be called with the completed bytes
before the index is tampered with.


nab> The following patch changes bio_integrity_verify() to use
nab> bio_for_each_segment_all() instead of bio_for_each_segment() to
nab> ensure that the segment walk always starts from zero, regardless of
nab> the current bio->bi_idx value after bio_advance().

That breaks partial completion, though. I'll take a look at Kent's
changes...

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: bio_integrity_verify() bug causing READ verify to be silently skipped

[Nicholas A. Bellinger]

On Fri, 2014-01-03 at 15:09 -0500, Martin K. Petersen wrote:
> >>>>> "nab" == Nicholas A Bellinger <nab@linux-iscsi.org> writes:
> 
> nab> Given that bio_integrity_verify() is using bio_for_each_segment(),
> nab> the loop starts from the updated bio->bi_idx, and not a zero value,
> nab> which ends up skipping individual bio segment calls to
> nab> bi->verify_fn().
> 
> That's botched. Verify is meant to be called with the completed bytes
> before the index is tampered with.
> 
> 
> nab> The following patch changes bio_integrity_verify() to use
> nab> bio_for_each_segment_all() instead of bio_for_each_segment() to
> nab> ensure that the segment walk always starts from zero, regardless of
> nab> the current bio->bi_idx value after bio_advance().
> 
> That breaks partial completion, though. I'll take a look at Kent's
> changes...
> 

Ping..?  Any updates on a proper bugfix for this..?

--nab

Re: bio_integrity_verify() bug causing READ verify to be silently skipped

[Martin K. Petersen]

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

>> That breaks partial completion, though. I'll take a look at Kent's
>> changes...

nab> Ping..?  Any updates on a proper bugfix for this..?

I did put your patch in my queue and have been working on a fix for the
partial completion case. The latter requires a bit of massaging that
interferes with other pending changes.

Given that your patch does address a valid issue I'm OK with Jens
putting it in as is. I'll build upon it for my changes.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: bio_integrity_verify() bug causing READ verify to be silently skipped

[Nicholas A. Bellinger]

On Fri, 2014-01-17 at 16:58 -0500, Martin K. Petersen wrote:
> >>>>> "nab" == Nicholas A Bellinger <nab@linux-iscsi.org> writes:
> 
> >> That breaks partial completion, though. I'll take a look at Kent's
> >> changes...
> 
> nab> Ping..?  Any updates on a proper bugfix for this..?
> 
> I did put your patch in my queue and have been working on a fix for the
> partial completion case. The latter requires a bit of massaging that
> interferes with other pending changes.
> 
> Given that your patch does address a valid issue I'm OK with Jens
> putting it in as is. I'll build upon it for my changes.
> 

<nod>

Jens, are you going to pick this one up, or shall I include it in the
upcoming target-pending/for-next pull request instead..?

Either way, it needs a CC' to stable for >= v3.10.y.

Thanks,

--nab

Re: bio_integrity_verify() bug causing READ verify to be silently skipped

[Jens Axboe]

On Tue, Jan 21 2014, Nicholas A. Bellinger wrote:
> On Fri, 2014-01-17 at 16:58 -0500, Martin K. Petersen wrote:
> > >>>>> "nab" == Nicholas A Bellinger <nab@linux-iscsi.org> writes:
> > 
> > >> That breaks partial completion, though. I'll take a look at Kent's
> > >> changes...
> > 
> > nab> Ping..?  Any updates on a proper bugfix for this..?
> > 
> > I did put your patch in my queue and have been working on a fix for the
> > partial completion case. The latter requires a bit of massaging that
> > interferes with other pending changes.
> > 
> > Given that your patch does address a valid issue I'm OK with Jens
> > putting it in as is. I'll build upon it for my changes.
> > 
> 
> <nod>
> 
> Jens, are you going to pick this one up, or shall I include it in the
> upcoming target-pending/for-next pull request instead..?
> 
> Either way, it needs a CC' to stable for >= v3.10.y.

I'll queue it up, thanks.

-- 
Jens Axboe