linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] p54: Fix an error handling path in p54spi_probe()
@ 2022-06-12 21:12 Christophe JAILLET
  2022-06-13 20:02 ` Christian Lamparter
  2022-07-18 11:51 ` [v2] wifi: " Kalle Valo
  0 siblings, 2 replies; 12+ messages in thread
From: Christophe JAILLET @ 2022-06-12 21:12 UTC (permalink / raw)
  To: Christian Lamparter, Kalle Valo, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, John W. Linville
  Cc: linux-kernel, kernel-janitors, Christophe JAILLET,
	Christian Lamparter, linux-wireless, netdev

If an error occurs after a successful call to p54spi_request_firmware(), it
must be undone by a corresponding release_firmware() as already done in
the error handling path of p54spi_request_firmware() and in the .remove()
function.

Add the missing call in the error handling path and remove it from
p54spi_request_firmware() now that it is the responsibility of the caller
to release the firmawre

Fixes: cd8d3d321285 ("p54spi: p54spi driver")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
v2: reduce diffstat and take advantage on the fact that release_firmware()
checks for NULL
---
 drivers/net/wireless/intersil/p54/p54spi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/intersil/p54/p54spi.c b/drivers/net/wireless/intersil/p54/p54spi.c
index f99b7ba69fc3..19152fd449ba 100644
--- a/drivers/net/wireless/intersil/p54/p54spi.c
+++ b/drivers/net/wireless/intersil/p54/p54spi.c
@@ -164,7 +164,7 @@ static int p54spi_request_firmware(struct ieee80211_hw *dev)
 
 	ret = p54_parse_firmware(dev, priv->firmware);
 	if (ret) {
-		release_firmware(priv->firmware);
+		/* the firmware is released by the caller */
 		return ret;
 	}
 
@@ -659,6 +659,7 @@ static int p54spi_probe(struct spi_device *spi)
 	return 0;
 
 err_free_common:
+	release_firmware(priv->firmware);
 	free_irq(gpio_to_irq(p54spi_gpio_irq), spi);
 err_free_gpio_irq:
 	gpio_free(p54spi_gpio_irq);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] p54: Fix an error handling path in p54spi_probe()
  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-07-18 11:51 ` [v2] wifi: " Kalle Valo
  1 sibling, 1 reply; 12+ messages in thread
From: Christian Lamparter @ 2022-06-13 20:02 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: Kalle Valo, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, John W. Linville, linux-kernel, kernel-janitors,
	Christian Lamparter, linux-wireless, Netdev

On Sun, Jun 12, 2022 at 11:12 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> If an error occurs after a successful call to p54spi_request_firmware(), it
> must be undone by a corresponding release_firmware() as already done in
> the error handling path of p54spi_request_firmware() and in the .remove()
> function.
>
> Add the missing call in the error handling path and remove it from
> p54spi_request_firmware() now that it is the responsibility of the caller
> to release the firmawre

that last word hast a typo:  firmware. (maybe Kalle can fix this in post).

> Fixes: cd8d3d321285 ("p54spi: p54spi driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Acked-by: Christian Lamparter <chunkeey@gmail.com>
(Though, v1 was fine too.)
> ---
> v2: reduce diffstat and take advantage on the fact that release_firmware()
> checks for NULL

Heh, ok ;) . Now that I see it,  the "ret = p54_parse_firmware(...); ... "
could have been replaced with "return p54_parse_firmware(dev, priv->firmware);"
so the p54spi.c could shrink another 5-6 lines.

I think leaving p54spi_request_firmware() callee to deal with
releasing the firmware
in the error case as well is nicer because it gets rid of a "but in
this case" complexity.

(I still have hope for the devres-firmware to hit some day).

Cheers
Christian

> ---
>  drivers/net/wireless/intersil/p54/p54spi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/intersil/p54/p54spi.c b/drivers/net/wireless/intersil/p54/p54spi.c
> index f99b7ba69fc3..19152fd449ba 100644
> --- a/drivers/net/wireless/intersil/p54/p54spi.c
> +++ b/drivers/net/wireless/intersil/p54/p54spi.c
> @@ -164,7 +164,7 @@ static int p54spi_request_firmware(struct ieee80211_hw *dev)
>
>         ret = p54_parse_firmware(dev, priv->firmware);
>         if (ret) {
> -               release_firmware(priv->firmware);
> +               /* the firmware is released by the caller */
>                 return ret;
>         }
>
> @@ -659,6 +659,7 @@ static int p54spi_probe(struct spi_device *spi)
>         return 0;
>
>  err_free_common:
> +       release_firmware(priv->firmware);
>         free_irq(gpio_to_irq(p54spi_gpio_irq), spi);
>  err_free_gpio_irq:
>         gpio_free(p54spi_gpio_irq);
> --
> 2.34.1
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] p54: Fix an error handling path in p54spi_probe()
  2022-06-13 20:02 ` Christian Lamparter
@ 2022-06-13 20:57   ` Christophe JAILLET
  2022-06-14  6:15     ` Kalle Valo
                       ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Christophe JAILLET @ 2022-06-13 20:57 UTC (permalink / raw)
  To: Christian Lamparter
  Cc: Kalle Valo, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, John W. Linville, linux-kernel, kernel-janitors,
	Christian Lamparter, linux-wireless, Netdev

Le 13/06/2022 à 22:02, Christian Lamparter a écrit :
> On Sun, Jun 12, 2022 at 11:12 PM Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
>>
>> If an error occurs after a successful call to p54spi_request_firmware(), it
>> must be undone by a corresponding release_firmware() as already done in
>> the error handling path of p54spi_request_firmware() and in the .remove()
>> function.
>>
>> Add the missing call in the error handling path and remove it from
>> p54spi_request_firmware() now that it is the responsibility of the caller
>> to release the firmawre
> 
> that last word hast a typo:  firmware. (maybe Kalle can fix this in post).

More or less the same typo twice in a row... _Embarrassed_

> 
>> Fixes: cd8d3d321285 ("p54spi: p54spi driver")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Acked-by: Christian Lamparter <chunkeey@gmail.com>
> (Though, v1 was fine too.)
>> ---
>> v2: reduce diffstat and take advantage on the fact that release_firmware()
>> checks for NULL
> 
> Heh, ok ;) . Now that I see it,  the "ret = p54_parse_firmware(...); ... "
> could have been replaced with "return p54_parse_firmware(dev, priv->firmware);"
> so the p54spi.c could shrink another 5-6 lines.
> 
> I think leaving p54spi_request_firmware() callee to deal with
> releasing the firmware
> in the error case as well is nicer because it gets rid of a "but in
> this case" complexity.


Take the one you consider being the best one.

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.

CJ

> 
> (I still have hope for the devres-firmware to hit some day).
> 
> Cheers
> Christian
> 
>> ---
>>   drivers/net/wireless/intersil/p54/p54spi.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/intersil/p54/p54spi.c b/drivers/net/wireless/intersil/p54/p54spi.c
>> index f99b7ba69fc3..19152fd449ba 100644
>> --- a/drivers/net/wireless/intersil/p54/p54spi.c
>> +++ b/drivers/net/wireless/intersil/p54/p54spi.c
>> @@ -164,7 +164,7 @@ static int p54spi_request_firmware(struct ieee80211_hw *dev)
>>
>>          ret = p54_parse_firmware(dev, priv->firmware);
>>          if (ret) {
>> -               release_firmware(priv->firmware);
>> +               /* the firmware is released by the caller */
>>                  return ret;
>>          }
>>
>> @@ -659,6 +659,7 @@ static int p54spi_probe(struct spi_device *spi)
>>          return 0;
>>
>>   err_free_common:
>> +       release_firmware(priv->firmware);
>>          free_irq(gpio_to_irq(p54spi_gpio_irq), spi);
>>   err_free_gpio_irq:
>>          gpio_free(p54spi_gpio_irq);
>> --
>> 2.34.1
>>
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] p54: Fix an error handling path in p54spi_probe()
  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
  2 siblings, 0 replies; 12+ messages in thread
From: Kalle Valo @ 2022-06-14  6:15 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: Christian Lamparter, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, John W. Linville, linux-kernel,
	kernel-janitors, Christian Lamparter, linux-wireless, Netdev

Christophe JAILLET <christophe.jaillet@wanadoo.fr> writes:

> Le 13/06/2022 à 22:02, Christian Lamparter a écrit :
>> On Sun, Jun 12, 2022 at 11:12 PM Christophe JAILLET
>> <christophe.jaillet@wanadoo.fr> wrote:
>>>
>>> If an error occurs after a successful call to p54spi_request_firmware(), it
>>> must be undone by a corresponding release_firmware() as already done in
>>> the error handling path of p54spi_request_firmware() and in the .remove()
>>> function.
>>>
>>> Add the missing call in the error handling path and remove it from
>>> p54spi_request_firmware() now that it is the responsibility of the caller
>>> to release the firmawre
>>
>> that last word hast a typo:  firmware. (maybe Kalle can fix this in post).
>
> More or less the same typo twice in a row... _Embarrassed_

No worries, I can fix the typo.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] p54: Fix an error handling path in p54spi_probe()
  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
  2 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2022-06-14  7:25 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: Christian Lamparter, Kalle Valo, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, John W. Linville, linux-kernel,
	kernel-janitors, Christian Lamparter, linux-wireless, Netdev

On Mon, Jun 13, 2022 at 10:57:25PM +0200, Christophe JAILLET wrote:
> > > ---
> > > v2: reduce diffstat and take advantage on the fact that release_firmware()
> > > checks for NULL
> > 
> > Heh, ok ;) . Now that I see it,  the "ret = p54_parse_firmware(...); ... "
> > could have been replaced with "return p54_parse_firmware(dev, priv->firmware);"
> > so the p54spi.c could shrink another 5-6 lines.
> > 
> > I think leaving p54spi_request_firmware() callee to deal with
> > releasing the firmware
> > in the error case as well is nicer because it gets rid of a "but in
> > this case" complexity.
> 
> 
> Take the one you consider being the best one.
> 
> 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.
> 

I prefered v1 but with s/firmaware/firmware/...

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] p54: Fix an error handling path in p54spi_probe()
  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
  2 siblings, 2 replies; 12+ messages in thread
From: Christian Lamparter @ 2022-06-15 21:03 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: Kalle Valo, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, John W. Linville, linux-kernel, kernel-janitors,
	Christian Lamparter, linux-wireless, Netdev

On 13/06/2022 22:57, Christophe JAILLET wrote:
> Le 13/06/2022 à 22:02, Christian Lamparter a écrit :
>> On Sun, Jun 12, 2022 at 11:12 PM Christophe JAILLET
>> <christophe.jaillet@wanadoo.fr> wrote:
>>>
>>> If an error occurs after a successful call to p54spi_request_firmware(), it
>>> must be undone by a corresponding release_firmware() as already done in
>>> the error handling path of p54spi_request_firmware() and in the .remove()
>>> function.
>>>
>>> Add the missing call in the error handling path and remove it from
>>> p54spi_request_firmware() now that it is the responsibility of the caller
>>> to release the firmawre
>>
>> that last word hast a typo:  firmware. (maybe Kalle can fix this in post).
> 
> More or less the same typo twice in a row... _Embarrassed_
> 
>>
>>> Fixes: cd8d3d321285 ("p54spi: p54spi driver")
>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> Acked-by: Christian Lamparter <chunkeey@gmail.com>
>> (Though, v1 was fine too.)
>>> ---
>>> v2: reduce diffstat and take advantage on the fact that release_firmware()
>>> checks for NULL
>>
>> Heh, ok ;) . Now that I see it,  the "ret = p54_parse_firmware(...); ... "
>> could have been replaced with "return p54_parse_firmware(dev, priv->firmware);"
>> so the p54spi.c could shrink another 5-6 lines.
>>
>> I think leaving p54spi_request_firmware() callee to deal with
>> releasing the firmware
>> in the error case as well is nicer because it gets rid of a "but in
>> this case" complexity.
> 
> 
> Take the one you consider being the best one.

well said!

> 
> 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.
I don't know what's up on the horizon. For my part, I've been devresing
parts of carl9170 and now thinking about it. Because the various
request_firmware*() functions could be a target for devres too.
A driver usually loads the firmware in .probe(). It stays around because
of .suspend()+.resume() and gets freed by .release().
With devresing up request_firmware(), that release_firmware() would be
rendered obsolete in all of p54* cases.

There must be something that I have missed? right?

It's because there's already an extensive list of managed interfaces:
<https://www.kernel.org/doc/html/latest/driver-api/driver-model/devres.html>
But the firmware_class is not on it. Does somebody know the presumably
"very good" reason why not? I can't believe that this hasn't been done yet.

