[PATCH v5] block: blk-mq: fix uninit-value in blk_rq_prep_clone and refactor

[PATCH v5] block: blk-mq: fix uninit-value in blk_rq_prep_clone and refactor

[Suraj Sonawane]

Fix an issue detected by the `smatch` tool:

block/blk-mq.c:3314 blk_rq_prep_clone() error: uninitialized
symbol 'bio'.

This patch refactors `blk_rq_prep_clone()` to improve code
readability and ensure safety by addressing potential misuse of
the `bio` variable:

- Move the bio_put(bio); call to the bio_ctr error handling block,
  which is the only place where it can be triggered.
- Move the bio variable into the __rq_for_each_bio loop scope.
  This change removes the need to set bio to NULL at the loop's 
  end.

discussion on why bio remains uninitialized:
https://lore.kernel.org/lkml/20241004141037.43277-1-surajsonawane0215@gmail.com

Summary of above discussion:
- I pointed out that `bio` can remain uninitialized if the 
  allocation with `bio_alloc_clone` fails.
- Keith Busch explained that `bio` is initialized to `NULL` when 
  `bio_alloc_clone()` fails, preventing uninitialized usage.
- John Garry questioned whether `rq_src->bio` being `NULL` could 
  leave `bio` uninitialized. Keith clarified that in such cases, 
  `bio` is not referenced, so it does not need initialization.
- Christoph Hellwig recommended code improvements:
 - move the bio_put to the bio_ctr error handling, which is the only
   case where it can happen
 - move the bio variable into the __rq_for_each_bio scope, which
   also removed the need to zero it at the end of the loop

These changes enhance code clarity, address static analysis tool
warnings, and make the function more maintainable.

thread of previous version patch discussion:
https://lore.kernel.org/lkml/20241004100842.9052-1-surajsonawane0215@gmail.com

Signed-off-by: Suraj Sonawane <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 - Move the bio declaration into the loop scope, eliminating
     the need to set it to NULL at the end of the loop.
V4 - Adjust position of arguments of bio_alloc_clone.
V5 - Add commit log properly and change sign-off name from
     SurajSonawane2415 to Suraj Sonawane (used real name).

 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 270cfd9fc..abdf44736 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -3273,19 +3273,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;
@@ -3293,7 +3295,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. */
@@ -3311,8 +3312,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;
-- 
2.34.1

Re: [PATCH v5] block: blk-mq: fix uninit-value in blk_rq_prep_clone and refactor

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v5] block: blk-mq: fix uninit-value in blk_rq_prep_clone and refactor

[Suraj Sonawane]

On 19/11/24 22:20, Christoph Hellwig wrote:
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
Thank you for reviewing the patch.

Re: [PATCH v5] block: blk-mq: fix uninit-value in blk_rq_prep_clone and refactor

[Jens Axboe]

On Tue, 19 Nov 2024 22:14:12 +0530, Suraj Sonawane wrote:
> Fix an issue detected by the `smatch` tool:
> 
> block/blk-mq.c:3314 blk_rq_prep_clone() error: uninitialized
> symbol 'bio'.
> 
> This patch refactors `blk_rq_prep_clone()` to improve code
> readability and ensure safety by addressing potential misuse of
> the `bio` variable:
> 
> [...]

Applied, thanks!

[1/1] block: blk-mq: fix uninit-value in blk_rq_prep_clone and refactor
      commit: dcbb598e689e30d4636201d35582e167d1b8dfa4

Best regards,
-- 
Jens Axboe

Re: [PATCH v5] block: blk-mq: fix uninit-value in blk_rq_prep_clone and refactor

[Suraj Sonawane]

On 20/11/24 07:36, Jens Axboe wrote:
> 
> On Tue, 19 Nov 2024 22:14:12 +0530, Suraj Sonawane wrote:
>> Fix an issue detected by the `smatch` tool:
>>
>> block/blk-mq.c:3314 blk_rq_prep_clone() error: uninitialized
>> symbol 'bio'.
>>
>> This patch refactors `blk_rq_prep_clone()` to improve code
>> readability and ensure safety by addressing potential misuse of
>> the `bio` variable:
>>
>> [...]
> 
> Applied, thanks!
> 
> [1/1] block: blk-mq: fix uninit-value in blk_rq_prep_clone and refactor
>        commit: dcbb598e689e30d4636201d35582e167d1b8dfa4
> 
> Best regards,

Thank you for applying the patch and for your time reviewing it.