Re: [PATCH] block: note about cloned bios and bio_for_each_segment_all

[Peter Zijlstra]

On Tue, Jul 18, 2017 at 03:19:00PM -0700, Bart Van Assche wrote:
> Hello Peter,

Sorry for being late, I'm trying to recover from a few weeks of leave
and the inbox is in shambles.

> In a test I ran myself with kernel v4.12-rc1 I also noticed that a
> WARN_ON_ONCE() statement triggered an oops in report_bug() and killed the
> kernel thread of the caller instead of letting the caller continue. What I
> ran into is probably the same oops as in the above call trace. For the test
> I ran myself the disassembly is as follows:
> 
> (gdb) list *(report_bug+0x94)
> 0x812ba024 is in report_bug (lib/bug.c:177).
> 172 return BUG_TRAP_TYPE_WARN;
> 173
> 174 /*
> 175  * Since this is the only store, concurrency 
> is not an issue.
> 176  */
> 177 bug->flags |= BUGFLAG_DONE;
> 178 }
> 179 }
> 180
> 181 if (warning) {
> 
> Could this be related to patch "debug: Add _ONCE() logic to report_bug()"?
> Is this a known issue? If so, is a fix perhaps already available?

Yep..  please see:

  https://marc.info/?l=linux-kernel=150055119925677

Re: [PATCH] block: note about cloned bios and bio_for_each_segment_all

[Bart Van Assche]

On 07/14/17 13:54, Liu Bo wrote:
> On Fri, Jul 14, 2017 at 08:22:31AM -0600, Jens Axboe wrote:
>> On 07/14/2017 07:47 AM, Ming Lei wrote:
 @@ -156,6 +156,9 @@ static inline void *bio_data(struct bio *bio)
  /*
   * drivers should _never_ use the all version - the bio may have been 
 split
   * before it got to the driver and the driver won't own all of it
 + *
 + * Note that cloned bios must not use this as their bi_vcnt may be 
 invalid and
 + * this could lead to silent corruptions.
   */
  #define bio_for_each_segment_all(bvl, bio, i)  \
 for (i = 0, bvl = (bio)->bi_io_vec; i < (bio)->bi_vcnt; i++, bvl++)
 --
 2.13.0

>>>
>>> Maybe we can add a warning here if it is a cloned bio.
>>
>> I think that's a good idea, it's easy for people to get this wrong, and
>> the consequences can be dire. How about something like this?
>>
>> diff --git a/include/linux/bio.h b/include/linux/bio.h
>> index 7b1cf4ba0902..13b6ac6eae29 100644
>> --- a/include/linux/bio.h
>> +++ b/include/linux/bio.h
>> @@ -155,9 +155,12 @@ static inline void *bio_data(struct bio *bio)
>>  
>>  /*
>>   * drivers should _never_ use the all version - the bio may have been split
>> - * before it got to the driver and the driver won't own all of it
>> + * before it got to the driver and the driver won't own all of it.
>> + *
>> + * Don't use this on cloned bio's.
>>   */
>>  #define bio_for_each_segment_all(bvl, bio, i)   
>> \
>> +WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)); \
>>  for (i = 0, bvl = (bio)->bi_io_vec; i < (bio)->bi_vcnt; i++, bvl++)
>>  
>>  static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
>>
> 
> This patch gave me a crash, I'm double checking it..
> 
> thanks,
> -liubo
> 
> [  104.140220] BUG: unable to handle kernel paging request at a0399c1a
> [  104.140675] IP: report_bug+0xc4/0x180
> [  104.140916] PGD 2626067 
> [  104.140917] P4D 2626067 
> [  104.141089] PUD 2627063 
> [  104.141259] PMD 2346aa067 
> [  104.141429] PTE 80023569c161
> [  104.141610] 
> [  104.141926] Oops: 0003 [#1] SMP
> [  104.142137] Dumping ftrace buffer:
> [  104.142366](ftrace buffer empty)
> [  104.142602] Modules linked in: btrfs(OE) xor raid6_pq ppdev parport_pc 
> parport serio_raw nfsd auth_rpcgss nfs_acl lockd grace sunrpc [last unloaded$
>  xor]
> [  104.143493] CPU: 0 PID: 144 Comm: kworker/u16:4 Tainted: GW  OE   
> 4.12.0+ #801
> [  104.144009] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
> 1.9.3-1.fc25 04/01/2014
> [  104.144654] Workqueue: btrfs-rmw btrfs_rmw_helper [btrfs]
> [  104.145009] task: 8802382f8000 task.stack: c9f8
> [  104.145393] RIP: 0010:report_bug+0xc4/0x180
> [  104.145668] RSP: 0018:c9f83ac8 EFLAGS: 00010002
> [  104.146015] RAX: 0001 RBX: a0356cff RCX: 
> 0001
> [  104.146474] RDX: a0399c10 RSI: 0480 RDI: 
> 
> [  104.146931] RBP: c9f83ae8 R08: 0907 R09: 
> 
> [  104.147393] R10: 5170b2af R11: 2b881219 R12: 
> c9f83c38
> [  104.147852] R13: a038a415 R14: 0004 R15: 
> 0006
> [  104.148315] FS:  () GS:88023a60() 
> knlGS:
> [  104.148836] CS:  0010 DS:  ES:  CR0: 80050033
> [  104.149211] CR2: a0399c1a CR3: 0002244fc000 CR4: 
> 06f0
> [  104.149672] Call Trace:
> [  104.149840]  fixup_bug+0x43/0x60
> [  104.150060]  do_trap+0x18a/0x1f0
> [  104.150276]  do_error_trap+0xdf/0x1a0
> [  104.150606]  ? index_rbio_pages+0x14f/0x160 [btrfs]
> [  104.150929]  ? trace_hardirqs_off_thunk+0x1a/0x1c
> [  104.151241]  do_invalid_op+0x20/0x30
> [  104.151478]  invalid_op+0x1e/0x30
> [  104.151787] RIP: 0010:index_rbio_pages+0x14f/0x160 [btrfs]
> [  104.152148] RSP: 0018:c9f83ce8 EFLAGS: 00010002
> [  104.152488] RAX: a0421938 RBX:  RCX: 
> 
> [  104.152947] RDX: 0003 RSI: 0001 RDI: 
> a0440420
> [  104.153410] RBP: c9f83d18 R08:  R09: 
> 
> [  104.153869] R10: 0001 R11: 2b881219 R12: 
> a0421960
> [  104.154330] R13: 0001 R14: 880236e0 R15: 
> 880227718080
> [  104.154882]  ? index_rbio_pages+0xc6/0x160 [btrfs]
> [  104.155286]  rmw_work+0x76/0x310 [btrfs]
> [  104.155633]  btrfs_scrubparity_helper+0xad/0x8e0 [btrfs]
> [  104.156070]  btrfs_rmw_helper+0xe/0x10 [btrfs]
> [  104.156364]  process_one_work+0x34f/0x9c0
> [  104.156631]  worker_thread+0x34a/0x6b0
> [  104.156879]  kthread+0x180/0x190
> [  104.157095]  ? create_worker+0x230/0x230
> [  104.157352]  ? kthread_create_on_node+0x70/0x70
> [  104.157648]  ? kthread_create_on_node+0x70/0x70
> [  104.157944]  ret_from_fork+0x2a/0x40
> [ 

Re: [PATCH] block: note about cloned bios and bio_for_each_segment_all

[David Sterba]

On Fri, Jul 14, 2017 at 08:22:31AM -0600, Jens Axboe wrote:
>  /*
>   * drivers should _never_ use the all version - the bio may have been split
> - * before it got to the driver and the driver won't own all of it
> + * before it got to the driver and the driver won't own all of it.
> + *
> + * Don't use this on cloned bio's.
>   */
>  #define bio_for_each_segment_all(bvl, bio, i)
> \
> + WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)); \
>   for (i = 0, bvl = (bio)->bi_io_vec; i < (bio)->bi_vcnt; i++, bvl++)

