| Message ID | 297d2547ff2ee627731662abceeab9dbdaf23231.1655068321.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] p54: Fix an error handling path in p54spi_probe() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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 >
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 >> >
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.
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
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
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
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
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
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
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 > >
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()
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);
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(-)