Messages in this thread | | | Date | Thu, 16 Jun 2022 21:35:04 +0200 | Subject | Re: [PATCH v2] p54: Fix an error handling path in p54spi_probe() | From | Christophe JAILLET <> |
| |
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 > >
| |