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 <n...@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....@oracle.com>
Cc: Jens Axboe <ax...@kernel.dk>
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Nicholas Bellinger <n...@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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[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 <kover...@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

[Martin K. Petersen]

>>>>> "nab" == Nicholas A Bellinger <n...@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

[Nicholas A. Bellinger]

On Fri, 2014-01-03 at 15:09 -0500, Martin K. Petersen wrote:
> >>>>> "nab" == Nicholas A Bellinger <n...@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

[Martin K. Petersen]

>>>>> "nab" == Nicholas A Bellinger <n...@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

[Nicholas A. Bellinger]

On Fri, 2014-01-17 at 16:58 -0500, Martin K. Petersen wrote:
> >>>>> "nab" == Nicholas A Bellinger <n...@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

[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 <n...@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