* [PATCH] scsi: fix uninitialized pointers with free attr @ 2025-11-05 14:14 Ally Heev 2025-11-05 14:21 ` James Bottomley 2025-11-20 4:15 ` Martin K. Petersen 0 siblings, 2 replies; 13+ messages in thread From: Ally Heev @ 2025-11-05 14:14 UTC (permalink / raw) To: James E.J. Bottomley, Martin K. Petersen Cc: linux-scsi, linux-kernel, Dan Carpenter, Ally Heev 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> ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] scsi: fix uninitialized pointers with free attr 2025-11-05 14:14 [PATCH] scsi: fix uninitialized pointers with free attr Ally Heev @ 2025-11-05 14:21 ` James Bottomley 2025-11-05 14:46 ` Dan Carpenter 2025-11-20 4:15 ` Martin K. Petersen 1 sibling, 1 reply; 13+ messages in thread From: James Bottomley @ 2025-11-05 14:21 UTC (permalink / raw) To: Ally Heev, Martin K. Petersen; +Cc: linux-scsi, linux-kernel, Dan Carpenter 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] scsi: fix uninitialized pointers with free attr 2025-11-05 14:21 ` James Bottomley @ 2025-11-05 14:46 ` Dan Carpenter 2025-11-05 15:32 ` James Bottomley 0 siblings, 1 reply; 13+ messages in thread From: Dan Carpenter @ 2025-11-05 14:46 UTC (permalink / raw) To: James Bottomley; +Cc: Ally Heev, Martin K. Petersen, linux-scsi, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] scsi: fix uninitialized pointers with free attr 2025-11-05 14:46 ` Dan Carpenter @ 2025-11-05 15:32 ` James Bottomley 2025-11-06 14:46 ` Dan Carpenter 0 siblings, 1 reply; 13+ messages in thread From: James Bottomley @ 2025-11-05 15:32 UTC (permalink / raw) To: Dan Carpenter; +Cc: Ally Heev, Martin K. Petersen, linux-scsi, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] scsi: fix uninitialized pointers with free attr 2025-11-05 15:32 ` James Bottomley @ 2025-11-06 14:46 ` Dan Carpenter 2025-11-06 16:06 ` James Bottomley 0 siblings, 1 reply; 13+ messages in thread From: Dan Carpenter @ 2025-11-06 14:46 UTC (permalink / raw) To: James Bottomley; +Cc: Ally Heev, Martin K. Petersen, linux-scsi, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] scsi: fix uninitialized pointers with free attr 2025-11-06 14:46 ` Dan Carpenter @ 2025-11-06 16:06 ` James Bottomley 2025-11-18 6:17 ` Dan Carpenter 0 siblings, 1 reply; 13+ messages in thread From: James Bottomley @ 2025-11-06 16:06 UTC (permalink / raw) To: Dan Carpenter; +Cc: Ally Heev, Martin K. Petersen, linux-scsi, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] scsi: fix uninitialized pointers with free attr 2025-11-06 16:06 ` James Bottomley @ 2025-11-18 6:17 ` Dan Carpenter 2025-11-18 13:21 ` James Bottomley 0 siblings, 1 reply; 13+ messages in thread From: Dan Carpenter @ 2025-11-18 6:17 UTC (permalink / raw) To: James Bottomley; +Cc: Ally Heev, Martin K. Petersen, linux-scsi, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] scsi: fix uninitialized pointers with free attr 2025-11-18 6:17 ` Dan Carpenter @ 2025-11-18 13:21 ` James Bottomley 2025-11-18 14:22 ` Dan Carpenter 0 siblings, 1 reply; 13+ messages in thread From: James Bottomley @ 2025-11-18 13:21 UTC (permalink / raw) To: Dan Carpenter; +Cc: Ally Heev, Martin K. Petersen, linux-scsi, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] scsi: fix uninitialized pointers with free attr 2025-11-18 13:21 ` James Bottomley @ 2025-11-18 14:22 ` Dan Carpenter 2025-11-19 6:56 ` ally heev 0 siblings, 1 reply; 13+ messages in thread From: Dan Carpenter @ 2025-11-18 14:22 UTC (permalink / raw) To: James Bottomley; +Cc: Ally Heev, Martin K. Petersen, linux-scsi, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] scsi: fix uninitialized pointers with free attr 2025-11-18 14:22 ` Dan Carpenter @ 2025-11-19 6:56 ` ally heev 2025-11-19 8:31 ` Christoph Hellwig 0 siblings, 1 reply; 13+ messages in thread From: ally heev @ 2025-11-19 6:56 UTC (permalink / raw) To: Dan Carpenter, James Bottomley Cc: Martin K. Petersen, linux-scsi, linux-kernel As per the ongoing discussion https://lore.kernel.org/lkml/58fd478f408a34b578ee8d949c5c4b4da4d4f41d.camel@HansenPartnership.com/ , I believe there are no changes required here Thanks, Ally ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] scsi: fix uninitialized pointers with free attr 2025-11-19 6:56 ` ally heev @ 2025-11-19 8:31 ` Christoph Hellwig 2025-11-19 12:43 ` James Bottomley 0 siblings, 1 reply; 13+ messages in thread From: Christoph Hellwig @ 2025-11-19 8:31 UTC (permalink / raw) To: ally heev Cc: Dan Carpenter, James Bottomley, Martin K. Petersen, linux-scsi, linux-kernel 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? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] scsi: fix uninitialized pointers with free attr 2025-11-19 8:31 ` Christoph Hellwig @ 2025-11-19 12:43 ` James Bottomley 0 siblings, 0 replies; 13+ messages in thread From: James Bottomley @ 2025-11-19 12:43 UTC (permalink / raw) To: Christoph Hellwig, ally heev Cc: Dan Carpenter, Martin K. Petersen, linux-scsi, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] scsi: fix uninitialized pointers with free attr 2025-11-05 14:14 [PATCH] scsi: fix uninitialized pointers with free attr Ally Heev 2025-11-05 14:21 ` James Bottomley @ 2025-11-20 4:15 ` Martin K. Petersen 1 sibling, 0 replies; 13+ messages in thread From: Martin K. Petersen @ 2025-11-20 4:15 UTC (permalink / raw) To: James E.J. Bottomley, Ally Heev Cc: Martin K . Petersen, linux-scsi, linux-kernel, Dan Carpenter 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-11-20 4:16 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-11-05 14:14 [PATCH] scsi: fix uninitialized pointers with free attr Ally Heev 2025-11-05 14:21 ` James Bottomley 2025-11-05 14:46 ` Dan Carpenter 2025-11-05 15:32 ` James Bottomley 2025-11-06 14:46 ` Dan Carpenter 2025-11-06 16:06 ` James Bottomley 2025-11-18 6:17 ` Dan Carpenter 2025-11-18 13:21 ` James Bottomley 2025-11-18 14:22 ` Dan Carpenter 2025-11-19 6:56 ` ally heev 2025-11-19 8:31 ` Christoph Hellwig 2025-11-19 12:43 ` James Bottomley 2025-11-20 4:15 ` Martin K. Petersen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox