Documentation/dev-tools/checkpatch.rst | 5 +++++ scripts/checkpatch.pl | 6 ++++++ 2 files changed, 11 insertions(+)
uninitialized pointers with __free attribute can cause undefined
behaviour as the memory allocated to the pointer is freed
automatically when the pointer goes out of scope.
add check in checkpatch to detect such issues
Suggested-by: Dan Carpenter <dan.carpenter@linaro.org>
Link: https://lore.kernel.org/all/8a4c0b43-cf63-400d-b33d-d9c447b7e0b9@suswa.mountain/
Signed-off-by: Ally Heev <allyheev@gmail.com>
---
Test:
ran checkpatch.pl before and after the change on
crypto/asymmetric_keys/x509_public_key.c, which has
both initialized and uninitialized pointers
---
Documentation/dev-tools/checkpatch.rst | 5 +++++
scripts/checkpatch.pl | 6 ++++++
2 files changed, 11 insertions(+)
diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index d5c47e560324fb2399a5b1bc99c891ed1de10535..1a304bf38bcd27e50bbb7cd4383b07ac54d20b0a 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -1009,6 +1009,11 @@ Functions and Variables
return bar;
+ **UNINITIALIZED_PTR_WITH_FREE**
+ Pointers with __free attribute should be initialized. Not doing so
+ may lead to undefined behavior as the memory allocated (garbage,
+ in case not initialized) to the pointer is freed automatically
+ when the pointer goes out of scope.
Permissions
-----------
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 92669904eecc7a8d2afd3f2625528e02b6d17cd6..33cb09843431bebef72a4f5daab3a5d321bcb911 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7721,6 +7721,12 @@ sub process {
ERROR("MISSING_SENTINEL", "missing sentinel in ID array\n" . "$here\n$stat\n");
}
}
+
+# check for uninitialized pointers with __free attribute
+ if ($line =~ /\s*$Type\s*($Ident)\s+__free\s*\(\s*$Ident\s*\)\s*;/) {
+ WARN("UNINITIALIZED_PTR_WITH_FREE",
+ "pointer '$1' with __free attribute should be initialized\n" . $herecurr);
+ }
}
# If we have no input at all, then there is nothing to report on
---
base-commit: 6548d364a3e850326831799d7e3ea2d7bb97ba08
change-id: 20251021-aheev-checkpatch-uninitialized-free-5c39f75e10a1
Best regards,
--
Ally Heev <allyheev@gmail.com>I made a list of the warnings this generates (on Monday's linux-next). None of the warnings are real bugs. Every single one of these has the assignment as the first statement after the declaration block. We have had bugs because of this before but Smatch and (I think) Clang detect them so they don't last for long. regards, dan carpenter arch/powerpc/platforms/82xx/km82xx.c:30: crypto/asymmetric_keys/x509_cert_parser.c:63: crypto/asymmetric_keys/x509_public_key.c:151: drivers/firmware/arm_scmi/shmem.c:199: drivers/net/ethernet/intel/ice/ice_flow.c:1576: drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:1015: drivers/net/ethernet/microsoft/mana/gdma_main.c:1508: drivers/net/wireless/intel/iwlwifi/fw/uefi.c:821: drivers/net/wireless/intel/iwlwifi/mld/d3.c:1788: drivers/opp/core.c:1413: drivers/opp/core.c:1480: drivers/opp/core.c:1797: drivers/opp/core.c:1888: drivers/opp/core.c:2874: drivers/opp/core.c:2935: drivers/opp/core.c:2989: drivers/opp/core.c:3065: drivers/opp/core.c:3085: drivers/opp/core.c:3104: drivers/opp/core.c:312: drivers/opp/core.c:330: drivers/opp/core.c:412: drivers/opp/core.c:450: drivers/opp/core.c:608: drivers/opp/cpu.c:157: drivers/opp/cpu.c:204: drivers/opp/cpu.c:59: drivers/opp/of.c:1272: drivers/opp/of.c:1331: drivers/opp/of.c:1428: drivers/opp/of.c:1469: drivers/opp/of.c:149: drivers/opp/of.c:1505: drivers/opp/of.c:174: drivers/opp/of.c:276: drivers/opp/of.c:352: drivers/opp/of.c:409: drivers/opp/of.c:48: drivers/opp/of.c:98: drivers/scsi/scsi_debug.c:2964: drivers/tee/qcomtee/call.c:648: fs/overlayfs/params.c:451:
On Wed, Oct 22, 2025 at 12:55 PM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > I made a list of the warnings this generates (on Monday's linux-next). > > None of the warnings are real bugs. Every single one of these has the > assignment as the first statement after the declaration block. We have > had bugs because of this before but Smatch and (I think) Clang detect > them so they don't last for long. > > regards, > dan carpenter > > arch/powerpc/platforms/82xx/km82xx.c:30: > crypto/asymmetric_keys/x509_cert_parser.c:63: > crypto/asymmetric_keys/x509_public_key.c:151: > drivers/firmware/arm_scmi/shmem.c:199: > drivers/net/ethernet/intel/ice/ice_flow.c:1576: > drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:1015: > drivers/net/ethernet/microsoft/mana/gdma_main.c:1508: > drivers/net/wireless/intel/iwlwifi/fw/uefi.c:821: > drivers/net/wireless/intel/iwlwifi/mld/d3.c:1788: > drivers/opp/core.c:1413: > drivers/opp/core.c:1480: > drivers/opp/core.c:1797: > drivers/opp/core.c:1888: > drivers/opp/core.c:2874: > drivers/opp/core.c:2935: > drivers/opp/core.c:2989: > drivers/opp/core.c:3065: > drivers/opp/core.c:3085: > drivers/opp/core.c:3104: > drivers/opp/core.c:312: > drivers/opp/core.c:330: > drivers/opp/core.c:412: > drivers/opp/core.c:450: > drivers/opp/core.c:608: > drivers/opp/cpu.c:157: > drivers/opp/cpu.c:204: > drivers/opp/cpu.c:59: > drivers/opp/of.c:1272: > drivers/opp/of.c:1331: > drivers/opp/of.c:1428: > drivers/opp/of.c:1469: > drivers/opp/of.c:149: > drivers/opp/of.c:1505: > drivers/opp/of.c:174: > drivers/opp/of.c:276: > drivers/opp/of.c:352: > drivers/opp/of.c:409: > drivers/opp/of.c:48: > drivers/opp/of.c:98: > drivers/scsi/scsi_debug.c:2964: > drivers/tee/qcomtee/call.c:648: > fs/overlayfs/params.c:451: Thanks for reporting these. I will try to get to these in later patches. But, how do we test the changes? KTODO: run checkpatch with uninitialized pointer with __free attribute check and fix the errors
On Tue, 2025-10-21 at 17:00 +0530, Ally Heev wrote:
> uninitialized pointers with __free attribute can cause undefined
> behaviour as the memory allocated to the pointer is freed
> automatically when the pointer goes out of scope.
> add check in checkpatch to detect such issues
Seems sensible. Couple minor points below:
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -7721,6 +7721,12 @@ sub process {
> ERROR("MISSING_SENTINEL", "missing sentinel in ID array\n" . "$here\n$stat\n");
> }
> }
> +
> +# check for uninitialized pointers with __free attribute
> + if ($line =~ /\s*$Type\s*($Ident)\s+__free\s*\(\s*$Ident\s*\)\s*;/) {
The leading \s* isn't useful, but \b should be used.
Perhaps verify that $Type is a pointer as well
if ($line =~ /\b($Type)\s*($Ident)\s*__free\s*\(\s*$Ident\s*\)\s*;/ &&
$1 =~ /\*\s*$/) {
to avoid things like:
drivers/net/ethernet/microsoft/mana/gdma_main.c: cpumask_var_t cpus __free(free_cpumask_var);
> + WARN("UNINITIALIZED_PTR_WITH_FREE",
> + "pointer '$1' with __free attribute should be initialized\n" . $herecurr);
pointer '$2' etc
And this would not find uses like the below where another definition
is done before a definition with __free on the same line:
crypto/testmgr.c: u8 *ptr, *key __free(kfree);
There are many uses in drivers/opp/ that could be updated where
the initialization is done after the definition like the below:
(I've added the opp maintainers to the cc's)
drivers/opp/core.c-unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev)
drivers/opp/core.c-{
drivers/opp/core.c: struct opp_table *opp_table __free(put_opp_table);
drivers/opp/core.c-
drivers/opp/core.c- opp_table = _find_opp_table(dev);
An aside found while using grep:
There are uses of DEFINE_FREE that seem to have an unnecessary trailing ;
$ git grep -w DEFINE_FREE | grep ';'
drivers/firmware/efi/libstub/efistub.h:DEFINE_FREE(efi_pool, void *, if (_T) efi_bs_call(free_pool, _T));
drivers/fwctl/mlx5/main.c:DEFINE_FREE(mlx5ctl, struct mlx5ctl_dev *, if (_T) fwctl_put(&_T->fwctl));
drivers/pci/msi/msi.c:DEFINE_FREE(free_msi_irqs, struct pci_dev *, if (_T) pci_free_msi_irqs(_T));
drivers/tty/vt/vc_screen.c:DEFINE_FREE(free_page_ptr, void *, if (_T) free_page((unsigned long)_T));
fs/pstore/inode.c:DEFINE_FREE(pstore_private, struct pstore_private *, free_pstore_private(_T));
include/linux/cpumask.h:DEFINE_FREE(free_cpumask_var, struct cpumask *, if (_T) free_cpumask_var(_T));
include/linux/execmem.h:DEFINE_FREE(execmem, void *, if (_T) execmem_free(_T));
include/linux/fwctl.h:DEFINE_FREE(fwctl, struct fwctl_device *, if (_T) fwctl_put(_T));
net/core/dev.h:DEFINE_FREE(netdev_unlock, struct net_device *, if (_T) netdev_unlock(_T));
On Tue, 2025-10-21 at 10:06 -0700, Joe Perches wrote:
> On Tue, 2025-10-21 at 17:00 +0530, Ally Heev wrote:
> > uninitialized pointers with __free attribute can cause undefined
> > behaviour as the memory allocated to the pointer is freed
> > automatically when the pointer goes out of scope.
> > add check in checkpatch to detect such issues
>
> Seems sensible. Couple minor points below:
>
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -7721,6 +7721,12 @@ sub process {
> > ERROR("MISSING_SENTINEL", "missing sentinel in ID array\n" . "$here\n$stat\n");
> > }
> > }
> > +
> > +# check for uninitialized pointers with __free attribute
> > + if ($line =~ /\s*$Type\s*($Ident)\s+__free\s*\(\s*$Ident\s*\)\s*;/) {
>
> The leading \s* isn't useful, but \b should be used.
Sure
>
> Perhaps verify that $Type is a pointer as well
>
> if ($line =~ /\b($Type)\s*($Ident)\s*__free\s*\(\s*$Ident\s*\)\s*;/ &&
> $1 =~ /\*\s*$/) {
>
> to avoid things like:
>
> drivers/net/ethernet/microsoft/mana/gdma_main.c: cpumask_var_t cpus __free(free_cpumask_var);
>
>
> > + WARN("UNINITIALIZED_PTR_WITH_FREE",
> > + "pointer '$1' with __free attribute should be initialized\n" . $herecurr);
>
> pointer '$2' etc
>
Nice find. Thanks!
> And this would not find uses like the below where another definition
> is done before a definition with __free on the same line:
>
> crypto/testmgr.c: u8 *ptr, *key __free(kfree);
I will add the check for each pointer defined on the same line
On 21-10-25, 10:06, Joe Perches wrote:
> There are many uses in drivers/opp/ that could be updated where
> the initialization is done after the definition like the below:
> (I've added the opp maintainers to the cc's)
>
> drivers/opp/core.c-unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev)
> drivers/opp/core.c-{
> drivers/opp/core.c: struct opp_table *opp_table __free(put_opp_table);
> drivers/opp/core.c-
> drivers/opp/core.c- opp_table = _find_opp_table(dev);
https://lore.kernel.org/all/45a64ff434a027c296d1d5c35f442e51378c9425.1761128347.git.viresh.kumar@linaro.org/
--
viresh
Joe Perches wrote:
[..]
> drivers/opp/core.c-unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev)
> drivers/opp/core.c-{
> drivers/opp/core.c: struct opp_table *opp_table __free(put_opp_table);
> drivers/opp/core.c-
> drivers/opp/core.c- opp_table = _find_opp_table(dev);
The documentation in include/linux/cleanup.h recommends always combining
declaration and allocation. The rule of "variable declarations at the
top of the function" is relaxed for scope-based cleanup.
> An aside found while using grep:
>
> There are uses of DEFINE_FREE that seem to have an unnecessary trailing ;
>
> $ git grep -w DEFINE_FREE | grep ';'
> drivers/firmware/efi/libstub/efistub.h:DEFINE_FREE(efi_pool, void *, if (_T) efi_bs_call(free_pool, _T));
> drivers/fwctl/mlx5/main.c:DEFINE_FREE(mlx5ctl, struct mlx5ctl_dev *, if (_T) fwctl_put(&_T->fwctl));
> drivers/pci/msi/msi.c:DEFINE_FREE(free_msi_irqs, struct pci_dev *, if (_T) pci_free_msi_irqs(_T));
> drivers/tty/vt/vc_screen.c:DEFINE_FREE(free_page_ptr, void *, if (_T) free_page((unsigned long)_T));
> fs/pstore/inode.c:DEFINE_FREE(pstore_private, struct pstore_private *, free_pstore_private(_T));
> include/linux/cpumask.h:DEFINE_FREE(free_cpumask_var, struct cpumask *, if (_T) free_cpumask_var(_T));
> include/linux/execmem.h:DEFINE_FREE(execmem, void *, if (_T) execmem_free(_T));
> include/linux/fwctl.h:DEFINE_FREE(fwctl, struct fwctl_device *, if (_T) fwctl_put(_T));
> net/core/dev.h:DEFINE_FREE(netdev_unlock, struct net_device *, if (_T) netdev_unlock(_T));
Note that until this issue is addressed:
https://github.com/universal-ctags/ctags/issues/4289
...it may be the case that Linux *wants* to allow dangling semicolon
for DEFINE_FREE() if only to help "make tags" find the symbols following
DEFINE_FREE() invocations.
It is interesting that this happens to not impact DEFINE_FREE()
instances that use IS_ERR_OR_NULL.
Ally Heev wrote:
> uninitialized pointers with __free attribute can cause undefined
> behaviour as the memory allocated to the pointer is freed
> automatically when the pointer goes out of scope.
> add check in checkpatch to detect such issues
>
> Suggested-by: Dan Carpenter <dan.carpenter@linaro.org>
> Link: https://lore.kernel.org/all/8a4c0b43-cf63-400d-b33d-d9c447b7e0b9@suswa.mountain/
> Signed-off-by: Ally Heev <allyheev@gmail.com>
> ---
> Test:
> ran checkpatch.pl before and after the change on
> crypto/asymmetric_keys/x509_public_key.c, which has
> both initialized and uninitialized pointers
> ---
> Documentation/dev-tools/checkpatch.rst | 5 +++++
> scripts/checkpatch.pl | 6 ++++++
> 2 files changed, 11 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index d5c47e560324fb2399a5b1bc99c891ed1de10535..1a304bf38bcd27e50bbb7cd4383b07ac54d20b0a 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -1009,6 +1009,11 @@ Functions and Variables
>
> return bar;
>
> + **UNINITIALIZED_PTR_WITH_FREE**
> + Pointers with __free attribute should be initialized. Not doing so
> + may lead to undefined behavior as the memory allocated (garbage,
> + in case not initialized) to the pointer is freed automatically
> + when the pointer goes out of scope.
>
> Permissions
> -----------
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 92669904eecc7a8d2afd3f2625528e02b6d17cd6..33cb09843431bebef72a4f5daab3a5d321bcb911 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -7721,6 +7721,12 @@ sub process {
> ERROR("MISSING_SENTINEL", "missing sentinel in ID array\n" . "$here\n$stat\n");
> }
> }
> +
> +# check for uninitialized pointers with __free attribute
> + if ($line =~ /\s*$Type\s*($Ident)\s+__free\s*\(\s*$Ident\s*\)\s*;/) {
> + WARN("UNINITIALIZED_PTR_WITH_FREE",
> + "pointer '$1' with __free attribute should be initialized\n" . $herecurr);
Looks good to me, but I why WARN and not ERROR? Is there ever a valid
reason to ignore this warning?
I would go futher and suggest that the pattern of:
type foo __free(free_foo) = NULL;
...be made into a warning because that easily leads to situations where
declaration order is out of sync with allocation order. I.e. can be made
technically correct, but at a level of cleverness that undermines the
benefit.
With or without the conversion to ERROR() for the above,
Acked-by: Dan Williams <dan.j.williams@intel.com>
On Tue, Oct 21, 2025 at 09:43:28AM -0700, dan.j.williams@intel.com wrote: > I would go futher and suggest that the pattern of: > > type foo __free(free_foo) = NULL; > > ...be made into a warning because that easily leads to situations where > declaration order is out of sync with allocation order. I.e. can be made > technically correct, but at a level of cleverness that undermines the > benefit. To be honest, I'm not sure what you're saying here... I have written code like this. There are 515 places which use this format. I think it would be a controversial change. $ git grep __free | grep "= NULL" | wc -l 515 regards, dan carpenter
On Tue, 2025-10-21 at 09:43 -0700, dan.j.williams@intel.com wrote: [...] > Looks good to me, but I why WARN and not ERROR? Is there ever a valid > reason to ignore this warning? makes sense. I will make it an ERROR > > I would go futher and suggest that the pattern of: > > type foo __free(free_foo) = NULL; > > ...be made into a warning because that easily leads to situations where > declaration order is out of sync with allocation order. I.e. can be made > technically correct, but at a level of cleverness that undermines the > benefit. But, does this pattern cause any real issue? I found allocating memory later useful in cases like below arch/powerpc/perf/vpa-dtl.c ``` struct vpa_pmu_buf *buf __free(kfree) = NULL; struct page **pglist __free(kfree) = NULL; /* We need at least one page for this to work. */ if (!nr_pages) return NULL; if (cpu == -1) cpu = raw_smp_processor_id(); buf = kzalloc_node(sizeof(*buf), GFP_KERNEL, cpu_to_node(cpu)); ```
On Thu, Oct 23, 2025 at 4:01 PM ally heev <allyheev@gmail.com> wrote: > > I would go futher and suggest that the pattern of: > > > > type foo __free(free_foo) = NULL; > > > > ...be made into a warning because that easily leads to situations where > > declaration order is out of sync with allocation order. I.e. can be made > > technically correct, but at a level of cleverness that undermines the > > benefit. > > But, does this pattern cause any real issue? I found allocating memory > later useful in cases like below > > arch/powerpc/perf/vpa-dtl.c > ``` > > struct vpa_pmu_buf *buf __free(kfree) = NULL; > struct page **pglist __free(kfree) = NULL; > > /* We need at least one page for this to work. */ > if (!nr_pages) > return NULL; > > if (cpu == -1) > cpu = raw_smp_processor_id(); > > buf = kzalloc_node(sizeof(*buf), GFP_KERNEL, > cpu_to_node(cpu)); > ``` > I will take this back. Found this 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. ```
On Thu, Oct 23, 2025 at 04:38:43PM +0530, ally heev wrote: > I will take this back. Found this 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. > ``` Ah, right. regards, dan carpenter
© 2016 - 2026 Red Hat, Inc.