This needs a multiline statement protection, otherwise

if (...)
bio_for_each_segment_all(...) {
...
}

would expand to

if (...)
WARN_ON_ONCE(...);

bio_for_each_segment_all(...) { ... }

However "do { ... } while(0)" cannot be used here, so WARN_ON_ONCE could
be moved to the initialization block of for:

#define bio_for_each_segment_all(bvl, bio, i)   \
for (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)),\
 i = 0, bvl = (bio)->bi_io_vec; i < (bio)->bi_vcnt; i++, bvl++)

I haven't found any code using the exact broken pattern. So this does
not explain the crash Liu Bo reports.

Re: [PATCH] block: note about cloned bios and bio_for_each_segment_all

[Jens Axboe]

On 07/14/2017 04:35 PM, Ming Lei wrote:
> On Sat, Jul 15, 2017 at 4:54 AM, Liu Bo  wrote:
>> On Fri, Jul 14, 2017 at 08:22:31AM -0600, Jens Axboe wrote:
>>> On 07/14/2017 07:47 AM, Ming Lei wrote:
> @@ -156,6 +156,9 @@ static inline void *bio_data(struct bio *bio)
>  /*
>   * drivers should _never_ use the all version - the bio may have been 
> split
>   * before it got to the driver and the driver won't own all of it
> + *
> + * Note that cloned bios must not use this as their bi_vcnt may be 
> invalid and
> + * this could lead to silent corruptions.
>   */
>  #define bio_for_each_segment_all(bvl, bio, i)  \
> for (i = 0, bvl = (bio)->bi_io_vec; i < (bio)->bi_vcnt; i++, 
> bvl++)
> --
> 2.13.0
>

 Maybe we can add a warning here if it is a cloned bio.
>>>
>>> I think that's a good idea, it's easy for people to get this wrong, and
>>> the consequences can be dire. How about something like this?
>>>
>>> diff --git a/include/linux/bio.h b/include/linux/bio.h
>>> index 7b1cf4ba0902..13b6ac6eae29 100644
>>> --- a/include/linux/bio.h
>>> +++ b/include/linux/bio.h
>>> @@ -155,9 +155,12 @@ static inline void *bio_data(struct bio *bio)
>>>
>>>  /*
>>>   * drivers should _never_ use the all version - the bio may have been split
>>> - * before it got to the driver and the driver won't own all of it
>>> + * before it got to the driver and the driver won't own all of it.
>>> + *
>>> + * Don't use this on cloned bio's.
>>>   */
>>>  #define bio_for_each_segment_all(bvl, bio, i)  
>>>   \
>>> + WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)); \
>>>   for (i = 0, bvl = (bio)->bi_io_vec; i < (bio)->bi_vcnt; i++, bvl++)
>>>
>>>  static inline void bio_advance_iter(struct bio *bio, struct bvec_iter 
>>> *iter,
>>>
>>
>> This patch gave me a crash, I'm double checking it..
> 
> Hi Liu Bo,
> 
> Looks one extra warning shouldn't have trigger a crash, please double
> check and update
> with us.
> 
> I just start a VM and run a quick test on ext4, btrfs, looks
> everything is fine, and not see
> any warning.

I booted it here too, and it works fine for me. So I agree, the crash seems
to be unrelated.

-- 
Jens Axboe

Re: [PATCH] block: note about cloned bios and bio_for_each_segment_all

[Ming Lei]

On Sat, Jul 15, 2017 at 4:54 AM, Liu Bo  wrote:
> On Fri, Jul 14, 2017 at 08:22:31AM -0600, Jens Axboe wrote:
>> On 07/14/2017 07:47 AM, Ming Lei wrote:
>> >> @@ -156,6 +156,9 @@ static inline void *bio_data(struct bio *bio)
>> >>  /*
>> >>   * drivers should _never_ use the all version - the bio may have been 
>> >> split
>> >>   * before it got to the driver and the driver won't own all of it
>> >> + *
>> >> + * Note that cloned bios must not use this as their bi_vcnt may be 
>> >> invalid and
>> >> + * this could lead to silent corruptions.
>> >>   */
>> >>  #define bio_for_each_segment_all(bvl, bio, i)  \
>> >> for (i = 0, bvl = (bio)->bi_io_vec; i < (bio)->bi_vcnt; i++, 
>> >> bvl++)
>> >> --
>> >> 2.13.0
>> >>
>> >
>> > Maybe we can add a warning here if it is a cloned bio.
>>
>> I think that's a good idea, it's easy for people to get this wrong, and
>> the consequences can be dire. How about something like this?
>>
>> diff --git a/include/linux/bio.h b/include/linux/bio.h
>> index 7b1cf4ba0902..13b6ac6eae29 100644
>> --- a/include/linux/bio.h
>> +++ b/include/linux/bio.h
>> @@ -155,9 +155,12 @@ static inline void *bio_data(struct bio *bio)
>>
>>  /*
>>   * drivers should _never_ use the all version - the bio may have been split
>> - * before it got to the driver and the driver won't own all of it
>> + * before it got to the driver and the driver won't own all of it.
>> + *
>> + * Don't use this on cloned bio's.
>>   */
>>  #define bio_for_each_segment_all(bvl, bio, i)   
>>  \
>> + WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)); \
>>   for (i = 0, bvl = (bio)->bi_io_vec; i < (bio)->bi_vcnt; i++, bvl++)
>>
>>  static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
>>
>
> This patch gave me a crash, I'm double checking it..

