checkpatch: add uninitialized pointer with __free attribute check

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 <[email protected]>

Link https://lore.kernel.org/all/[email protected]/ open here

Signed-off-by Ally Heev <[email protected]>

---
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(+)

Documentation/dev-tools/checkpatch.rst +5 -0

index d5c47e560324fb2399a5b1bc99c891ed1de10535..1a304bf38bcd27e50bbb7cd4383b07ac54d20b0a 100644
@@ -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
 -----------

scripts/checkpatch.pl +0 -0

index 92669904eecc7a8d2afd3f2625528e02b6d17cd6..33cb09843431bebef72a4f5daab3a5d321bcb911 100755

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.

Quoted 8 lines

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

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.

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

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

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

Quoted 4 lines · Ally Heev

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:

scripts/checkpatch.pl

Quoted 1 line

scripts/checkpatch.pl +0 -0

[]

scripts/checkpatch.pl sub process { · line 7726

Quoted 7 lines

scripts/checkpatch.pl +3 -0

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

scripts/checkpatch.pl sub process { · line 7728

Quoted 2 lines

scripts/checkpatch.pl +2 -0

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

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: