Uninitialized pointers with `__free` attribute can cause undefined
behaviour as the memory assigned(randomly) to the pointer is freed
automatically when the pointer goes out of scope

scsi doesn't have any bugs related to this as of now, but
it is better to initialize and assign pointers with `__free` attr
in one statement to ensure proper scope-based cleanup

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/aPiG_F5EBQUjZqsl@stanley.mountain/
Signed-off-by: Ally Heev <allyheev@gmail.com>
---
 drivers/scsi/scsi_debug.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index b2ab97be5db3d43d5a5647968623b8db72448379..89b36d65926bdd15c0ae93a6bd2ea968e25c0e74 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -2961,11 +2961,11 @@ static int resp_mode_sense(struct scsi_cmnd *scp,
 	int target_dev_id;
 	int target = scp->device->id;
 	unsigned char *ap;
-	unsigned char *arr __free(kfree);
 	unsigned char *cmd = scp->cmnd;
 	bool dbd, llbaa, msense_6, is_disk, is_zbc, is_tape;
 
-	arr = kzalloc(SDEBUG_MAX_MSENSE_SZ, GFP_ATOMIC);
+	unsigned char *arr __free(kfree) = kzalloc(SDEBUG_MAX_MSENSE_SZ, GFP_ATOMIC);
+
 	if (!arr)
 		return -ENOMEM;
 	dbd = !!(cmd[1] & 0x8);		/* disable block descriptors */

---
base-commit: c9cfc122f03711a5124b4aafab3211cf4d35a2ac
change-id: 20251105-aheev-uninitialized-free-attr-scsi-d5f249383404

Best regards,
-- 
Ally Heev <allyheev@gmail.com>

On Wed, 05 Nov 2025 19:44:43 +0530, Ally Heev wrote:

> Uninitialized pointers with `__free` attribute can cause undefined
> behaviour as the memory assigned(randomly) to the pointer is freed
> automatically when the pointer goes out of scope
> 
> scsi doesn't have any bugs related to this as of now, but
> it is better to initialize and assign pointers with `__free` attr
> in one statement to ensure proper scope-based cleanup
> 
> [...]

Applied to 6.19/scsi-queue, thanks!

[1/1] scsi: fix uninitialized pointers with free attr
      https://git.kernel.org/mkp/scsi/c/3813d28b2b12

-- 
Martin K. Petersen

On Wed, 2025-11-05 at 19:44 +0530, Ally Heev wrote:
> Uninitialized pointers with `__free` attribute can cause undefined
> behaviour as the memory assigned(randomly) to the pointer is freed
> automatically when the pointer goes out of scope
> 
> scsi doesn't have any bugs related to this as of now, but
> it is better to initialize and assign pointers with `__free` attr
> in one statement to ensure proper scope-based cleanup
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes:
> https://lore.kernel.org/all/aPiG_F5EBQUjZqsl@stanley.mountain/
> Signed-off-by: Ally Heev <allyheev@gmail.com>
> ---
>  drivers/scsi/scsi_debug.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index
> b2ab97be5db3d43d5a5647968623b8db72448379..89b36d65926bdd15c0ae93a6bd2
> ea968e25c0e74 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -2961,11 +2961,11 @@ static int resp_mode_sense(struct scsi_cmnd
> *scp,
>  	int target_dev_id;
>  	int target = scp->device->id;
>  	unsigned char *ap;
> -	unsigned char *arr __free(kfree);
>  	unsigned char *cmd = scp->cmnd;
>  	bool dbd, llbaa, msense_6, is_disk, is_zbc, is_tape;
>  
> -	arr = kzalloc(SDEBUG_MAX_MSENSE_SZ, GFP_ATOMIC);
> +	unsigned char *arr __free(kfree) =
> kzalloc(SDEBUG_MAX_MSENSE_SZ, GFP_ATOMIC);
> +

Moving variable assignments inside code makes it way harder to read. 
Given that compilers will eventually detect if we do a return before
initialization, can't you have smatch do the same rather than trying to
force something like this?

Regards,

James

On Wed, Nov 05, 2025 at 09:21:45AM -0500, James Bottomley wrote:
> On Wed, 2025-11-05 at 19:44 +0530, Ally Heev wrote:
> > Uninitialized pointers with `__free` attribute can cause undefined
> > behaviour as the memory assigned(randomly) to the pointer is freed
> > automatically when the pointer goes out of scope
> > 
> > scsi doesn't have any bugs related to this as of now, but
> > it is better to initialize and assign pointers with `__free` attr
> > in one statement to ensure proper scope-based cleanup
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes:
> > https://lore.kernel.org/all/aPiG_F5EBQUjZqsl@stanley.mountain/
> > Signed-off-by: Ally Heev <allyheev@gmail.com>
> > ---
> >  drivers/scsi/scsi_debug.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> > index
> > b2ab97be5db3d43d5a5647968623b8db72448379..89b36d65926bdd15c0ae93a6bd2
> > ea968e25c0e74 100644
> > --- a/drivers/scsi/scsi_debug.c
> > +++ b/drivers/scsi/scsi_debug.c
> > @@ -2961,11 +2961,11 @@ static int resp_mode_sense(struct scsi_cmnd
> > *scp,
> >  	int target_dev_id;
> >  	int target = scp->device->id;
> >  	unsigned char *ap;
> > -	unsigned char *arr __free(kfree);
> >  	unsigned char *cmd = scp->cmnd;
> >  	bool dbd, llbaa, msense_6, is_disk, is_zbc, is_tape;
> >  
> > -	arr = kzalloc(SDEBUG_MAX_MSENSE_SZ, GFP_ATOMIC);
> > +	unsigned char *arr __free(kfree) =
> > kzalloc(SDEBUG_MAX_MSENSE_SZ, GFP_ATOMIC);
> > +
> 
> Moving variable assignments inside code makes it way harder to read. 
> Given that compilers will eventually detect if we do a return before
> initialization, can't you have smatch do the same rather than trying to
> force something like this?

This isn't a Smatch thing, it's a change to checkpatch.

(Smatch does work as you describe).

regards,
dan carpenter

On Wed, 2025-11-05 at 17:46 +0300, Dan Carpenter wrote:
> On Wed, Nov 05, 2025 at 09:21:45AM -0500, James Bottomley wrote:
> > On Wed, 2025-11-05 at 19:44 +0530, Ally Heev wrote:
> > > Uninitialized pointers with `__free` attribute can cause
> > > undefined
> > > behaviour as the memory assigned(randomly) to the pointer is
> > > freed
> > > automatically when the pointer goes out of scope
> > > 
> > > scsi doesn't have any bugs related to this as of now, but
> > > it is better to initialize and assign pointers with `__free` attr
> > > in one statement to ensure proper scope-based cleanup
> > > 
> > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > Closes:
> > > https://lore.kernel.org/all/aPiG_F5EBQUjZqsl@stanley.mountain/
> > > Signed-off-by: Ally Heev <allyheev@gmail.com>
> > > ---
> > >  drivers/scsi/scsi_debug.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/scsi_debug.c
> > > b/drivers/scsi/scsi_debug.c
> > > index
> > > b2ab97be5db3d43d5a5647968623b8db72448379..89b36d65926bdd15c0ae93a
> > > 6bd2
> > > ea968e25c0e74 100644
> > > --- a/drivers/scsi/scsi_debug.c
> > > +++ b/drivers/scsi/scsi_debug.c
> > > @@ -2961,11 +2961,11 @@ static int resp_mode_sense(struct
> > > scsi_cmnd
> > > *scp,
> > >  	int target_dev_id;
> > >  	int target = scp->device->id;
> > >  	unsigned char *ap;
> > > -	unsigned char *arr __free(kfree);
> > >  	unsigned char *cmd = scp->cmnd;
> > >  	bool dbd, llbaa, msense_6, is_disk, is_zbc, is_tape;
> > >  
> > > -	arr = kzalloc(SDEBUG_MAX_MSENSE_SZ, GFP_ATOMIC);
> > > +	unsigned char *arr __free(kfree) =
> > > kzalloc(SDEBUG_MAX_MSENSE_SZ, GFP_ATOMIC);
> > > +
> > 
> > Moving variable assignments inside code makes it way harder to
> > read. Given that compilers will eventually detect if we do a return
> > before initialization, can't you have smatch do the same rather
> > than trying to force something like this?
> 
> This isn't a Smatch thing, it's a change to checkpatch.
> 
> (Smatch does work as you describe).

So why are we bothering with something like this in checkpatch if we
can detect the true problem condition and we expect compilers to catch
up?  Encouraging people to write code like the above isn't in anyone's
best interest.

Regards,

James

On Wed, Nov 05, 2025 at 10:32:19AM -0500, James Bottomley wrote:
> > > > diff --git a/drivers/scsi/scsi_debug.c
> > > > b/drivers/scsi/scsi_debug.c
> > > > index
> > > > b2ab97be5db3d43d5a5647968623b8db72448379..89b36d65926bdd15c0ae93a
> > > > 6bd2
> > > > ea968e25c0e74 100644
> > > > --- a/drivers/scsi/scsi_debug.c
> > > > +++ b/drivers/scsi/scsi_debug.c
> > > > @@ -2961,11 +2961,11 @@ static int resp_mode_sense(struct
> > > > scsi_cmnd
> > > > *scp,
> > > >  	int target_dev_id;
> > > >  	int target = scp->device->id;
> > > >  	unsigned char *ap;
> > > > -	unsigned char *arr __free(kfree);
> > > >  	unsigned char *cmd = scp->cmnd;
> > > >  	bool dbd, llbaa, msense_6, is_disk, is_zbc, is_tape;
> > > >  
> > > > -	arr = kzalloc(SDEBUG_MAX_MSENSE_SZ, GFP_ATOMIC);
> > > > +	unsigned char *arr __free(kfree) =
> > > > kzalloc(SDEBUG_MAX_MSENSE_SZ, GFP_ATOMIC);
> > > > +
> > > 
> > > Moving variable assignments inside code makes it way harder to
> > > read. Given that compilers will eventually detect if we do a return
> > > before initialization, can't you have smatch do the same rather
> > > than trying to force something like this?
> > 
> > This isn't a Smatch thing, it's a change to checkpatch.
> > 
> > (Smatch does work as you describe).
> 
> So why are we bothering with something like this in checkpatch if we
> can detect the true problem condition and we expect compilers to catch
> up?  Encouraging people to write code like the above isn't in anyone's
> best interest.

Initializing __free variables has been considered best practice for a
long time.  Reviewers often will complain even if it doesn't cause a
bug.

regards,
dan carpenter

On Thu, 2025-11-06 at 17:46 +0300, Dan Carpenter wrote:
> On Wed, Nov 05, 2025 at 10:32:19AM -0500, James Bottomley wrote:
> > > > > diff --git a/drivers/scsi/scsi_debug.c
> > > > > b/drivers/scsi/scsi_debug.c
> > > > > index
> > > > > b2ab97be5db3d43d5a5647968623b8db72448379..89b36d65926bdd15c0a
> > > > > e93a
> > > > > 6bd2
> > > > > ea968e25c0e74 100644
> > > > > --- a/drivers/scsi/scsi_debug.c
> > > > > +++ b/drivers/scsi/scsi_debug.c
> > > > > @@ -2961,11 +2961,11 @@ static int resp_mode_sense(struct
> > > > > scsi_cmnd
> > > > > *scp,
> > > > >  	int target_dev_id;
> > > > >  	int target = scp->device->id;
> > > > >  	unsigned char *ap;
> > > > > -	unsigned char *arr __free(kfree);
> > > > >  	unsigned char *cmd = scp->cmnd;
> > > > >  	bool dbd, llbaa, msense_6, is_disk, is_zbc, is_tape;
> > > > >  
> > > > > -	arr = kzalloc(SDEBUG_MAX_MSENSE_SZ, GFP_ATOMIC);
> > > > > +	unsigned char *arr __free(kfree) =
> > > > > kzalloc(SDEBUG_MAX_MSENSE_SZ, GFP_ATOMIC);
> > > > > +
> > > > 
> > > > Moving variable assignments inside code makes it way harder to
> > > > read. Given that compilers will eventually detect if we do a
> > > > return before initialization, can't you have smatch do the same
> > > > rather than trying to force something like this?
> > > 
> > > This isn't a Smatch thing, it's a change to checkpatch.
> > > 
> > > (Smatch does work as you describe).
> > 
> > So why are we bothering with something like this in checkpatch if
> > we can detect the true problem condition and we expect compilers to
> > catch up?  Encouraging people to write code like the above isn't in
> > anyone's best interest.
> 
> Initializing __free variables has been considered best practice for a
> long time.  Reviewers often will complain even if it doesn't cause a
> bug.

Well, not responsible for the daft ideas other people have.

However, why would we treat a __free variable any differently from one
without the annotation?  The only difference is that a function gets
called on it before exit, but as long as something can detect calling
this on uninitialized variables their properties are definitely no
different from non-__free variables so the can be treated exactly the
same.

To revisit why we do this for non-__free variables: most people
(although there are definitely languages where this isn't true and
people who think we should follow this) think that having variables at
the top of a function (or at least top of a code block) make the code
easier to understand.  Additionally, keeping the variable uninitialized
allows the compiler to detect any use before set scenarios, which can
be somewhat helpful detecting code faults (I'm less persuaded by this,
particularly given the number of false positive warnings we've seen
that force us to add annotations, although this seems to be getting
better).

So either we throw out the above for everything ... which I really
wouldn't want, or we enforce it for *all* variables.

Regards,

James

On Thu, Nov 06, 2025 at 11:06:29AM -0500, James Bottomley wrote:
> On Thu, 2025-11-06 at 17:46 +0300, Dan Carpenter wrote:
> > On Wed, Nov 05, 2025 at 10:32:19AM -0500, James Bottomley wrote:
> > > > > > diff --git a/drivers/scsi/scsi_debug.c
> > > > > > b/drivers/scsi/scsi_debug.c
> > > > > > index
> > > > > > b2ab97be5db3d43d5a5647968623b8db72448379..89b36d65926bdd15c0a
> > > > > > e93a
> > > > > > 6bd2
> > > > > > ea968e25c0e74 100644
> > > > > > --- a/drivers/scsi/scsi_debug.c
> > > > > > +++ b/drivers/scsi/scsi_debug.c
> > > > > > @@ -2961,11 +2961,11 @@ static int resp_mode_sense(struct
> > > > > > scsi_cmnd
> > > > > > *scp,
> > > > > >  	int target_dev_id;
> > > > > >  	int target = scp->device->id;
> > > > > >  	unsigned char *ap;
> > > > > > -	unsigned char *arr __free(kfree);
> > > > > >  	unsigned char *cmd = scp->cmnd;
> > > > > >  	bool dbd, llbaa, msense_6, is_disk, is_zbc, is_tape;
> > > > > >  
> > > > > > -	arr = kzalloc(SDEBUG_MAX_MSENSE_SZ, GFP_ATOMIC);
> > > > > > +	unsigned char *arr __free(kfree) =
> > > > > > kzalloc(SDEBUG_MAX_MSENSE_SZ, GFP_ATOMIC);
> > > > > > +
> > > > > 
> > > > > Moving variable assignments inside code makes it way harder to
> > > > > read. Given that compilers will eventually detect if we do a
> > > > > return before initialization, can't you have smatch do the same
> > > > > rather than trying to force something like this?
> > > > 
> > > > This isn't a Smatch thing, it's a change to checkpatch.
> > > > 
> > > > (Smatch does work as you describe).
> > > 
> > > So why are we bothering with something like this in checkpatch if
> > > we can detect the true problem condition and we expect compilers to
> > > catch up?  Encouraging people to write code like the above isn't in
> > > anyone's best interest.
> > 
> > Initializing __free variables has been considered best practice for a
> > long time.  Reviewers often will complain even if it doesn't cause a
> > bug.
> 
> Well, not responsible for the daft ideas other people have.
> 
> However, why would we treat a __free variable any differently from one
> without the annotation?  The only difference is that a function gets
> called on it before exit, but as long as something can detect calling
> this on uninitialized variables their properties are definitely no
> different from non-__free variables so the can be treated exactly the
> same.
> 
> To revisit why we do this for non-__free variables: most people
> (although there are definitely languages where this isn't true and
> people who think we should follow this) think that having variables at
> the top of a function (or at least top of a code block) make the code
> easier to understand.  Additionally, keeping the variable uninitialized
> allows the compiler to detect any use before set scenarios, which can
> be somewhat helpful detecting code faults (I'm less persuaded by this,
> particularly given the number of false positive warnings we've seen
> that force us to add annotations, although this seems to be getting
> better).
> 
> So either we throw out the above for everything ... which I really
> wouldn't want, or we enforce it for *all* variables.
> 

Yeah.  You make a good point...

On the other hand, a bunch of maintainers are convinced that every free
variable should be initialized to a valid value at declaration time and
will reject patches which don't do that.  I see checkpatch as a way of
avoiding this round trip where a patch is automatically rejected because
of something trivial.

The truth is that the cleanup.h stuff is really new and I don't think
we've necessarily figured out all the best practices yet.

regards,
dan carpenter

On Tue, 2025-11-18 at 09:17 +0300, Dan Carpenter wrote:
> On Thu, Nov 06, 2025 at 11:06:29AM -0500, James Bottomley wrote:
[...]
> > However, why would we treat a __free variable any differently from
> > one without the annotation?  The only difference is that a function
> > gets called on it before exit, but as long as something can detect
> > calling this on uninitialized variables their properties are
> > definitely no different from non-__free variables so the can be
> > treated exactly the same.
> > 
> > To revisit why we do this for non-__free variables: most people
> > (although there are definitely languages where this isn't true and
> > people who think we should follow this) think that having variables
> > at the top of a function (or at least top of a code block) make the
> > code easier to understand.  Additionally, keeping the variable
> > uninitialized allows the compiler to detect any use before set
> > scenarios, which can be somewhat helpful detecting code faults (I'm
> > less persuaded by this, particularly given the number of false
> > positive warnings we've seen that force us to add annotations,
> > although this seems to be getting better).
> > 
> > So either we throw out the above for everything ... which I really
> > wouldn't want, or we enforce it for *all* variables.
> > 
> 
> Yeah.  You make a good point...
> 
> On the other hand, a bunch of maintainers are convinced that every
> free variable should be initialized to a valid value at declaration
> time and will reject patches which don't do that.

Which maintainers?  The true evil I see here is rule inconsistency
because it leads to confusion and bad coding.  So I'd hope if's fairly
easy to point out the errors ... and if they want to argue for
consistently coding everything with either variables always initialized
or variables declared in code, I'm sure they'll get their day.

> I see checkpatch as a way of avoiding this round trip where a patch
> is automatically rejected because of something trivial.

But you aren't just doing that ... what you're actually doing is
forcing bad coding rules on the rest of us.  Why else would I see a
patch in SCSI trying to move something that's correctly coded according
to our usual variable rules to this?

> The truth is that the cleanup.h stuff is really new and I don't think
> we've necessarily figured out all the best practices yet.

I get that ... and I'm not saying we shouldn't change stuff because of
it, I'm just saying that any rule we do change should be reasonable and
consistently applied.

Regards,

James

On Tue, Nov 18, 2025 at 08:21:16AM -0500, James Bottomley wrote:
> On Tue, 2025-11-18 at 09:17 +0300, Dan Carpenter wrote:
> > On Thu, Nov 06, 2025 at 11:06:29AM -0500, James Bottomley wrote:
> [...]
> > > However, why would we treat a __free variable any differently from
> > > one without the annotation?  The only difference is that a function
> > > gets called on it before exit, but as long as something can detect
> > > calling this on uninitialized variables their properties are
> > > definitely no different from non-__free variables so the can be
> > > treated exactly the same.
> > > 
> > > To revisit why we do this for non-__free variables: most people
> > > (although there are definitely languages where this isn't true and
> > > people who think we should follow this) think that having variables
> > > at the top of a function (or at least top of a code block) make the
> > > code easier to understand.  Additionally, keeping the variable
> > > uninitialized allows the compiler to detect any use before set
> > > scenarios, which can be somewhat helpful detecting code faults (I'm
> > > less persuaded by this, particularly given the number of false
> > > positive warnings we've seen that force us to add annotations,
> > > although this seems to be getting better).
> > > 
> > > So either we throw out the above for everything ... which I really
> > > wouldn't want, or we enforce it for *all* variables.
> > > 
> > 
> > Yeah.  You make a good point...
> > 
> > On the other hand, a bunch of maintainers are convinced that every
> > free variable should be initialized to a valid value at declaration
> > time and will reject patches which don't do that.
> 
> Which maintainers?

Here is an example where Krzysztof says "This is not recommended way of
using cleanup. You should declare it with constructor."
https://lore.kernel.org/all/a2fc02e2-d75a-43ed-8057-9b3860873ebb@kernel.org/

I know there are other people who feel this way as well but I can't
recall who.  It's in the documentation in include/linux/cleanup.h

 * Given that the "__free(...) = NULL" pattern for variables defined at
 * the top of the function poses this potential interdependency problem
 * the recommendation is to always define and assign variables in one
 * statement and not group variable definitions at the top of the
 * function when __free() is used.

regards,
dan carpenter

As per the ongoing discussion
https://lore.kernel.org/lkml/58fd478f408a34b578ee8d949c5c4b4da4d4f41d.camel@HansenPartnership.com/
, I believe there are no changes required here

Thanks,
Ally

On Wed, Nov 19, 2025 at 12:26:56PM +0530, ally heev wrote:
> As per the ongoing discussion
> https://lore.kernel.org/lkml/58fd478f408a34b578ee8d949c5c4b4da4d4f41d.camel@HansenPartnership.com/
> , I believe there are no changes required here

What about just dropping that __free thing that just make the code
harder to read and more buggy?

On Wed, 2025-11-19 at 00:31 -0800, Christoph Hellwig wrote:
> On Wed, Nov 19, 2025 at 12:26:56PM +0530, ally heev wrote:
> > As per the ongoing discussion
> > https://lore.kernel.org/lkml/58fd478f408a34b578ee8d949c5c4b4da4d4f41d.camel@HansenPartnership.com/
> > , I believe there are no changes required here
> 
> What about just dropping that __free thing that just make the code
> harder to read and more buggy?

It does?  The original patch was this:

https://lore.kernel.org/linux-scsi/20240222214508.1630719-9-bvanassche@acm.org/

It's part of the series adding Advice Hints, so we can't just revert it
because the sense buffer would be too small.  Using __free to replace a
stack allocation looks like a nice use of the cleanup primitives: I
think if we removed the __free and added a kfree before each of the six
returns that would make the code more prone to bugs on its next update.

Regards,

James