[PATCH] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[PATCH] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[SurajSonawane2415]

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.1

Re: [PATCH] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[Christoph Hellwig]

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.

Explanation on Uninitialized Variable bio in blk_rq_prep_clone

[SurajSonawane2415]

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

Re: Explanation on Uninitialized Variable bio in blk_rq_prep_clone

[Hannes Reinecke]

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

Re: Explanation on Uninitialized Variable bio in blk_rq_prep_clone

[John Garry]

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.

Re: Explanation on Uninitialized Variable bio in blk_rq_prep_clone

[Keith Busch]

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.

Re: Explanation on Uninitialized Variable bio in blk_rq_prep_clone

[Keith Busch]

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.

Re: Explanation on Uninitialized Variable bio in blk_rq_prep_clone

[Suraj Sonawane]

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

Re: Explanation on Uninitialized Variable bio in blk_rq_prep_clone

[Suraj Sonawane]

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

Re: Explanation on Uninitialized Variable bio in blk_rq_prep_clone

[Suraj Sonawane]

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

Re: Explanation on Uninitialized Variable bio in blk_rq_prep_clone

[Christoph Hellwig]

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.

[PATCH v2] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[SurajSonawane2415]

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.1

Re: [PATCH v2] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[Christoph Hellwig]

On 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.

[PATCH v3] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[SurajSonawane2415]

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.1

Re: [PATCH v3] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[Christoph Hellwig]

> +		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);

Re: [PATCH v3] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[Keith Busch]

On 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.

Re: [PATCH v3] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[Keith Busch]

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.

[PATCH v4] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[SurajSonawane2415]

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.1

Re: [PATCH v4] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[Christoph Hellwig]

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.

Re: [PATCH v4] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[Suraj Sonawane]

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

Re: [PATCH v4] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[Christoph Hellwig]

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.

Re: [PATCH v4] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[Suraj Sonawane]

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

Re: [PATCH v4] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[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

Re: [PATCH v4] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[Jens Axboe]

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

Re: [PATCH v4] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[Suraj Sonawane]

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

Re: [PATCH v4] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[Christoph Hellwig]

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.

Re: [PATCH v4] block: Fix uninitialized symbol 'bio' in blk_rq_prep_clone

[Suraj Sonawane]

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