Skip to content

Commit dcbb598

Browse files
SurajSonawane2415axboe
authored andcommitted
block: blk-mq: fix uninit-value in blk_rq_prep_clone and refactor
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> Reviewed-by: Christoph Hellwig <hch@lst.de> Link: https://lore.kernel.org/r/20241119164412.37609-1-surajsonawane0215@gmail.com Signed-off-by: Jens Axboe <axboe@kernel.dk>
1 parent cf5a60d commit dcbb598

File tree

1 file changed

+6
-7
lines changed

1 file changed

+6
-7
lines changed

block/blk-mq.c

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3273,27 +3273,28 @@ int blk_rq_prep_clone(struct request *rq, struct request *rq_src,
32733273
int (*bio_ctr)(struct bio *, struct bio *, void *),
32743274
void *data)
32753275
{
3276-
struct bio *bio, *bio_src;
3276+
struct bio *bio_src;
32773277

32783278
if (!bs)
32793279
bs = &fs_bio_set;
32803280

32813281
__rq_for_each_bio(bio_src, rq_src) {
3282-
bio = bio_alloc_clone(rq->q->disk->part0, bio_src, gfp_mask,
3283-
bs);
3282+
struct bio *bio = bio_alloc_clone(rq->q->disk->part0, bio_src,
3283+
gfp_mask, bs);
32843284
if (!bio)
32853285
goto free_and_out;
32863286

3287-
if (bio_ctr && bio_ctr(bio, bio_src, data))
3287+
if (bio_ctr && bio_ctr(bio, bio_src, data)) {
3288+
bio_put(bio);
32883289
goto free_and_out;
3290+
}
32893291

32903292
if (rq->bio) {
32913293
rq->biotail->bi_next = bio;
32923294
rq->biotail = bio;
32933295
} else {
32943296
rq->bio = rq->biotail = bio;
32953297
}
3296-
bio = NULL;
32973298
}
32983299

32993300
/* 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,
33113312
return 0;
33123313

33133314
free_and_out:
3314-
if (bio)
3315-
bio_put(bio);
33163315
blk_rq_unprep_clone(rq);
33173316

33183317
return -ENOMEM;

0 commit comments

Comments
 (0)