On Thu, 18 May 2023, Christophe JAILLET wrote:
> 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.
Here is an attempt that is a bit more defensive than Christophe's:
@@
identifier d =~ "devm";
identifier x;
expression e;
int r != 0;
global idexpression f;
iterator i;
@@
* e = d(...)
...
(
* list_add_tail(&e->x, &f)
|
* list_add(&e->x, &f)
)
... when != list_del(&e->x)
when != i(...) { ... list_del(&e->x) ... }
return r;
Perhaps Christophe's more general list_add pattern is a good idea.
This has false positives when the return value is a variable that actually
contains 0. These false positives are easy to detect.
This rule requires that the list is a global variable. That constraint
implies that the rule gives a small number of results that are "easy" to
deal with.
Afterwards, the question is what does one do to deal with them? If you
remove the line when != i(...) { ... list_del(&e->x) ... }, then one of
the results does have a loop at the end of the function that deletes all
the elements in the list. That is fine as a solution if the list started
out empty. Otherwise, I guess one would have to collect another list of
things added to the list along the way, to be able to remove them all in
the failure case.
If the list_add can be moved to a safer place, as Christophe suggests,
then that also seems like a good solution. But often the allocation and
list_add are in a loop.
There are a lot more results if one replaces the two occurrences of &f by
... The typical result has the list being &a->b where a is a parameter.
Then one would have to trace up the nested callers to check that they are
all destroyed on failure. That seems painful to do manually, and probably
painful to do with Coccinelle. Does there exist some smatch code to do
that?
julia
>
>
>
> 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
>
