Re: [PATCH] scsi: fix uninitialized pointers with free attr

[James Bottomley <James.Bottomley@HansenPartnership.com>]

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