Re: [Cocci] [PATCH v3] coccinelle: semantic code search for missingof_node_put

[<wen.yang99@zte.com.cn>]

[-- Attachment #1.1: Type: text/plain, Size: 6283 bytes --]

Hi Markus,

> > We find these functions by using the following script:
> 
> Why would you like to keep this SmPL code in the commit description?
> 
> I would prefer software evolution in an other direction.
> https://lore.kernel.org/lkml/44be5924-26ca-5106-aa25-3cbc3343aa2c@web.de/
> https://lkml.org/lkml/2019/7/4/21
> > @initialize:ocaml@
> > @@
> >
> > let relevant_str = "use of_node_put() on it when done"
> 
> I see further possibilities to improve this data processing approach.
> https://lore.kernel.org/lkml/904b9362-cd01-ffc9-600b-0c48848617a0@web.de/
> https://lore.kernel.org/patchwork/patch/1095169/#1291378
> https://lkml.org/lkml/2019/6/28/326
> 
> 
> I am missing more constructive answers for mentioned development concerns.
> 

Let me try to guess what you mean:
1), Provides a SmPL A that parses the annotations of a particular kernel file and extracts a list of function names to be followed;
2), Then SmPL A generates another SmPL B based on the function name list;
3), Finally SmPL A executes SmPL B on the entire kernel code, looking for the missing of_node_put.
You expect the entire process above to be automated.

This idea may be interesting, but it can't be done now, and it will introduce uncontrollable factors. 

We agree with julia's comments:
I would prefer not to put semantic patches that involve iteration into the kernel, for simplicity.

> 
> > And this patch also looks for places …
> 
> Does a SmPL script perform an action?
> 

Thanks.
We'll continue to improve the description here.

> > Finally, this patch finds use-after-free issues for a node.
> > (implemented by the r_use_after_put rule)
> 
> This software extension is another interesting contribution.
> But I imagine that a separate SmPL script can be more helpful for
> this source code search pattern.

We found that adding the missing of_node_put may cause use-after-free
if not handled properly, so we have added a new r_use_after_put to detect it.

Our file is called of_node_put.cocci, which contains three rules: r_miss_put,
 r_miss_put_ext and r_use_after_put. 
If you separate them, it seems inappropriate.

> > v3: delete the global set, …
> 
> To which previous implementation detail do you refer here?

Here is an improvement based on julia's comments:
https://lkml.org/lkml/2019/7/5/55
We are very grateful to her. 
This is a real, valuable comment that can be applied in code practice.

> > +virtual report
> > +virtual org
> > +
> > +@initialize:python@
> > +@@
> > +
> > +report_miss_prefix = "ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line "
> > +report_miss_suffix = ", but without a corresponding object release within this function."
> > +org_miss_main = "acquired a node pointer with refcount incremented"
> > +org_miss_sec = "needed of_node_put"
> > +report_use_after_put = "ERROR: use-after-free; reference preceded by of_node_put on line "
> > +org_use_after_put_main = "of_node_put"
> > +org_use_after_put_sec = "reference"
> 
> If you would insist on the usage of these variables, they should be applied
> only for the selected analysis operation mode.
> I would expect corresponding SmPL dependency specifications.
> https://github.com/coccinelle/coccinelle/blob/b4509f6e7fb06d5616bb19dd5a110b203fd0e566/docs/manual/cocci_syntax.tex#L559
> 

Thanks.
Here are some improvements.

> > +@r_miss_put exists@
> > +local idexpression struct device_node *x;
> > +expression e, e1;
> > +position p1, p2;
> > +statement S;
> > +type T, T1;
> > +@@
> > +
> > +* x = @p1\(of_find_all_nodes\|
> 
> The usage of the SmPL asterisk functionality can fit to the operation mode “context”.
> https://bottest.wiki.kernel.org/coccicheck#modes
> Would you like to add any corresponding SmPL details?
> 
> Under which circumstances will remaining programming concerns be clarified
> for such SmPL disjunctions?

Adding an asterisk here is more convenient to use, 
it can mark the location of the code of interest, such as:

$ /usr/local/bin/spatch -D report  --cocci-file scripts/coccinelle/free/of_node_put.cocci arch/arm/mach-axxia/platsmp.c 
init_defs_builtins: /usr/local/bin/../lib/coccinelle/standard.h
HANDLING: arch/arm/mach-axxia/platsmp.c
arch/arm/mach-axxia/platsmp.c:43:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 37, but without a corresponding object release within this function.
arch/arm/mach-axxia/platsmp.c:50:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 37, but without a corresponding object release within this function.
diff = 
--- arch/arm/mach-axxia/platsmp.c
+++ /tmp/cocci-output-13026-88f3a1-platsmp.c
@@ -34,20 +34,17 @@ static int axxia_boot_secondary(unsigned
 	void __iomem *syscon;
 	u32 tmp;

-	syscon_np = of_find_compatible_node(NULL, NULL, "lsi,axxia-syscon");
 	if (!syscon_np)
 		return -ENOENT;

 	syscon = of_iomap(syscon_np, 0);
 	if (!syscon)
-		return -ENOMEM;

 	tmp = readl(syscon + SC_RST_CPU_HOLD);
 	writel(0xab, syscon + SC_CRIT_WRITE_KEY);
 	tmp &= ~(1 << cpu);
 	writel(tmp, syscon + SC_RST_CPU_HOLD);

-	return 0;
 }
...

In addition to reading the documentation,
you may also need to do some experiments yourself.
Thank you.

--
Regards,
Wen

> > +... when != e = (T)x
> > +    when != true x == NULL
> 
> Will assignment exclusions get any more software development attention?
> https://lore.kernel.org/lkml/03cc4df5-ce7f-ba91-36b5-687fec8c7297@web.de/
> https://lore.kernel.org/patchwork/patch/1095169/#1291892
> https://lkml.org/lkml/2019/6/29/193
> 
> 
> > +    when != of_node_put(x)
> …
> > +)
> > +&
> > +x = f(...)
> > +...
> > +if (<+...x...+>) S
> > +...
> > +of_node_put(x);
> > +)
> 
> You propose once more to use a SmPL conjunction in the rule “r_miss_put_ext”.
> I am also still waiting for a definitive explanation on the applicability
> of this combination.
> 
> 
> > +@r_put@
> > +expression E;
> > +position p1;
> > +@@
> > +
> > +* of_node_put@p1(E)
> 
> I guess that this SmPL code will need further adjustments.
> 
> 
> > +@r_use_after_put exists@
> > +expression r_put.E, subE<=r_put.E;
> 
> I have got an understanding difficulty around the interpretation
> of the shown SmPL constraint.
> How will the clarification be continued?
> 
> Regards,
> Markus

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci