From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
To: Dan Carpenter <dan.carpenter@oracle.com>,
Christian Lamparter <chunkeey@gmail.com>
Cc: Kalle Valo <kvalo@kernel.org>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
"John W. Linville" <linville@tuxdriver.com>,
linux-kernel <linux-kernel@vger.kernel.org>,
kernel-janitors@vger.kernel.org,
linux-wireless <linux-wireless@vger.kernel.org>,
Netdev <netdev@vger.kernel.org>,
Johannes Berg <johannes@sipsolutions.net>
Subject: Re: [PATCH v2] p54: Fix an error handling path in p54spi_probe()
Date: Thu, 16 Jun 2022 21:35:04 +0200 [thread overview]
Message-ID: <70ea2718-4979-5587-7f31-2361ae3ff8ad@wanadoo.fr> (raw)
In-Reply-To: <20220616151948.GD16517@kadam>
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
>
>
next prev parent reply other threads:[~2022-06-16 19:35 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-12 21:12 [PATCH v2] p54: Fix an error handling path in p54spi_probe() Christophe JAILLET
2022-06-13 20:02 ` Christian Lamparter
2022-06-13 20:57 ` Christophe JAILLET
2022-06-14 6:15 ` Kalle Valo
2022-06-14 7:25 ` Dan Carpenter
2022-06-15 21:03 ` Christian Lamparter
2022-06-15 21:12 ` Johannes Berg
2022-06-16 10:36 ` Dan Carpenter
2022-06-16 13:13 ` Christian Lamparter
2022-06-16 15:19 ` Dan Carpenter
2022-06-16 19:35 ` Christophe JAILLET [this message]
2022-07-18 11:51 ` [v2] wifi: " Kalle Valo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=70ea2718-4979-5587-7f31-2361ae3ff8ad@wanadoo.fr \
--to=christophe.jaillet@wanadoo.fr \
--cc=chunkeey@gmail.com \
--cc=dan.carpenter@oracle.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=johannes@sipsolutions.net \
--cc=kernel-janitors@vger.kernel.org \
--cc=kuba@kernel.org \
--cc=kvalo@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=linville@tuxdriver.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.