| Message ID | 20241008175215.23975-1-surajsonawane0215@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone | expand |
The patch itself looks good: Reviewed-by: Christoph Hellwig <hch@lst.de> On Tue, Oct 08, 2024 at 11:22:15PM +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'. To make this more readable I'd usually keep and empty line before the actual error message. But more importantly it would be useful to explain what tool generated said error message, and maybe also add a summary of the discussion why this function was in many ways pretty horrible code.
On 09/10/24 13:00, Christoph Hellwig wrote: > The patch itself looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > On Tue, Oct 08, 2024 at 11:22:15PM +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'. > > To make this more readable I'd usually keep and empty line before > the actual error message. But more importantly it would be useful > to explain what tool generated said error message, and maybe also add > a summary of the discussion why this function was in many ways > pretty horrible code. > Thank you for the review and suggestions. Should I submit a new version with the added empty line and explanation about the tool and function issues? Best regards, Suraj Sonawane
On Wed, Oct 09, 2024 at 04:30:56PM +0530, Suraj Sonawane wrote: > Should I submit a new version with the added empty line and explanation > about the tool and function issues? Let's wait for Jens if he wants a resend or not. In the meantime just tell us what tool you are using.
On 09/10/24 17:07, Christoph Hellwig wrote: > On Wed, Oct 09, 2024 at 04:30:56PM +0530, Suraj Sonawane wrote: >> Should I submit a new version with the added empty line and explanation >> about the tool and function issues? > > Let's wait for Jens if he wants a resend or not. In the meantime > just tell us what tool you are using. > Okay sure! I found this error using the Smatch tool. Best, Suraj Sonawane
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. Thank you for your time and consideration. I look forward to your response. Best regards, Suraj Sonawane
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.
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
On Sat, Nov 16, 2024 at 05:02:57PM +0530, Suraj Sonawane wrote: > 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. FYI, I still think the change is useful. It makes the code a lot better to read for humans and machines, and fixes a static checker false positive. So I'd still love to see it, it just needs a better commit log. Feel free to contact me off list if you need help with that.
On 18/11/24 11:58, Christoph Hellwig wrote: > On Sat, Nov 16, 2024 at 05:02:57PM +0530, Suraj Sonawane wrote: >> 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. > > FYI, I still think the change is useful. It makes the code a lot > better to read for humans and machines, and fixes a static checker > false positive. So I'd still love to see it, it just needs a better > commit log. Feel free to contact me off list if you need help with > that. > Thank you for your valuable input and for encouraging me to improve the patch. I have sent a V5 version of the patch with an updated and detailed commit log, including a thorough explanation of the changes and a summary of the discussion so far. You can find the patch here: https://lore.kernel.org/lkml/20241119164412.37609-1-surajsonawane0215@gmail.com/ I appreciate your willingness to review it and provide feedback. Best regards, Suraj Sonawane
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;
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(-)