Re: [cocci] [PATCH] coccinelle: api: add a checker for device nodes in devm_ memory

Re: [cocci] [PATCH] coccinelle: api: add a checker for device nodes in devm_ memory

[Markus Elfring]

>> +@alloc depends on org || report exists@
> Since you only support org and report, you don't really need depends on
> org || report everywhere.

Would you get into the mood to collaborate with all known coccicheck operation modes?

Regards,
Markus

Re: [PATCH] coccinelle: api: add a checker for device nodes in devm_ memory

[Markus Elfring]

Thanks for your contribution.

How do you think about further collateral evolution according to the presented
source code analysis approach?


I suggest to use the word “devm” (without a trailing underscore) in the summary phrase
(and subsequent messages).

You propose something for device nodes.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/driver-api/driver-model/devres.rst?h=v7.4-rc1#n9


> A struct cdev, struct miscdevice, struct video_device or struct
> v4l2_device embedded in memory from devm_kzalloc() and friends is freed
> when the driver unbinds, but a file that user space holds open on the
> node outlives the unbind. Its later file operations are a use-after-free
> when they use that memory, and for a cdev or video_device so is its
> close(), where the core touches the object and the v4l2_device the
> video_device points to.
…

How many data structures should be touched after a close() call?

Are there more system dependencies to reconsider?


> +@initialize:python@
> +@@
> +seen = set()

Such a programming interface can look appealing.
https://gitlab.inria.fr/coccinelle/coccinelle/-/blob/ea58efe355dea2184217954248a01f5dc5995d24/docs/manual/cocci_syntax.tex#L697-700
https://docs.python.org/3/builtins/stdtypes.html#set

But further constraints should occasionally be taken better into account.
I guess you would like to benefit from parallel data processing with multiple
processor cores.


> +@alloc depends on org || report exists@
> +expression x, e, e2;
> +identifier fn =~ "^devm_k[mzcr]";
> +position p, q;
> +@@
> +
> +x = fn@p(...);
> +... when != x = e
> +(
> +misc_register@q(\(x\|&<+... x ...+>\))

Would such an SmPL disjunction be redundant for the passed argument expression?


> +|
> +cdev_add@q(\(x\|&<+... x ...+>\), ...)
> +|
> +cdev_device_add@q(\(x\|&<+... x ...+>\), ...)
> +|
> +video_register_device@q(\(x\|&<+... x ...+>\), ...)
> +|
> +video_register_device_no_warn@q(\(x\|&<+... x ...+>\), ...)
> +|
> +__video_register_device@q(\(x\|&<+... x ...+>\), ...)
> +|
> +v4l2_device_register@q(e2, \(x\|&<+... x ...+>\))
> +)

Were these function names ordered by their usage probability?


…
> +@script:python depends on report@
> +p << alloc.p;
> +q << alloc.q;
> +@@
> +if (p[0].file, p[0].line, p[0].column) not in seen:
> +    seen.add((p[0].file, p[0].line, p[0].column))
…

Can such a control flow pattern still be refined?
Would a temporary variable become helpful here?

Regards,
Markus