Re: [PATCH] block: copy bi_vcnt in __bio_clone_fast

[Jens Axboe <axboe@kernel.dk>]

On 10/09/2014 11:58 AM, Jeff Mahoney wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 10/9/14, 12:13 PM, Ming Lei wrote:
>> On Thu, Oct 9, 2014 at 11:25 PM, Ming Lei <ming.lei@canonical.com>
>> wrote:
>>> On Thu, Oct 9, 2014 at 10:26 PM, Jeff Mahoney <jeffm@suse.com>
>>> wrote:
>>>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>>>>
>>>> On 10/9/14, 9:53 AM, Jeff Moyer wrote:
>>>>> Jeff Mahoney <jeffm@suse.com> writes:
>>>>>
>>>>>> Commit 05f1dd53152173 (block: add queue flag for disabling
>>>>>> SG merging) uses bi_vcnt to assign bio->bi_phys_segments if
>>>>>> sg merging is disabled. When using device mapper on top of
>>>>>> a blk-mq device (virtio_blk in my test), we'd end up
>>>>>> overflowing the scatterlist in __blk_bios_map_sg.
>>>>>>
>>>>>> __bio_clone_fast copies bi_iter and bi_io_vec but not
>>>>>> bi_vcnt, so blk_recount_segments would report
>>>>>> bi_phys_segments as 0. Since rq->nr_phys_segments is 0 as
>>>>>> well, the checks to ensure that we don't exceed the queue's
>>>>>> segment limit end up allowing more bios (and segments) to
>>>>>> attach the a request until we finally map it. That also
>>>>>> means we pass the BUG_ON at the beginning of 
>>>>>> virtio_queue_rq, ultimately causing memory corruption and
>>>>>> a crash.
>>>>>>
>>>>>> If we copy bi_vcnt in __bio_clone_fast, the bios and
>>>>>> requests properly report the number of segments and
>>>>>> everything works as expected.
>>>>>>
>>>>>> Originally reported at 
>>>>>> http://bugzilla.opensuse.org/show_bug.cgi?id=888259
>>>>>
>>>>> Hi, Jeff,
>>>>>
>>>>> Did you manage to reproduce this problem with commit 0738854 
>>>>> (blk-merge: fix blk_recount_segments) applied?  Or perhaps
>>>>> with commit 200612e (dm table: propagate
>>>>> QUEUE_FLAG_NO_SG_MERGE)?
>>>>
>>>> Yep. I was able to reproduce it with 3.17. I did try 0738854
>>>> when I was still using 3.16 since it looked like a good
>>>> candidate. Neither of those patches affect the problem here.
>>>> bio->bi_phys_segments never gets a value set in the fast clone
>>>> case and that translates to req->nr_phys_segments never getting
>>>> properly accumulated. That might not be a problem except that
>>>> the NO_SG_MERGE behavior bypasses the iteration that would come
>>>> up with the correct value. In either case,
>>>
>>> This patch may get incorrect rq->nr_phys_segments because bio
>>> cloning is often used in case of I/O splitting, so could you test
>>> if the attached patch fixes your problem?
> 
> Ah. Right.
> 
>> Please ignore last patch and test the attached patch since we still
>> should use no sg merge to recalculate req's segments for cloned
>> bio.
> 
> Yep, this fix works as expected.

Thanks Ming, I've queued it up (and added Jeff's tested-by).

-- 
Jens Axboe