Regards,
Christian

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] p54: Fix an error handling path in p54spi_probe()
  2022-06-15 21:03     ` Christian Lamparter
@ 2022-06-15 21:12       ` Johannes Berg
  2022-06-16 10:36       ` Dan Carpenter
  1 sibling, 0 replies; 12+ messages in thread
From: Johannes Berg @ 2022-06-15 21:12 UTC (permalink / raw)
  To: Christian Lamparter, Christophe JAILLET
  Cc: Kalle Valo, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, John W. Linville, linux-kernel, kernel-janitors,
	Christian Lamparter, linux-wireless, Netdev

On Wed, 2022-06-15 at 23:03 +0200, Christian Lamparter wrote:
> 
> A driver usually loads the firmware in .probe(). It stays around because
> of .suspend()+.resume()
> 

Does it, though? the firmware cache thing is a bit odd, it sometimes
seems to me that it only re-requests/loads the firmware when suspending,
and drops the cache when resumed, just in case it's requested inbetween.

johannes

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] p54: Fix an error handling path in p54spi_probe()
  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
  1 sibling, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2022-06-16 10:36 UTC (permalink / raw)
  To: Christian Lamparter
  Cc: Christophe JAILLET, Kalle Valo, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, John W. Linville, linux-kernel,
	kernel-janitors, Christian Lamparter, linux-wireless, Netdev

On Wed, Jun 15, 2022 at 11:03:34PM +0200, Christian Lamparter wrote:
> On 13/06/2022 22:57, Christophe JAILLET wrote:
> > Le 13/06/2022 à 22:02, Christian Lamparter a écrit :
> > > On Sun, Jun 12, 2022 at 11:12 PM Christophe JAILLET
> > > <christophe.jaillet@wanadoo.fr> wrote:
> > > > 
> > > > If an error occurs after a successful call to p54spi_request_firmware(), it
> > > > must be undone by a corresponding release_firmware() as already done in
> > > > the error handling path of p54spi_request_firmware() and in the .remove()
> > > > function.
> > > > 
> > > > Add the missing call in the error handling path and remove it from
> > > > p54spi_request_firmware() now that it is the responsibility of the caller
> > > > to release the firmawre
> > > 
> > > that last word hast a typo:  firmware. (maybe Kalle can fix this in post).
> > 
> > More or less the same typo twice in a row... _Embarrassed_
> > 
> > > 
> > > > Fixes: cd8d3d321285 ("p54spi: p54spi driver")
> > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > Acked-by: Christian Lamparter <chunkeey@gmail.com>
> > > (Though, v1 was fine too.)
> > > > ---
> > > > v2: reduce diffstat and take advantage on the fact that release_firmware()
> > > > checks for NULL
> > > 
> > > Heh, ok ;) . Now that I see it,  the "ret = p54_parse_firmware(...); ... "
> > > could have been replaced with "return p54_parse_firmware(dev, priv->firmware);"
> > > so the p54spi.c could shrink another 5-6 lines.
> > > 
> > > I think leaving p54spi_request_firmware() callee to deal with
> > > releasing the firmware
> > > in the error case as well is nicer because it gets rid of a "but in
> > > this case" complexity.
> > 
> > 
> > Take the one you consider being the best one.
> 
> well said!
> 
> > 
> > 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, "$"},

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] p54: Fix an error handling path in p54spi_probe()
  2022-06-16 10:36       ` Dan Carpenter
@ 2022-06-16 13:13         ` Christian Lamparter
  2022-06-16 15:19           ` Dan Carpenter
  0 siblings, 1 reply; 12+ messages in thread
From: Christian Lamparter @ 2022-06-16 13:13 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Christophe JAILLET, Kalle Valo, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, John W. Linville, linux-kernel,
	kernel-janitors, linux-wireless, Netdev, Johannes Berg

