Le 16/06/2022 à 17:19, Dan Carpenter a écrit :
On Thu, Jun 16, 2022 at 03:13:26PM +0200, Christian Lamparter wrote:
On 16/06/2022 12:36, Dan Carpenter wrote:
If it deserves a v3 to axe some lines of code, I can do it but, as said
previously,
v1 is for me the cleaner and more future proof.
Gee, that last sentence about "future proof" is daring.
The future is vast and unknowable but one thing which is pretty likely
is that Christophe's patch will introduce a static checker warning. We
really would have expected a to find a release_firmware() in the place
where it was in the original code. There is a comment there now so no
one is going to re-add the release_firmware() but that's been an issue
in the past.
I'm sort of surprised that it wasn't a static checker warning already.
Anyway, I'll add this to Smatch check_unwind.c
+ { "request_firmware", ALLOC, 0, "*$", &int_zero, &int_zero},
+ { "release_firmware", RELEASE, 0, "$"},
hmm? I don't follow you there. Why should there be a warning "now"?
(I assume you mean with v2 but not with v1?).
Yep. Generally, static checkers assume that functions clean up after
themselves on error paths so there would be a warning in
p54spi_request_firmware(). This is the easiest kind of static analysis
to implement and it's the way most kernel error handling is written.
If it's because the static
checker can't look beyond the function scope then this would be bad news
since on the "success" path the firmware will stick around until
p54spi_remove().
Presumably Christophe found this bug with static analysis already but
True, I use a coccinelle script that looks at functions called in
.remove() functions that are not called in what looks like an error
handling path in the corresponding probe.
my guess is that it has a lot of false positives?
This is SOOOO true !
The output is 23k LoC, mostly false positive!
In fact I only checks the diff between the outputs of my coccinelle
script from time to time.
Looking at only the diff, most of the false positives get ignored and I
manage to spot ~5-10 issues of this kind in each dev cycle in new code.
CJ
Eventually the leak in the probe function would be found with static
analysis as well. The truth is that there are a lot of leaks so I'm
already a bit overwhelmed fixing the ones that I know about.
It would be fairly simple to make a high quality resource leak checker
which is specific to probe functions. But the thing is that leaks in
probe functions are not really exploitable. Also some devices are
needed for the system to boot so often the devs don't care about about
cleaning up... My motivation is low.
regards,
dan carpenter