Hi Liu Bo,

Looks one extra warning shouldn't have trigger a crash, please double
check and update
with us.

I just start a VM and run a quick test on ext4, btrfs, looks
everything is fine, and not see
any warning.

-- 
Ming Lei

Re: [PATCH] block: note about cloned bios and bio_for_each_segment_all

[Liu Bo]

On Fri, Jul 14, 2017 at 08:22:31AM -0600, Jens Axboe wrote:
> On 07/14/2017 07:47 AM, Ming Lei wrote:
> >> @@ -156,6 +156,9 @@ static inline void *bio_data(struct bio *bio)
> >>  /*
> >>   * drivers should _never_ use the all version - the bio may have been 
> >> split
> >>   * before it got to the driver and the driver won't own all of it
> >> + *
> >> + * Note that cloned bios must not use this as their bi_vcnt may be 
> >> invalid and
> >> + * this could lead to silent corruptions.
> >>   */
> >>  #define bio_for_each_segment_all(bvl, bio, i)  \
> >> for (i = 0, bvl = (bio)->bi_io_vec; i < (bio)->bi_vcnt; i++, bvl++)
> >> --
> >> 2.13.0
> >>
> > 
> > Maybe we can add a warning here if it is a cloned bio.
> 
> I think that's a good idea, it's easy for people to get this wrong, and
> the consequences can be dire. How about something like this?
> 
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 7b1cf4ba0902..13b6ac6eae29 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -155,9 +155,12 @@ static inline void *bio_data(struct bio *bio)
>  
>  /*
>   * drivers should _never_ use the all version - the bio may have been split
> - * before it got to the driver and the driver won't own all of it
> + * before it got to the driver and the driver won't own all of it.
> + *
> + * Don't use this on cloned bio's.
>   */
>  #define bio_for_each_segment_all(bvl, bio, i)
> \
> + WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)); \
>   for (i = 0, bvl = (bio)->bi_io_vec; i < (bio)->bi_vcnt; i++, bvl++)
>  
>  static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
> 

This patch gave me a crash, I'm double checking it..

thanks,
-liubo

