* [PATCH] carl9170: fix leaks at failure path in carl9170_usb_probe()
@ 2013-09-28 3:51 Alexey Khoroshilov
2013-09-28 4:17 ` Fabio Estevam
0 siblings, 1 reply; 5+ messages in thread
From: Alexey Khoroshilov @ 2013-09-28 3:51 UTC (permalink / raw)
To: Christian Lamparter
Cc: Alexey Khoroshilov, John W. Linville, linux-wireless, netdev,
linux-kernel, ldv-project
carl9170_usb_probe() does not handle request_firmware_nowait() failure
that leads to several leaks in this case.
The patch adds all required deallocations.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
drivers/net/wireless/ath/carl9170/usb.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/carl9170/usb.c b/drivers/net/wireless/ath/carl9170/usb.c
index 307bc0d..3c76de1 100644
--- a/drivers/net/wireless/ath/carl9170/usb.c
+++ b/drivers/net/wireless/ath/carl9170/usb.c
@@ -1076,8 +1076,14 @@ static int carl9170_usb_probe(struct usb_interface *intf,
carl9170_set_state(ar, CARL9170_STOPPED);
- return request_firmware_nowait(THIS_MODULE, 1, CARL9170FW_NAME,
+ err = request_firmware_nowait(THIS_MODULE, 1, CARL9170FW_NAME,
&ar->udev->dev, GFP_KERNEL, ar, carl9170_usb_firmware_step2);
+ if (err) {
+ usb_put_dev(udev);
+ usb_put_dev(udev);
+ carl9170_free(ar);
+ }
+ return err;
}
static void carl9170_usb_disconnect(struct usb_interface *intf)
--
1.8.1.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] carl9170: fix leaks at failure path in carl9170_usb_probe()
2013-09-28 3:51 [PATCH] carl9170: fix leaks at failure path in carl9170_usb_probe() Alexey Khoroshilov
@ 2013-09-28 4:17 ` Fabio Estevam
2013-09-28 5:16 ` Alexey Khoroshilov
0 siblings, 1 reply; 5+ messages in thread
From: Fabio Estevam @ 2013-09-28 4:17 UTC (permalink / raw)
To: Alexey Khoroshilov
Cc: Christian Lamparter, John W. Linville, linux-wireless,
netdev@vger.kernel.org, linux-kernel, ldv-project
On Sat, Sep 28, 2013 at 12:51 AM, Alexey Khoroshilov
<khoroshilov@ispras.ru> wrote:
> - return request_firmware_nowait(THIS_MODULE, 1, CARL9170FW_NAME,
> + err = request_firmware_nowait(THIS_MODULE, 1, CARL9170FW_NAME,
> &ar->udev->dev, GFP_KERNEL, ar, carl9170_usb_firmware_step2);
> + if (err) {
> + usb_put_dev(udev);
> + usb_put_dev(udev);
You are doing the same free twice.
I guess you meant to also free: usb_put_dev(ar->udev)
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] carl9170: fix leaks at failure path in carl9170_usb_probe()
2013-09-28 4:17 ` Fabio Estevam
@ 2013-09-28 5:16 ` Alexey Khoroshilov
2013-10-10 17:59 ` John W. Linville
0 siblings, 1 reply; 5+ messages in thread
From: Alexey Khoroshilov @ 2013-09-28 5:16 UTC (permalink / raw)
To: Fabio Estevam
Cc: Christian Lamparter, John W. Linville, linux-wireless,
netdev@vger.kernel.org, linux-kernel, ldv-project
On 28.09.2013 00:17, Fabio Estevam wrote:
> On Sat, Sep 28, 2013 at 12:51 AM, Alexey Khoroshilov
> <khoroshilov@ispras.ru> wrote:
>
>> - return request_firmware_nowait(THIS_MODULE, 1, CARL9170FW_NAME,
>> + err = request_firmware_nowait(THIS_MODULE, 1, CARL9170FW_NAME,
>> &ar->udev->dev, GFP_KERNEL, ar, carl9170_usb_firmware_step2);
>> + if (err) {
>> + usb_put_dev(udev);
>> + usb_put_dev(udev);
> You are doing the same free twice.
Yes, because it was get twice.
> I guess you meant to also free: usb_put_dev(ar->udev)
udev and ar->udev are equal, so technically the patch is correct.
I agree that there is some inconsistency, but I would prefer to fix it
at usb_get_dev() side with a comment about reasons for the double get.
--
Alexey
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] carl9170: fix leaks at failure path in carl9170_usb_probe()
2013-09-28 5:16 ` Alexey Khoroshilov
@ 2013-10-10 17:59 ` John W. Linville
2013-10-10 18:17 ` Christian Lamparter
0 siblings, 1 reply; 5+ messages in thread
From: John W. Linville @ 2013-10-10 17:59 UTC (permalink / raw)
To: Alexey Khoroshilov
Cc: Fabio Estevam, Christian Lamparter, linux-wireless,
netdev@vger.kernel.org, linux-kernel, ldv-project
On Sat, Sep 28, 2013 at 01:16:20AM -0400, Alexey Khoroshilov wrote:
> On 28.09.2013 00:17, Fabio Estevam wrote:
> >On Sat, Sep 28, 2013 at 12:51 AM, Alexey Khoroshilov
> ><khoroshilov@ispras.ru> wrote:
> >
> >>- return request_firmware_nowait(THIS_MODULE, 1, CARL9170FW_NAME,
> >>+ err = request_firmware_nowait(THIS_MODULE, 1, CARL9170FW_NAME,
> >> &ar->udev->dev, GFP_KERNEL, ar, carl9170_usb_firmware_step2);
> >>+ if (err) {
> >>+ usb_put_dev(udev);
> >>+ usb_put_dev(udev);
> >You are doing the same free twice.
> Yes, because it was get twice.
> >I guess you meant to also free: usb_put_dev(ar->udev)
> udev and ar->udev are equal, so technically the patch is correct.
>
> I agree that there is some inconsistency, but I would prefer to fix
> it at usb_get_dev() side with a comment about reasons for the double
> get.
What is the reason for the double get?
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] carl9170: fix leaks at failure path in carl9170_usb_probe()
2013-10-10 17:59 ` John W. Linville
@ 2013-10-10 18:17 ` Christian Lamparter
0 siblings, 0 replies; 5+ messages in thread
From: Christian Lamparter @ 2013-10-10 18:17 UTC (permalink / raw)
To: John W. Linville
Cc: Alexey Khoroshilov, Fabio Estevam, linux-wireless,
netdev@vger.kernel.org, linux-kernel, ldv-project
On Thursday, October 10, 2013 01:59:52 PM John W. Linville wrote:
> On Sat, Sep 28, 2013 at 01:16:20AM -0400, Alexey Khoroshilov wrote:
> > On 28.09.2013 00:17, Fabio Estevam wrote:
> > >On Sat, Sep 28, 2013 at 12:51 AM, Alexey Khoroshilov
> > ><khoroshilov@ispras.ru> wrote:
> > >
> > >>- return request_firmware_nowait(THIS_MODULE, 1, CARL9170FW_NAME,
> > >>+ err = request_firmware_nowait(THIS_MODULE, 1, CARL9170FW_NAME,
> > >> &ar->udev->dev, GFP_KERNEL, ar, carl9170_usb_firmware_step2);
> > >>+ if (err) {
> > >>+ usb_put_dev(udev);
> > >>+ usb_put_dev(udev);
> > >You are doing the same free twice.
> > Yes, because it was get twice.
> > >I guess you meant to also free: usb_put_dev(ar->udev)
> > udev and ar->udev are equal, so technically the patch is correct.
> >
> > I agree that there is some inconsistency, but I would prefer to fix
> > it at usb_get_dev() side with a comment about reasons for the double
> > get.
>
> What is the reason for the double get?
The idea is:
One (extra) reference protects the asynchronous firmware loader callback
from disappearing "udev".
Regards,
Chr
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-10-10 18:17 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-28 3:51 [PATCH] carl9170: fix leaks at failure path in carl9170_usb_probe() Alexey Khoroshilov
2013-09-28 4:17 ` Fabio Estevam
2013-09-28 5:16 ` Alexey Khoroshilov
2013-10-10 17:59 ` John W. Linville
2013-10-10 18:17 ` Christian Lamparter
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).