Re: [PATCH v2] p54: Fix an error handling path in p54spi_probe()

[Dan Carpenter]

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
my guess is that it has a lot of false positives?

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