| Message ID | 20241004100842.9052-1-surajsonawane0215@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone | expand |
On Fri, Oct 04, 2024 at 03:38:42PM +0530, 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'. > > Initialize 'bio' to NULL to prevent undefined behavior from uninitialized > access and safe cleanup in case of failure. Please explain how bio could be used uninitialized in this function.
Explaination of how bio could be used uninitialized in this function: In the function blk_rq_prep_clone, the variable bio is declared but can remain uninitialized if the allocation with bio_alloc_clone fails. This can lead to undefined behavior when the function attempts to free bio in the error handling section using bio_put(bio). By initializing bio to NULL at declaration, we ensure that the cleanup code will only interact with bio if it has been successfully allocated. Best regards, Suraj Sonawane
On 10/4/24 16:10, SurajSonawane2415 wrote: > Explaination of how bio could be used uninitialized in this function: > > In the function blk_rq_prep_clone, the variable bio is declared but can remain uninitialized > if the allocation with bio_alloc_clone fails. This can lead to undefined behavior when the > function attempts to free bio in the error handling section using bio_put(bio). > By initializing bio to NULL at declaration, we ensure that the cleanup code will only > interact with bio if it has been successfully allocated. > Hate to say it, but it looks you are correct. Care to send a patch? Cheers, Hannes
On 04/10/2024 15:10, SurajSonawane2415 wrote: > Explaination of how bio could be used uninitialized in this function: > > In the function blk_rq_prep_clone, the variable bio is declared but can remain uninitialized > if the allocation with bio_alloc_clone fails. This can lead to undefined behavior when the > function attempts to free bio in the error handling section using bio_put(bio). > By initializing bio to NULL at declaration, we ensure that the cleanup code will only > interact with bio if it has been successfully allocated. > > What about if rq_src->bio is NULL for blk_rq_prep_clone() -> __rq_for_each_bio(,rq_src): #define __rq_for_each_bio(_bio, rq) \ if ((rq->bio)) \ for (_bio = (rq)->bio; _bio; _bio = _bio->bi_next) Then I don't think bio it get init'ed. Whether this is possible (rq_src->bio is NULL) is another question.
On Fri, Oct 04, 2024 at 07:40:37PM +0530, SurajSonawane2415 wrote: > In the function blk_rq_prep_clone, the variable bio is declared but can remain uninitialized > if the allocation with bio_alloc_clone fails. This can lead to undefined behavior when the > function attempts to free bio in the error handling section using bio_put(bio). > By initializing bio to NULL at declaration, we ensure that the cleanup code will only > interact with bio if it has been successfully allocated. I don't think your explanation makes sense. The line where bio_alloc_clone happens: bio = bio_alloc_clone(rq->q->disk->part0, bio_src, gfp_mask, bs); If it fails, then bio is initialized to NULL.
On Fri, Oct 04, 2024 at 03:33:00PM +0100, John Garry wrote: > On 04/10/2024 15:10, SurajSonawane2415 wrote: > > Explaination of how bio could be used uninitialized in this function: > > > > In the function blk_rq_prep_clone, the variable bio is declared but can remain uninitialized > > if the allocation with bio_alloc_clone fails. This can lead to undefined behavior when the > > function attempts to free bio in the error handling section using bio_put(bio). > > By initializing bio to NULL at declaration, we ensure that the cleanup code will only > > interact with bio if it has been successfully allocated. > > > > > > What about if rq_src->bio is NULL for blk_rq_prep_clone() -> > __rq_for_each_bio(,rq_src): > > #define __rq_for_each_bio(_bio, rq) \ > if ((rq->bio)) \ > for (_bio = (rq)->bio; _bio; _bio = _bio->bi_next) > > Then I don't think bio it get init'ed. Whether this is possible (rq_src->bio > is NULL) is another question. If the source request doesn't have a bio, then the onstack 'bio' is never referenced, so should be okay if it's not initialized in that case.
On 04/10/24 20:03, John Garry wrote: > On 04/10/2024 15:10, SurajSonawane2415 wrote: >> Explaination of how bio could be used uninitialized in this function: >> >> In the function blk_rq_prep_clone, the variable bio is declared but >> can remain uninitialized >> if the allocation with bio_alloc_clone fails. This can lead to >> undefined behavior when the >> function attempts to free bio in the error handling section using >> bio_put(bio). >> By initializing bio to NULL at declaration, we ensure that the cleanup >> code will only >> interact with bio if it has been successfully allocated. >> >> > > What about if rq_src->bio is NULL for blk_rq_prep_clone() -> > __rq_for_each_bio(,rq_src): > > #define __rq_for_each_bio(_bio, rq) \ > if ((rq->bio)) \ > for (_bio = (rq)->bio; _bio; _bio = _bio->bi_next) > > Then I don't think bio it get init'ed. Whether this is possible > (rq_src->bio is NULL) is another question. Hi Keith, You're right to bring this up. If rq_src->bio is NULL, the __rq_for_each_bio macro will skip the loop, meaning the bio variable won't be used at all. So, even if bio isn’t initialized, it won't cause any issues in that case. Thanks for pointing that out. Best regards, Suraj
On 04/10/24 20:09, Keith Busch wrote: > On Fri, Oct 04, 2024 at 07:40:37PM +0530, SurajSonawane2415 wrote: >> In the function blk_rq_prep_clone, the variable bio is declared but can remain uninitialized >> if the allocation with bio_alloc_clone fails. This can lead to undefined behavior when the >> function attempts to free bio in the error handling section using bio_put(bio). >> By initializing bio to NULL at declaration, we ensure that the cleanup code will only >> interact with bio if it has been successfully allocated. > > I don't think your explanation makes sense. The line where > bio_alloc_clone happens: > > bio = bio_alloc_clone(rq->q->disk->part0, bio_src, gfp_mask, bs); > > If it fails, then bio is initialized to NULL. You're correct, bio_alloc_clone returns NULL if it fails, so there’s no uninitialized bio after that. My initial explanation wasn’t fully accurate, but initializing bio to NULL is just a safety measure for any unexpected issues later on. Or i am just trying to solve this issue by smatch tool: block/blk-mq.c:3199 blk_rq_prep_clone() error: uninitialized symbol 'bio'. Thanks for the clarification. Best regards, Suraj
On 06/10/24 12:28, Suraj Sonawane wrote: > On 04/10/24 20:03, John Garry wrote: >> On 04/10/2024 15:10, SurajSonawane2415 wrote: >>> Explaination of how bio could be used uninitialized in this function: >>> >>> In the function blk_rq_prep_clone, the variable bio is declared but >>> can remain uninitialized >>> if the allocation with bio_alloc_clone fails. This can lead to >>> undefined behavior when the >>> function attempts to free bio in the error handling section using >>> bio_put(bio). >>> By initializing bio to NULL at declaration, we ensure that the >>> cleanup code will only >>> interact with bio if it has been successfully allocated. >>> >>> >> >> What about if rq_src->bio is NULL for blk_rq_prep_clone() -> >> __rq_for_each_bio(,rq_src): >> >> #define __rq_for_each_bio(_bio, rq) \ >> if ((rq->bio)) \ >> for (_bio = (rq)->bio; _bio; _bio = _bio->bi_next) >> >> Then I don't think bio it get init'ed. Whether this is possible >> (rq_src->bio is NULL) is another question. > > Hi Keith, I realized I mistakenly addressed my reply to you as "Keith" in this message. Apologies for the confusion. Thank you again for your input! > > You're right to bring this up. If rq_src->bio is NULL, the > __rq_for_each_bio macro will skip the loop, meaning the bio variable > won't be used at all. So, even if bio isn’t initialized, it won't cause > any issues in that case. > > Thanks for pointing that out. > > Best regards, > Suraj Best regards, Suraj
On Sun, Oct 06, 2024 at 12:33:58PM +0530, Suraj Sonawane wrote: > > If it fails, then bio is initialized to NULL. > You're correct, bio_alloc_clone returns NULL if it fails, so there’s no > uninitialized bio after that. My initial explanation wasn’t fully accurate, > but initializing bio to NULL is just a safety measure for any unexpected > issues later on. Or i am just trying to solve this issue by smatch tool: > block/blk-mq.c:3199 blk_rq_prep_clone() error: uninitialized symbol 'bio'. Please do this kind of research and clearly state it in the commit log. Now the actual useful cleanup here would be to: - 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 That makes the easier to reason about, which should make whatever static checker you have happy, and also allows humans to read it more nicely.
diff --git a/block/blk-mq.c b/block/blk-mq.c index 4b2c8e940..b2087bdd9 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -3156,7 +3156,7 @@ 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 = NULL, *bio_src; if (!bs) bs = &fs_bio_set;
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'. Initialize 'bio' to NULL to prevent undefined behavior from uninitialized access and safe cleanup in case of failure. Signed-off-by: SurajSonawane2415 <surajsonawane0215@gmail.com> --- block/blk-mq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)