block/blk-mq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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(-)
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;
--
2.34.1On 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 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 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 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.
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 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 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 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 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 -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
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;
--
2.34.1On 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.
--
Jens Axboe
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
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
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.
block/blk-mq.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 4b2c8e940..27b22dbfc 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,
+ 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;
--
2.34.1On Tue, Oct 08, 2024 at 05:34:13PM +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'.
...
> @@ -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,
> + 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);
I think your commit message is missing the real "fix" here. The other
place that goto's this label is if blk_crypto_rq_bio_prep() fails. At
this point, the cloned 'rq' has all the bio's that get cleaned up in
blk_rq_unprep_clone(), so that failure scenario is double put'ing the
last bio.
On Tue, Oct 08, 2024 at 08:52:07AM -0600, Keith Busch wrote: > I think your commit message is missing the real "fix" here. The other > place that goto's this label is if blk_crypto_rq_bio_prep() fails. At > this point, the cloned 'rq' has all the bio's that get cleaned up in > blk_rq_unprep_clone(), so that failure scenario is double put'ing the > last bio. Ah, forget that. The existing code was NULL'ing the bio before prep_clone, so the scenario I described doesn't happen.
> + struct bio *bio = bio_alloc_clone(rq->q->disk->part0, bio_src, gfp_mask, > bs); Overly long line here, plus now pretty weird positioning of the bs argument. Should be something like: struct bio *bio = bio_alloc_clone(rq->q->disk->part0, bio_src, gfp_mask, bs);
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 - https://lore.kernel.org/lkml/20241004100842.9052-1-surajsonawane0215@gmail.com/
V2 - Move bio_put(bio) into the bio_ctr error handling block,
ensuring memory cleanup occurs only when the bio_ctr fail.
block/blk-mq.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 4b2c8e940..32f99116c 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -3167,8 +3167,10 @@ int blk_rq_prep_clone(struct request *rq, struct request *rq_src,
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;
@@ -3196,8 +3198,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.1On Tue, Oct 08, 2024 at 01:28:36AM +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'. Jens' comment baout the commit logs apply here as well. Otherwise this looks much better than the first version, but please also move the bio variable into the loop scope while you're at it, which also removes the need to clear it to NULL at the end of the loop.
© 2016 - 2024 Red Hat, Inc.