[  104.140220] BUG: unable to handle kernel paging request at a0399c1a
[  104.140675] IP: report_bug+0xc4/0x180
[  104.140916] PGD 2626067 
[  104.140917] P4D 2626067 
[  104.141089] PUD 2627063 
[  104.141259] PMD 2346aa067 
[  104.141429] PTE 80023569c161
[  104.141610] 
[  104.141926] Oops: 0003 [#1] SMP
[  104.142137] Dumping ftrace buffer:
[  104.142366](ftrace buffer empty)
[  104.142602] Modules linked in: btrfs(OE) xor raid6_pq ppdev parport_pc 
parport serio_raw nfsd auth_rpcgss nfs_acl lockd grace sunrpc [last unloaded$
 xor]
[  104.143493] CPU: 0 PID: 144 Comm: kworker/u16:4 Tainted: GW  OE   
4.12.0+ #801
[  104.144009] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
1.9.3-1.fc25 04/01/2014
[  104.144654] Workqueue: btrfs-rmw btrfs_rmw_helper [btrfs]
[  104.145009] task: 8802382f8000 task.stack: c9f8
[  104.145393] RIP: 0010:report_bug+0xc4/0x180
[  104.145668] RSP: 0018:c9f83ac8 EFLAGS: 00010002
[  104.146015] RAX: 0001 RBX: a0356cff RCX: 0001
[  104.146474] RDX: a0399c10 RSI: 0480 RDI: 
[  104.146931] RBP: c9f83ae8 R08: 0907 R09: 
[  104.147393] R10: 5170b2af R11: 2b881219 R12: c9f83c38
[  104.147852] R13: a038a415 R14: 0004 R15: 0006
[  104.148315] FS:  () GS:88023a60() 
knlGS:
[  104.148836] CS:  0010 DS:  ES:  CR0: 80050033
[  104.149211] CR2: a0399c1a CR3: 0002244fc000 CR4: 06f0
[  104.149672] Call Trace:
[  104.149840]  fixup_bug+0x43/0x60
[  104.150060]  do_trap+0x18a/0x1f0
[  104.150276]  do_error_trap+0xdf/0x1a0
[  104.150606]  ? index_rbio_pages+0x14f/0x160 [btrfs]
[  104.150929]  ? trace_hardirqs_off_thunk+0x1a/0x1c
[  104.151241]  do_invalid_op+0x20/0x30
[  104.151478]  invalid_op+0x1e/0x30
[  104.151787] RIP: 0010:index_rbio_pages+0x14f/0x160 [btrfs]
[  104.152148] RSP: 0018:c9f83ce8 EFLAGS: 00010002
[  104.152488] RAX: a0421938 RBX:  RCX: 
[  104.152947] RDX: 0003 RSI: 0001 RDI: a0440420
[  104.153410] RBP: c9f83d18 R08:  R09: 
[  104.153869] R10: 0001 R11: 2b881219 R12: a0421960
[  104.154330] R13: 0001 R14: 880236e0 R15: 880227718080
[  104.154882]  ? index_rbio_pages+0xc6/0x160 [btrfs]
[  104.155286]  rmw_work+0x76/0x310 [btrfs]
[  104.155633]  btrfs_scrubparity_helper+0xad/0x8e0 [btrfs]
[  104.156070]  btrfs_rmw_helper+0xe/0x10 [btrfs]
[  104.156364]  process_one_work+0x34f/0x9c0
[  104.156631]  worker_thread+0x34a/0x6b0
[  104.156879]  kthread+0x180/0x190
[  104.157095]  ? create_worker+0x230/0x230
[  104.157352]  ? kthread_create_on_node+0x70/0x70
[  104.157648]  ? kthread_create_on_node+0x70/0x70
[  104.157944]  ret_from_fork+0x2a/0x40
[  104.158183] Code: 44 89 c7 31 c0 66 83 e7 04 0f 95 c0 48 83 c0 02 48 83 04 
c5 18 e6 dc 82 01 66 85 ff b8 01 00 00 00 0f 85 72 ff ff ff 41 83 c8 04 <
66> 44 89 42 0a 48 89 c8 83 e0 01 48 83 c0 02 48 83 04 c5 f0 

Re: [PATCH] block: note about cloned bios and bio_for_each_segment_all

[Jens Axboe]

On 07/14/2017 11:56 AM, Filipe Manana wrote:
> 
> 
> On 07/14/2017 04:03 PM, David Sterba wrote:
>> On Fri, Jul 14, 2017 at 09:47:30PM +0800, Ming Lei wrote:
>>> On Fri, Jul 14, 2017 at 9:40 PM, David Sterba  wrote:
 We've switched to cloned bios in btrfs and hit a nasty bug leading to
 corruptions, when cloned bios are iterated by bio_for_each_segment_all.
>>>
>>> No, you simply can't use bio_for_each_segment_all on cloned bio, and the
>>> reason is obviously.
>>
>> This was not obvious to us, speaking for the btrfs developers trying to
>> make more use of the of the bio API, so we had to find out the hard way.
> 
> Yep, it might be obvious to those familiar with the block layer's
> internals, but for those not so familiar, it's not. There's no mention
> in bio_clone_fast() that the cloned bio's bi_vcnt shouldn't be used,
> and after finding that, one has to check which bio APIs use it and
> which don't. In this specific btrfs issue, it lead to silent write
> corruptions, making it harder to find (as opposed to crashes or other
> immediate failures).

It's hard to circulate info like that, but the WARN_ON() should have
been there from the get-go. I just need someone to test that patch
triggers for the problematic case, then I'd be happy to get it queued
up.

-- 
Jens Axboe

Re: [PATCH] block: note about cloned bios and bio_for_each_segment_all

[Filipe Manana]

On 07/14/2017 04:03 PM, David Sterba wrote:
> On Fri, Jul 14, 2017 at 09:47:30PM +0800, Ming Lei wrote:
>> On Fri, Jul 14, 2017 at 9:40 PM, David Sterba  wrote:
>>> We've switched to cloned bios in btrfs and hit a nasty bug leading to
>>> corruptions, when cloned bios are iterated by bio_for_each_segment_all.
>>
>> No, you simply can't use bio_for_each_segment_all on cloned bio, and the
>> reason is obviously.
> 
> This was not obvious to us, speaking for the btrfs developers trying to
> make more use of the of the bio API, so we had to find out the hard way.

Yep, it might be obvious to those familiar with the block layer's
internals, but for those not so familiar, it's not. There's no mention
in bio_clone_fast() that the cloned bio's bi_vcnt shouldn't be used, and
after finding that, one has to check which bio APIs use it and which
don't. In this specific btrfs issue, it lead to silent write
corruptions, making it harder to find (as opposed to crashes or other
immediate failures).

> 
> The proposed WARN_ON, possibly more sanity checks or documentation would
> help us not to trip over similar problems in the future. I try to take
> great care when dealing with code changing bios on our side so I read
> the headers, and partially the implementation, but still can miss
> something.
> 

Re: [PATCH] block: note about cloned bios and bio_for_each_segment_all

[David Sterba]

On Fri, Jul 14, 2017 at 09:47:30PM +0800, Ming Lei wrote:
> On Fri, Jul 14, 2017 at 9:40 PM, David Sterba  wrote:
> > We've switched to cloned bios in btrfs and hit a nasty bug leading to
> > corruptions, when cloned bios are iterated by bio_for_each_segment_all.
> 
> No, you simply can't use bio_for_each_segment_all on cloned bio, and the
> reason is obviously.

This was not obvious to us, speaking for the btrfs developers trying to
make more use of the of the bio API, so we had to find out the hard way.

The proposed WARN_ON, possibly more sanity checks or documentation would
help us not to trip over similar problems in the future. I try to take
great care when dealing with code changing bios on our side so I read
the headers, and partially the implementation, but still can miss
something.

Re: [PATCH] block: note about cloned bios and bio_for_each_segment_all

[Ming Lei]

On Fri, Jul 14, 2017 at 9:40 PM, David Sterba  wrote:
> We've switched to cloned bios in btrfs and hit a nasty bug leading to
> corruptions, when cloned bios are iterated by bio_for_each_segment_all.

No, you simply can't use bio_for_each_segment_all on cloned bio, and the
reason is obviously.

>
> Report and fix:
> https://patchwork.kernel.org/patch/9838535/
>
> As a matter of precaution, we've added assertions to btrfs code to catch
> the bad usage pattern:
>
> https://patchwork.kernel.org/patch/9839267/
>
> The cloned/bi_vcnt behaviour seems tobe implementation dependent and is
> not documented, so this patch at least warns about this one particular
> case but this might still be insufficient.
>
> CC: linux-block@vger.kernel.org
> Signed-off-by: David Sterba 
> ---
>  include/linux/bio.h | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 7b1cf4ba0902..f1ac84edcf39 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -156,6 +156,9 @@ static inline void *bio_data(struct bio *bio)
>  /*
>   * drivers should _never_ use the all version - the bio may have been split
>   * before it got to the driver and the driver won't own all of it
> + *
> + * Note that cloned bios must not use this as their bi_vcnt may be invalid 
> and
> + * this could lead to silent corruptions.
>   */
>  #define bio_for_each_segment_all(bvl, bio, i)  \
> for (i = 0, bvl = (bio)->bi_io_vec; i < (bio)->bi_vcnt; i++, bvl++)
> --
> 2.13.0
>

Maybe we can add a warning here if it is a cloned bio.


-- 
Ming Lei

[PATCH] block: note about cloned bios and bio_for_each_segment_all

[David Sterba]

We've switched to cloned bios in btrfs and hit a nasty bug leading to
corruptions, when cloned bios are iterated by bio_for_each_segment_all.

Report and fix:
https://patchwork.kernel.org/patch/9838535/

As a matter of precaution, we've added assertions to btrfs code to catch
the bad usage pattern:

https://patchwork.kernel.org/patch/9839267/

The cloned/bi_vcnt behaviour seems tobe implementation dependent and is
not documented, so this patch at least warns about this one particular
case but this might still be insufficient.

CC: linux-block@vger.kernel.org
Signed-off-by: David Sterba 
---
 include/linux/bio.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/bio.h b/include/linux/bio.h
index 7b1cf4ba0902..f1ac84edcf39 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -156,6 +156,9 @@ static inline void *bio_data(struct bio *bio)
 /*
  * drivers should _never_ use the all version - the bio may have been split
  * before it got to the driver and the driver won't own all of it
+ *
+ * Note that cloned bios must not use this as their bi_vcnt may be invalid and
+ * this could lead to silent corruptions.
  */
 #define bio_for_each_segment_all(bvl, bio, i)  \
for (i = 0, bvl = (bio)->bi_io_vec; i < (bio)->bi_vcnt; i++, bvl++)
-- 
2.13.0