Re: [PATCH v4] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[Suraj Sonawane <surajsonawane0215@gmail.com>]

On 15/11/24 21:40, Jens Axboe wrote:
> On 11/15/24 9:07 AM, Suraj Sonawane wrote:
>> On 08/10/24 23:22, SurajSonawane2415 wrote:
>>> Fix the uninitialized symbol 'bio' in the function blk_rq_prep_clone
>>> to resolve the following error:
>>> block/blk-mq.c:3199 blk_rq_prep_clone() error: uninitialized symbol 'bio'.
>>>
>>> Signed-off-by: SurajSonawane2415 <surajsonawane0215@gmail.com>
>>> ---
>>> V1 - Initialize 'bio' to NULL.
>>> V2 - Move bio_put(bio) into the bio_ctr error handling block,
>>> ensuring memory cleanup occurs only when the bio_ctr fail.
>>> V3 - Moved the bio declaration into the loop scope, eliminating
>>> the need to set it to NULL at the end of the loop.
>>> V4 - Adjusted position of arguments of bio_alloc_clone.
>>>
>>>    block/blk-mq.c | 13 ++++++-------
>>>    1 file changed, 6 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>> index 4b2c8e940..89c9a6c4d 100644
>>> --- a/block/blk-mq.c
>>> +++ b/block/blk-mq.c
>>> @@ -3156,19 +3156,21 @@ int blk_rq_prep_clone(struct request *rq, struct request *rq_src,
>>>                  int (*bio_ctr)(struct bio *, struct bio *, void *),
>>>                  void *data)
>>>    {
>>> -    struct bio *bio, *bio_src;
>>> +    struct bio *bio_src;
>>>          if (!bs)
>>>            bs = &fs_bio_set;
>>>          __rq_for_each_bio(bio_src, rq_src) {
>>> -        bio = bio_alloc_clone(rq->q->disk->part0, bio_src, gfp_mask,
>>> -                      bs);
>>> +        struct bio *bio = bio_alloc_clone(rq->q->disk->part0, bio_src,
>>> +                    gfp_mask, bs);
>>>            if (!bio)
>>>                goto free_and_out;
>>>    -        if (bio_ctr && bio_ctr(bio, bio_src, data))
>>> +        if (bio_ctr && bio_ctr(bio, bio_src, data)) {
>>> +            bio_put(bio);
>>>                goto free_and_out;
>>> +        }
>>>              if (rq->bio) {
>>>                rq->biotail->bi_next = bio;
>>> @@ -3176,7 +3178,6 @@ int blk_rq_prep_clone(struct request *rq, struct request *rq_src,
>>>            } else {
>>>                rq->bio = rq->biotail = bio;
>>>            }
>>> -        bio = NULL;
>>>        }
>>>          /* Copy attributes of the original request to the clone request. */
>>> @@ -3196,8 +3197,6 @@ int blk_rq_prep_clone(struct request *rq, struct request *rq_src,
>>>        return 0;
>>>      free_and_out:
>>> -    if (bio)
>>> -        bio_put(bio);
>>>        blk_rq_unprep_clone(rq);
>>>          return -ENOMEM;
>>
>> Hello Jens!
>>
>> I wanted to follow up on this patch I submitted. I have done all the
>> suggested changes till v4. I was wondering if you had a chance to
>> review it and if there are any comments or feedback.
> 
> Sorry missed this one. Is this a legit case of it being used
> uninitialized, or is it just cleaning up the code so that smatch is
> happy? The commit is woefully non-descriptive, unfortunately. So perhaps
> resend this one and improve the commit message.
> 

Apologies for any confusion earlier, and thank you for your attention to 
this. After further analysis, I realize that this change isn't 
necessary, as bio is already set to NULL by bio_alloc_clone on failure, 
preventing any real case of uninitialized use. My initial patch aimed to 
clean up the code and satisfy smatch, ensuring better readability and 
error handling.

I appreciate your feedback and the opportunity to learn from this. I now 
understand that no change is needed here. Thank you for your guidance 
and understanding.

Best regards,
Suraj Sonawane