Re: devm_ on lists

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





Le 18/05/2023 à 09:59, Christophe JAILLET a écrit :
Le 18/05/2023 à 09:27, Dan Carpenter a écrit :
I sometimes have more ideas than I can handle in a day and my mind
resets every night so every morning I am a blank slate like that guy
from that movie.

So I'm going to start writing them down and adding a hashtag.
#StaticCheckerIdeas  I'm going to CC kernel janitors until the Smatch
list gets archived on lore.kernel.org

There is a bug in ti_sci_probe() where it does:

    info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
    ...
    list_add_tail(&info->node, &ti_sci_list);
    ...
    return of_platform_populate(dev->of_node, NULL, NULL, dev);

Idea 1:  If of_platform_populate() fails, then info freed but it's it's
still on the the &ti_sci_list list.  Look for list_add of devm_ data
and ensure that it's removed from the list before an error code.

There are other lifetime issues surrounding devm_ data.  We need a
list_del in the release function as well.

Idea 2: The other error paths all have clean up so last error path
should have clean up too.

regards,
dan carpenter


Nice!

A really naive:
@r@
expression m;
identifier alloc_fn =~ "devm_";
identifier list_fn =~ "list_add";
@@

*    m = alloc_fn(...);
     ...
*    list_fn(..., <+... m ...+>, ...);


also seems to give some decent results.
Written as-is, there is a lot of false positive, but is a start.

I'll give it a look.


BTW, in such cases, should a Suggested-by, or something else be added?

CJ



One of my first match is:diff -u -p drivers/perf/qcom_l2_pmu.c /tmp/nothing/perf/qcom_l2_pmu.c
--- drivers/perf/qcom_l2_pmu.c
+++ /tmp/nothing/perf/qcom_l2_pmu.c
@@ -852,12 +852,10 @@ static int l2_cache_pmu_probe_cluster(st
          return err;
      }

-    cluster = devm_kzalloc(&pdev->dev, sizeof(*cluster), GFP_KERNEL);
      if (!cluster)
          return -ENOMEM;

      INIT_LIST_HEAD(&cluster->next);
-    list_add(&cluster->next, &l2cache_pmu->clusters);
      cluster->cluster_id = fw_cluster_id;

      irq = platform_get_irq(sdev, 0);

which looks like a real one.

list_add() should be at the very end of the probe in order not to "corrupt" the l2cache_pmu list.



Adding Julia if she wishes to dig it a little more with coccinelle.

CJ




[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux