Re: [PATCH] checkpatch: add uninitialized pointer with __free attribute check

Re: [PATCH] checkpatch: add uninitialized pointer with __free attribute check

[Joe Perches]

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

Re: [PATCH] checkpatch: add uninitialized pointer with __free attribute check

[dan.j.williams]

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.

Re: [PATCH] checkpatch: add uninitialized pointer with __free attribute check

[Viresh Kumar]

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

Re: [PATCH] checkpatch: add uninitialized pointer with __free attribute check

[ally heev]

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

Re: [PATCH] checkpatch: add uninitialized pointer with __free attribute check

[Dan Carpenter]

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:

Re: [PATCH] checkpatch: add uninitialized pointer with __free attribute check

[ally heev]

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

Re: [PATCH] checkpatch: add uninitialized pointer with __free attribute check

[ally heev]

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

Re: [PATCH] checkpatch: add uninitialized pointer with __free attribute check

[ally heev]

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

Re: [PATCH] checkpatch: add uninitialized pointer with __free attribute check

[Dan Carpenter]

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