[PATCH] dm integrity: reinitialize __bi_remaining when reusing bio

[PATCH] dm integrity: reinitialize __bi_remaining when reusing bio

[Daniel Glöckner]

In cases where dec_in_flight has to requeue the integrity_bio_wait work
to transfer the rest of the data, the __bi_remaining field of the bio
might already have been decremented to zero. Reusing the bio without
reinitializing that counter to 1 can then result in integrity_end_io
being called too early when the BIO_CHAIN flag is set, f.ex. due to
blk_queue_split. In our case this triggered the BUG() in
blk_mq_end_request when the hardware signalled completion of the bio
after integrity_end_io had modified it.

Signed-off-by: Daniel Glöckner <dg@emlix.com>
---
 drivers/md/dm-integrity.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index b225b3e445fa..8cea2978fc24 100644
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -1438,6 +1438,7 @@ static void dec_in_flight(struct dm_integrity_io *dio)
 		if (likely(!bio->bi_status) && unlikely(bio_sectors(bio) != dio->range.n_sectors)) {
 			dio->range.logical_sector += dio->range.n_sectors;
 			bio_advance(bio, dio->range.n_sectors << SECTOR_SHIFT);
+			atomic_set(&bio->__bi_remaining, 1);
 			INIT_WORK(&dio->work, integrity_bio_wait);
 			queue_work(ic->wait_wq, &dio->work);
 			return;
-- 
2.17.1

[PATCH] dm integrity: reinitialize __bi_remaining when reusing bio

[Daniel Glöckner]

In cases where dec_in_flight has to requeue the integrity_bio_wait work
to transfer the rest of the data, the __bi_remaining field of the bio
might already have been decremented to zero. Reusing the bio without
reinitializing that counter to 1 can then result in integrity_end_io
being called too early when the BIO_CHAIN flag is set, f.ex. due to
blk_queue_split. In our case this triggered the BUG() in
blk_mq_end_request when the hardware signalled completion of the bio
after integrity_end_io had modified it.

Signed-off-by: Daniel Glöckner <dg@emlix.com>
---
 drivers/md/dm-integrity.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index b225b3e445fa..8cea2978fc24 100644
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -1438,6 +1438,7 @@ static void dec_in_flight(struct dm_integrity_io *dio)
 		if (likely(!bio->bi_status) && unlikely(bio_sectors(bio) != dio->range.n_sectors)) {
 			dio->range.logical_sector += dio->range.n_sectors;
 			bio_advance(bio, dio->range.n_sectors << SECTOR_SHIFT);
+			atomic_set(&bio->__bi_remaining, 1);
 			INIT_WORK(&dio->work, integrity_bio_wait);
 			queue_work(ic->wait_wq, &dio->work);
 			return;
-- 
2.17.1


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH] dm integrity: reinitialize __bi_remaining when reusing bio

[Christoph Hellwig]

On Tue, Feb 25, 2020 at 06:07:44PM +0100, Daniel Glöckner wrote:
> In cases where dec_in_flight has to requeue the integrity_bio_wait work
> to transfer the rest of the data, the __bi_remaining field of the bio
> might already have been decremented to zero. Reusing the bio without
> reinitializing that counter to 1 can then result in integrity_end_io
> being called too early when the BIO_CHAIN flag is set, f.ex. due to
> blk_queue_split. In our case this triggered the BUG() in
> blk_mq_end_request when the hardware signalled completion of the bio
> after integrity_end_io had modified it.
> 
> Signed-off-by: Daniel Glöckner <dg@emlix.com>

Drivers have no business poking into these internals.  If a bio is
reused the caller needs to use bio_reset instead.

Re: [dm-devel] [PATCH] dm integrity: reinitialize __bi_remaining when reusing bio

[Daniel Glöckner]

Hello Christoph,

Am 02/25/20 um 20:12 schrieb Christoph Hellwig:
> On Tue, Feb 25, 2020 at 06:07:44PM +0100, Daniel Glöckner wrote:
>> In cases where dec_in_flight has to requeue the integrity_bio_wait work
>> to transfer the rest of the data, the __bi_remaining field of the bio
>> might already have been decremented to zero. Reusing the bio without
>> reinitializing that counter to 1 can then result in integrity_end_io
>> being called too early when the BIO_CHAIN flag is set, f.ex. due to
>> blk_queue_split. In our case this triggered the BUG() in
>> blk_mq_end_request when the hardware signalled completion of the bio
>> after integrity_end_io had modified it.
>>
>> Signed-off-by: Daniel Glöckner <dg@emlix.com>
> 
> Drivers have no business poking into these internals.  If a bio is
> reused the caller needs to use bio_reset instead.

bio_reset will reset too many fields. As you can see in the context of
the diff, dm-integrity expects f.ex. the values modified by bio_advance
to stay intact and the transfer should of course use the same disk and
operation.

How about doing the atomic_set in bio_remaining_done (in block/bio.c)
where the BIO_CHAIN flag is cleared once __bi_remaining hits zero?
Or is requeuing a bio without bio_reset really a no-go? In that case a
one-liner won't do...

Best regards,

  Daniel

-- 
Besuchen Sie uns auf der Embedded World 2020 in Nürnberg!
-> Halle 4, Stand 368

Dipl.-Math. Daniel Glöckner, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11,
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke
Ust-IdNr.: DE 205 198 055

emlix - your embedded linux partner

Re: [dm-devel] [PATCH] dm integrity: reinitialize __bi_remaining when reusing bio

[Christoph Hellwig]

On Tue, Feb 25, 2020 at 08:54:07PM +0100, Daniel Glöckner wrote:
> bio_reset will reset too many fields. As you can see in the context of
> the diff, dm-integrity expects f.ex. the values modified by bio_advance
> to stay intact and the transfer should of course use the same disk and
> operation.
> 
> How about doing the atomic_set in bio_remaining_done (in block/bio.c)
> where the BIO_CHAIN flag is cleared once __bi_remaining hits zero?
> Or is requeuing a bio without bio_reset really a no-go? In that case a
> one-liner won't do...

That tends to add a overhead to the fast path for a rather exotic
case.  I'm having a bit of a hard time understanding the dm-integrity
code due to it's annoyingly obsfucated code, but it seems like it
tries to submit a bio again after it came out of a ->end_io handler.
That might have some other problems, but if we only want to paper
over the remaining count a isngle call to bio_inc_remaining might be all
you need.

Re: dm integrity: reinitialize __bi_remaining when reusing bio

[Mike Snitzer]

On Tue, Feb 25 2020 at  5:02pm -0500,
Christoph Hellwig <hch@infradead.org> wrote:

> On Tue, Feb 25, 2020 at 08:54:07PM +0100, Daniel Glöckner wrote:
> > bio_reset will reset too many fields. As you can see in the context of
> > the diff, dm-integrity expects f.ex. the values modified by bio_advance
> > to stay intact and the transfer should of course use the same disk and
> > operation.
> > 
> > How about doing the atomic_set in bio_remaining_done (in block/bio.c)
> > where the BIO_CHAIN flag is cleared once __bi_remaining hits zero?
> > Or is requeuing a bio without bio_reset really a no-go? In that case a
> > one-liner won't do...
>
> That tends to add a overhead to the fast path for a rather exotic
> case.  I'm having a bit of a hard time understanding the dm-integrity
> code due to it's annoyingly obsfucated code, but it seems like it
> tries to submit a bio again after it came out of a ->end_io handler.

Yeah, the dm-integrity code has always been complex and it has only
gotten moreso.  I'm struggling with it too.

This case that Daniel has seen a BUG_ON with is when there is a need to
finish a partially completed bio (as reflected by the per-bio-data's
internal accounting managed by dm-integrity).

> That might have some other problems, but if we only want to paper
> over the remaining count a isngle call to bio_inc_remaining might be all
> you need.

bio_inc_remaining() is really meant to be paired with bio_chain().  They
are pretty tightly coupled these days.  We removed __bio_inc_remaining()
once we weened all (ab)users many releases ago.  Definitely don't want
an open-coded equivalent buried in an obscure dm-integrity usecase.

Could be bio_split() + generic_make_request() recursion is a way
forward -- but that'd likely require some extra work in dm-integrity.
All the additional code in dm_integrity_map_continue() makes me think I
could easily be missing something.

Mikulas, if you could look closer at this issue and see what your best
suggestion would be that'd be appreciated.

Thanks,
Mike