On 16/06/2022 12:36, Dan Carpenter wrote:
> On Wed, Jun 15, 2022 at 11:03:34PM +0200, Christian Lamparter wrote:
>> On 13/06/2022 22:57, Christophe JAILLET wrote:
>>> Le 13/06/2022 à 22:02, Christian Lamparter a écrit :
>>>> On Sun, Jun 12, 2022 at 11:12 PM Christophe JAILLET
>>>> <christophe.jaillet@wanadoo.fr> wrote:
>>>>>
>>>>> If an error occurs after a successful call to p54spi_request_firmware(), it
>>>>> must be undone by a corresponding release_firmware() as already done in
>>>>> the error handling path of p54spi_request_firmware() and in the .remove()
>>>>> function.
>>>>>
>>>>> Add the missing call in the error handling path and remove it from
>>>>> p54spi_request_firmware() now that it is the responsibility of the caller
>>>>> to release the firmawre
>>>>
>>>> that last word hast a typo:  firmware. (maybe Kalle can fix this in post).
>>>
>>> More or less the same typo twice in a row... _Embarrassed_
>>>
>>>>
>>>>> Fixes: cd8d3d321285 ("p54spi: p54spi driver")
>>>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>>> Acked-by: Christian Lamparter <chunkeey@gmail.com>
>>>> (Though, v1 was fine too.)
>>>>> ---
>>>>> v2: reduce diffstat and take advantage on the fact that release_firmware()
>>>>> checks for NULL
>>>>
>>>> Heh, ok ;) . Now that I see it,  the "ret = p54_parse_firmware(...); ... "
>>>> could have been replaced with "return p54_parse_firmware(dev, priv->firmware);"
>>>> so the p54spi.c could shrink another 5-6 lines.
>>>>
>>>> I think leaving p54spi_request_firmware() callee to deal with
>>>> releasing the firmware
>>>> in the error case as well is nicer because it gets rid of a "but in
>>>> this case" complexity.
>>>
>>>
>>> Take the one you consider being the best one.
>>
>> well said!
>>
>>>
>>> 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?). 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().

The p54* drivers would need to be updated (they are ooold) to make good
use of the firmware_cache. There must have been a good reason why it
was designed that way. Especially because the firmware_class uses devm
for the cache already internally. (thanks for Johannes for pointing this
out).

Cheers,
Christian

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] p54: Fix an error handling path in p54spi_probe()
  2022-06-16 13:13         ` Christian Lamparter
@ 2022-06-16 15:19           ` Dan Carpenter
  2022-06-16 19:35             ` Christophe JAILLET
  0 siblings, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2022-06-16 15:19 UTC (permalink / raw)
  To: Christian Lamparter
  Cc: Christophe JAILLET, Kalle Valo, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, John W. Linville, linux-kernel,
	kernel-janitors, linux-wireless, Netdev, Johannes Berg

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] p54: Fix an error handling path in p54spi_probe()
  2022-06-16 15:19           ` Dan Carpenter
@ 2022-06-16 19:35             ` Christophe JAILLET
  0 siblings, 0 replies; 12+ messages in thread
From: Christophe JAILLET @ 2022-06-16 19:35 UTC (permalink / raw)
  To: Dan Carpenter, Christian Lamparter
  Cc: Kalle Valo, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, John W. Linville, linux-kernel, kernel-janitors,
	linux-wireless, Netdev, Johannes Berg

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
> 
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [v2] wifi: p54: Fix an error handling path in p54spi_probe()
  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-07-18 11:51 ` Kalle Valo
  1 sibling, 0 replies; 12+ messages in thread
From: Kalle Valo @ 2022-07-18 11:51 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: Christian Lamparter, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, John W. Linville, linux-kernel,
	kernel-janitors, Christophe JAILLET, Christian Lamparter,
	linux-wireless, netdev

Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:

> If an error occurs after a successful call to p54spi_request_firmware(), it
> must be undone by a corresponding release_firmware() as already done in
> the error handling path of p54spi_request_firmware() and in the .remove()
> function.
> 
> Add the missing call in the error handling path and remove it from
> p54spi_request_firmware() now that it is the responsibility of the caller
> to release the firmware
> 
> Fixes: cd8d3d321285 ("p54spi: p54spi driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Acked-by: Christian Lamparter <chunkeey@gmail.com>

Patch applied to wireless-next.git, thanks.

83781f0162d0 wifi: p54: Fix an error handling path in p54spi_probe()

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/297d2547ff2ee627731662abceeab9dbdaf23231.1655068321.git.christophe.jaillet@wanadoo.fr/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-07-18 11:51 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2022-07-18 11:51 ` [v2] wifi: " Kalle Valo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).