* [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv @ 2021-05-21 12:08 Dongliang Mu 2021-05-21 12:09 ` 慕冬亮 2021-05-21 12:18 ` Greg KH 0 siblings, 2 replies; 13+ messages in thread From: Dongliang Mu @ 2021-05-21 12:08 UTC (permalink / raw) To: Larry.Finger, florian.c.schilhabel, gregkh, rkovhaev, straube.linux, linux-staging, linux-kernel Cc: Dongliang Mu, syzbot+1c46f3771695bccbdb3a r871xu_dev_remove failed to call r8712_free_drv_sw() and free the resource (e.g., struct urb) due to the failure of firmware loading. Fix this by invoking r8712_free_drv_sw at the failure site. Reported-by: syzbot+1c46f3771695bccbdb3a@syzkaller.appspotmail.com Fixes: b4383c971bc5 ("staging: rtl8712: handle firmware load failure") Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> --- drivers/staging/rtl8712/usb_intf.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/staging/rtl8712/usb_intf.c b/drivers/staging/rtl8712/usb_intf.c index dc21e7743349..a5190b4250ce 100644 --- a/drivers/staging/rtl8712/usb_intf.c +++ b/drivers/staging/rtl8712/usb_intf.c @@ -589,7 +589,7 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf, */ static void r871xu_dev_remove(struct usb_interface *pusb_intf) { - struct net_device *pnetdev = usb_get_intfdata(pusb_intf); + struct net_device *pnetdev, *newpnetdev = usb_get_intfdata(pusb_intf); struct usb_device *udev = interface_to_usbdev(pusb_intf); if (pnetdev) { @@ -597,9 +597,9 @@ static void r871xu_dev_remove(struct usb_interface *pusb_intf) /* never exit with a firmware callback pending */ wait_for_completion(&padapter->rtl8712_fw_ready); - pnetdev = usb_get_intfdata(pusb_intf); + newpnetdev = usb_get_intfdata(pusb_intf); usb_set_intfdata(pusb_intf, NULL); - if (!pnetdev) + if (!newpnetdev) goto firmware_load_fail; release_firmware(padapter->fw); if (drvpriv.drv_registered) @@ -625,6 +625,13 @@ static void r871xu_dev_remove(struct usb_interface *pusb_intf) */ if (udev->state != USB_STATE_NOTATTACHED) usb_reset_device(udev); + if (pnetdev) { + struct _adapter *padapter = netdev_priv(pnetdev); + /* Stop driver mlme relation timer */ + //r8712_stop_drv_timers(padapter); + //r871x_dev_unload(padapter); + r8712_free_drv_sw(padapter); + } } static int __init r8712u_drv_entry(void) -- 2.25.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv 2021-05-21 12:08 [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv Dongliang Mu @ 2021-05-21 12:09 ` 慕冬亮 2021-05-21 12:18 ` Greg KH 1 sibling, 0 replies; 13+ messages in thread From: 慕冬亮 @ 2021-05-21 12:09 UTC (permalink / raw) To: Larry.Finger, florian.c.schilhabel, Greg KH, rkovhaev, straube.linux, linux-staging, linux-kernel Cc: syzbot+1c46f3771695bccbdb3a On Fri, May 21, 2021 at 8:08 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > r871xu_dev_remove failed to call r8712_free_drv_sw() and free the > resource (e.g., struct urb) due to the failure of firmware loading. > > Fix this by invoking r8712_free_drv_sw at the failure site. > > Reported-by: syzbot+1c46f3771695bccbdb3a@syzkaller.appspotmail.com > Fixes: b4383c971bc5 ("staging: rtl8712: handle firmware load failure") > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > --- > drivers/staging/rtl8712/usb_intf.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/rtl8712/usb_intf.c b/drivers/staging/rtl8712/usb_intf.c > index dc21e7743349..a5190b4250ce 100644 > --- a/drivers/staging/rtl8712/usb_intf.c > +++ b/drivers/staging/rtl8712/usb_intf.c > @@ -589,7 +589,7 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf, > */ > static void r871xu_dev_remove(struct usb_interface *pusb_intf) > { > - struct net_device *pnetdev = usb_get_intfdata(pusb_intf); > + struct net_device *pnetdev, *newpnetdev = usb_get_intfdata(pusb_intf); > struct usb_device *udev = interface_to_usbdev(pusb_intf); > > if (pnetdev) { > @@ -597,9 +597,9 @@ static void r871xu_dev_remove(struct usb_interface *pusb_intf) > > /* never exit with a firmware callback pending */ > wait_for_completion(&padapter->rtl8712_fw_ready); > - pnetdev = usb_get_intfdata(pusb_intf); > + newpnetdev = usb_get_intfdata(pusb_intf); > usb_set_intfdata(pusb_intf, NULL); > - if (!pnetdev) > + if (!newpnetdev) > goto firmware_load_fail; > release_firmware(padapter->fw); > if (drvpriv.drv_registered) > @@ -625,6 +625,13 @@ static void r871xu_dev_remove(struct usb_interface *pusb_intf) > */ > if (udev->state != USB_STATE_NOTATTACHED) > usb_reset_device(udev); > + if (pnetdev) { > + struct _adapter *padapter = netdev_priv(pnetdev); > + /* Stop driver mlme relation timer */ > + //r8712_stop_drv_timers(padapter); > + //r871x_dev_unload(padapter); I am not sure if I should add those deallocation functions in this branch. I will appreciate it if anyone can give some advice here. > + r8712_free_drv_sw(padapter); > + } > } > > static int __init r8712u_drv_entry(void) > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv @ 2021-05-21 12:09 ` 慕冬亮 0 siblings, 0 replies; 13+ messages in thread From: 慕冬亮 @ 2021-05-21 12:09 UTC (permalink / raw) To: Larry.Finger, florian.c.schilhabel, Greg KH, rkovhaev, straube.linux, linux-staging, linux-kernel Cc: syzbot+1c46f3771695bccbdb3a On Fri, May 21, 2021 at 8:08 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > r871xu_dev_remove failed to call r8712_free_drv_sw() and free the > resource (e.g., struct urb) due to the failure of firmware loading. > > Fix this by invoking r8712_free_drv_sw at the failure site. > > Reported-by: syzbot+1c46f3771695bccbdb3a@syzkaller.appspotmail.com > Fixes: b4383c971bc5 ("staging: rtl8712: handle firmware load failure") > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > --- > drivers/staging/rtl8712/usb_intf.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/rtl8712/usb_intf.c b/drivers/staging/rtl8712/usb_intf.c > index dc21e7743349..a5190b4250ce 100644 > --- a/drivers/staging/rtl8712/usb_intf.c > +++ b/drivers/staging/rtl8712/usb_intf.c > @@ -589,7 +589,7 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf, > */ > static void r871xu_dev_remove(struct usb_interface *pusb_intf) > { > - struct net_device *pnetdev = usb_get_intfdata(pusb_intf); > + struct net_device *pnetdev, *newpnetdev = usb_get_intfdata(pusb_intf); > struct usb_device *udev = interface_to_usbdev(pusb_intf); > > if (pnetdev) { > @@ -597,9 +597,9 @@ static void r871xu_dev_remove(struct usb_interface *pusb_intf) > > /* never exit with a firmware callback pending */ > wait_for_completion(&padapter->rtl8712_fw_ready); > - pnetdev = usb_get_intfdata(pusb_intf); > + newpnetdev = usb_get_intfdata(pusb_intf); > usb_set_intfdata(pusb_intf, NULL); > - if (!pnetdev) > + if (!newpnetdev) > goto firmware_load_fail; > release_firmware(padapter->fw); > if (drvpriv.drv_registered) > @@ -625,6 +625,13 @@ static void r871xu_dev_remove(struct usb_interface *pusb_intf) > */ > if (udev->state != USB_STATE_NOTATTACHED) > usb_reset_device(udev); > + if (pnetdev) { > + struct _adapter *padapter = netdev_priv(pnetdev); > + /* Stop driver mlme relation timer */ > + //r8712_stop_drv_timers(padapter); > + //r871x_dev_unload(padapter); I am not sure if I should add those deallocation functions in this branch. I will appreciate it if anyone can give some advice here. > + r8712_free_drv_sw(padapter); > + } > } > > static int __init r8712u_drv_entry(void) > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv 2021-05-21 12:08 [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv Dongliang Mu 2021-05-21 12:09 ` 慕冬亮 @ 2021-05-21 12:18 ` Greg KH 2021-05-21 12:24 ` 慕冬亮 1 sibling, 1 reply; 13+ messages in thread From: Greg KH @ 2021-05-21 12:18 UTC (permalink / raw) To: Dongliang Mu Cc: Larry.Finger, florian.c.schilhabel, rkovhaev, straube.linux, linux-staging, linux-kernel, syzbot+1c46f3771695bccbdb3a On Fri, May 21, 2021 at 08:08:11PM +0800, Dongliang Mu wrote: > r871xu_dev_remove failed to call r8712_free_drv_sw() and free the > resource (e.g., struct urb) due to the failure of firmware loading. > > Fix this by invoking r8712_free_drv_sw at the failure site. > > Reported-by: syzbot+1c46f3771695bccbdb3a@syzkaller.appspotmail.com > Fixes: b4383c971bc5 ("staging: rtl8712: handle firmware load failure") > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > --- > drivers/staging/rtl8712/usb_intf.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/rtl8712/usb_intf.c b/drivers/staging/rtl8712/usb_intf.c > index dc21e7743349..a5190b4250ce 100644 > --- a/drivers/staging/rtl8712/usb_intf.c > +++ b/drivers/staging/rtl8712/usb_intf.c > @@ -589,7 +589,7 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf, > */ > static void r871xu_dev_remove(struct usb_interface *pusb_intf) > { > - struct net_device *pnetdev = usb_get_intfdata(pusb_intf); > + struct net_device *pnetdev, *newpnetdev = usb_get_intfdata(pusb_intf); > struct usb_device *udev = interface_to_usbdev(pusb_intf); > > if (pnetdev) { Did you test this? I think you just broke the code right here :( thanks, greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv 2021-05-21 12:18 ` Greg KH @ 2021-05-21 12:24 ` 慕冬亮 0 siblings, 0 replies; 13+ messages in thread From: 慕冬亮 @ 2021-05-21 12:24 UTC (permalink / raw) To: Greg KH Cc: Larry.Finger, florian.c.schilhabel, rkovhaev, straube.linux, linux-staging, linux-kernel, syzbot+1c46f3771695bccbdb3a On Fri, May 21, 2021 at 8:18 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Fri, May 21, 2021 at 08:08:11PM +0800, Dongliang Mu wrote: > > r871xu_dev_remove failed to call r8712_free_drv_sw() and free the > > resource (e.g., struct urb) due to the failure of firmware loading. > > > > Fix this by invoking r8712_free_drv_sw at the failure site. > > > > Reported-by: syzbot+1c46f3771695bccbdb3a@syzkaller.appspotmail.com > > Fixes: b4383c971bc5 ("staging: rtl8712: handle firmware load failure") > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > --- > > drivers/staging/rtl8712/usb_intf.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/staging/rtl8712/usb_intf.c b/drivers/staging/rtl8712/usb_intf.c > > index dc21e7743349..a5190b4250ce 100644 > > --- a/drivers/staging/rtl8712/usb_intf.c > > +++ b/drivers/staging/rtl8712/usb_intf.c > > @@ -589,7 +589,7 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf, > > */ > > static void r871xu_dev_remove(struct usb_interface *pusb_intf) > > { > > - struct net_device *pnetdev = usb_get_intfdata(pusb_intf); > > + struct net_device *pnetdev, *newpnetdev = usb_get_intfdata(pusb_intf); > > struct usb_device *udev = interface_to_usbdev(pusb_intf); > > > > if (pnetdev) { > > Did you test this? For now, I only tested this patch in my local workspace. The memory leak does not occur any more. I have pushed a patch testing onto the syzbot dashboard [1]. Now it is in the pending state. [1] https://syzkaller.appspot.com/bug?id=3a325b8389fc41c1bc94de0f4ac437ed13cce584 > > I think you just broke the code right here :( If I broke any code logic, I am sorry. However, this patch only adds some code to deallocate some resources when failing to load firmware. Do you mean that I replace pnetdev with the variable - newpnetdev? > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv @ 2021-05-21 12:24 ` 慕冬亮 0 siblings, 0 replies; 13+ messages in thread From: 慕冬亮 @ 2021-05-21 12:24 UTC (permalink / raw) To: Greg KH Cc: Larry.Finger, florian.c.schilhabel, rkovhaev, straube.linux, linux-staging, linux-kernel, syzbot+1c46f3771695bccbdb3a On Fri, May 21, 2021 at 8:18 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Fri, May 21, 2021 at 08:08:11PM +0800, Dongliang Mu wrote: > > r871xu_dev_remove failed to call r8712_free_drv_sw() and free the > > resource (e.g., struct urb) due to the failure of firmware loading. > > > > Fix this by invoking r8712_free_drv_sw at the failure site. > > > > Reported-by: syzbot+1c46f3771695bccbdb3a@syzkaller.appspotmail.com > > Fixes: b4383c971bc5 ("staging: rtl8712: handle firmware load failure") > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > --- > > drivers/staging/rtl8712/usb_intf.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/staging/rtl8712/usb_intf.c b/drivers/staging/rtl8712/usb_intf.c > > index dc21e7743349..a5190b4250ce 100644 > > --- a/drivers/staging/rtl8712/usb_intf.c > > +++ b/drivers/staging/rtl8712/usb_intf.c > > @@ -589,7 +589,7 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf, > > */ > > static void r871xu_dev_remove(struct usb_interface *pusb_intf) > > { > > - struct net_device *pnetdev = usb_get_intfdata(pusb_intf); > > + struct net_device *pnetdev, *newpnetdev = usb_get_intfdata(pusb_intf); > > struct usb_device *udev = interface_to_usbdev(pusb_intf); > > > > if (pnetdev) { > > Did you test this? For now, I only tested this patch in my local workspace. The memory leak does not occur any more. I have pushed a patch testing onto the syzbot dashboard [1]. Now it is in the pending state. [1] https://syzkaller.appspot.com/bug?id=3a325b8389fc41c1bc94de0f4ac437ed13cce584 > > I think you just broke the code right here :( If I broke any code logic, I am sorry. However, this patch only adds some code to deallocate some resources when failing to load firmware. Do you mean that I replace pnetdev with the variable - newpnetdev? > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv 2021-05-21 12:24 ` 慕冬亮 (?) @ 2021-05-21 13:16 ` Greg KH 2021-05-21 13:42 ` 慕冬亮 -1 siblings, 1 reply; 13+ messages in thread From: Greg KH @ 2021-05-21 13:16 UTC (permalink / raw) To: 慕冬亮 Cc: Larry.Finger, florian.c.schilhabel, rkovhaev, straube.linux, linux-staging, linux-kernel, syzbot+1c46f3771695bccbdb3a On Fri, May 21, 2021 at 08:24:58PM +0800, 慕冬亮 wrote: > On Fri, May 21, 2021 at 8:18 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Fri, May 21, 2021 at 08:08:11PM +0800, Dongliang Mu wrote: > > > r871xu_dev_remove failed to call r8712_free_drv_sw() and free the > > > resource (e.g., struct urb) due to the failure of firmware loading. > > > > > > Fix this by invoking r8712_free_drv_sw at the failure site. > > > > > > Reported-by: syzbot+1c46f3771695bccbdb3a@syzkaller.appspotmail.com > > > Fixes: b4383c971bc5 ("staging: rtl8712: handle firmware load failure") > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > > --- > > > drivers/staging/rtl8712/usb_intf.c | 13 ++++++++++--- > > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/staging/rtl8712/usb_intf.c b/drivers/staging/rtl8712/usb_intf.c > > > index dc21e7743349..a5190b4250ce 100644 > > > --- a/drivers/staging/rtl8712/usb_intf.c > > > +++ b/drivers/staging/rtl8712/usb_intf.c > > > @@ -589,7 +589,7 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf, > > > */ > > > static void r871xu_dev_remove(struct usb_interface *pusb_intf) > > > { > > > - struct net_device *pnetdev = usb_get_intfdata(pusb_intf); > > > + struct net_device *pnetdev, *newpnetdev = usb_get_intfdata(pusb_intf); > > > struct usb_device *udev = interface_to_usbdev(pusb_intf); > > > > > > if (pnetdev) { > > > > Did you test this? > > For now, I only tested this patch in my local workspace. The memory > leak does not occur any more. > > I have pushed a patch testing onto the syzbot dashboard [1]. Now it is > in the pending state. > > [1] https://syzkaller.appspot.com/bug?id=3a325b8389fc41c1bc94de0f4ac437ed13cce584 > > > > > I think you just broke the code right here :( > > If I broke any code logic, I am sorry. However, this patch only adds > some code to deallocate some resources when failing to load firmware. > > Do you mean that I replace pnetdev with the variable - newpnetdev? Yes, and then the first thing the code does is check the value of pnetdev which is totally undefined :( ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv 2021-05-21 13:16 ` Greg KH @ 2021-05-21 13:42 ` 慕冬亮 0 siblings, 0 replies; 13+ messages in thread From: 慕冬亮 @ 2021-05-21 13:42 UTC (permalink / raw) To: Greg KH Cc: Larry.Finger, florian.c.schilhabel, rkovhaev, straube.linux, linux-staging, linux-kernel, syzbot+1c46f3771695bccbdb3a On Fri, May 21, 2021 at 9:16 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Fri, May 21, 2021 at 08:24:58PM +0800, 慕冬亮 wrote: > > On Fri, May 21, 2021 at 8:18 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > On Fri, May 21, 2021 at 08:08:11PM +0800, Dongliang Mu wrote: > > > > r871xu_dev_remove failed to call r8712_free_drv_sw() and free the > > > > resource (e.g., struct urb) due to the failure of firmware loading. > > > > > > > > Fix this by invoking r8712_free_drv_sw at the failure site. > > > > > > > > Reported-by: syzbot+1c46f3771695bccbdb3a@syzkaller.appspotmail.com > > > > Fixes: b4383c971bc5 ("staging: rtl8712: handle firmware load failure") > > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > > > --- > > > > drivers/staging/rtl8712/usb_intf.c | 13 ++++++++++--- > > > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/staging/rtl8712/usb_intf.c b/drivers/staging/rtl8712/usb_intf.c > > > > index dc21e7743349..a5190b4250ce 100644 > > > > --- a/drivers/staging/rtl8712/usb_intf.c > > > > +++ b/drivers/staging/rtl8712/usb_intf.c > > > > @@ -589,7 +589,7 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf, > > > > */ > > > > static void r871xu_dev_remove(struct usb_interface *pusb_intf) > > > > { > > > > - struct net_device *pnetdev = usb_get_intfdata(pusb_intf); > > > > + struct net_device *pnetdev, *newpnetdev = usb_get_intfdata(pusb_intf); > > > > struct usb_device *udev = interface_to_usbdev(pusb_intf); > > > > > > > > if (pnetdev) { > > > > > > Did you test this? > > > > For now, I only tested this patch in my local workspace. The memory > > leak does not occur any more. > > > > I have pushed a patch testing onto the syzbot dashboard [1]. Now it is > > in the pending state. > > > > [1] https://syzkaller.appspot.com/bug?id=3a325b8389fc41c1bc94de0f4ac437ed13cce584 > > > > > > > > I think you just broke the code right here :( > > > > If I broke any code logic, I am sorry. However, this patch only adds > > some code to deallocate some resources when failing to load firmware. > > > > Do you mean that I replace pnetdev with the variable - newpnetdev? > > Yes, and then the first thing the code does is check the value of > pnetdev which is totally undefined :( You are right. Apology for the previous patch. I test my old patch below in the local workspace, it works. ------------------------------------------------------------------------------------------------------------------------ --- a/drivers/staging/rtl8712/usb_intf.c +++ b/drivers/staging/rtl8712/usb_intf.c @@ -597,9 +597,9 @@ static void r871xu_dev_remove(struct usb_interface *pusb_intf) /* never exit with a firmware callback pending */ wait_for_completion(&padapter->rtl8712_fw_ready); - pnetdev = usb_get_intfdata(pusb_intf); + struct net_device *newpnetdev = usb_get_intfdata(pusb_intf); usb_set_intfdata(pusb_intf, NULL); - if (!pnetdev) + if (!newpnetdev) goto firmware_load_fail; release_firmware(padapter->fw); if (drvpriv.drv_registered) @@ -625,6 +625,14 @@ static void r871xu_dev_remove(struct usb_interface *pusb_intf) */ if (udev->state != USB_STATE_NOTATTACHED) usb_reset_device(udev); + if (pnetdev) { + struct _adapter *padapter = netdev_priv(pnetdev); + /* Stop driver mlme relation timer */ + //r8712_stop_drv_timers(padapter); + //r871x_dev_unload(padapter); + r8712_free_drv_sw(padapter); + /* udev is already freed in failed fireware loading */ + } } ------------------------------------------------------------------------------------------------------------------------ However, the compiler complains the declaration of newpnetdev. So I moved the declaration to the beginning, but I forget to initialize both two variables. :( I will revise this problem and test it in my local workspace. If it works, I will resend a v2 patch. BTW, should I uncomment "r8712_stop_drv_timers" and "r871x_dev_unload"? I am not very sure about its functionability. > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv @ 2021-05-21 13:42 ` 慕冬亮 0 siblings, 0 replies; 13+ messages in thread From: 慕冬亮 @ 2021-05-21 13:42 UTC (permalink / raw) To: Greg KH Cc: Larry.Finger, florian.c.schilhabel, rkovhaev, straube.linux, linux-staging, linux-kernel, syzbot+1c46f3771695bccbdb3a On Fri, May 21, 2021 at 9:16 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Fri, May 21, 2021 at 08:24:58PM +0800, 慕冬亮 wrote: > > On Fri, May 21, 2021 at 8:18 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > On Fri, May 21, 2021 at 08:08:11PM +0800, Dongliang Mu wrote: > > > > r871xu_dev_remove failed to call r8712_free_drv_sw() and free the > > > > resource (e.g., struct urb) due to the failure of firmware loading. > > > > > > > > Fix this by invoking r8712_free_drv_sw at the failure site. > > > > > > > > Reported-by: syzbot+1c46f3771695bccbdb3a@syzkaller.appspotmail.com > > > > Fixes: b4383c971bc5 ("staging: rtl8712: handle firmware load failure") > > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > > > --- > > > > drivers/staging/rtl8712/usb_intf.c | 13 ++++++++++--- > > > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/staging/rtl8712/usb_intf.c b/drivers/staging/rtl8712/usb_intf.c > > > > index dc21e7743349..a5190b4250ce 100644 > > > > --- a/drivers/staging/rtl8712/usb_intf.c > > > > +++ b/drivers/staging/rtl8712/usb_intf.c > > > > @@ -589,7 +589,7 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf, > > > > */ > > > > static void r871xu_dev_remove(struct usb_interface *pusb_intf) > > > > { > > > > - struct net_device *pnetdev = usb_get_intfdata(pusb_intf); > > > > + struct net_device *pnetdev, *newpnetdev = usb_get_intfdata(pusb_intf); > > > > struct usb_device *udev = interface_to_usbdev(pusb_intf); > > > > > > > > if (pnetdev) { > > > > > > Did you test this? > > > > For now, I only tested this patch in my local workspace. The memory > > leak does not occur any more. > > > > I have pushed a patch testing onto the syzbot dashboard [1]. Now it is > > in the pending state. > > > > [1] https://syzkaller.appspot.com/bug?id=3a325b8389fc41c1bc94de0f4ac437ed13cce584 > > > > > > > > I think you just broke the code right here :( > > > > If I broke any code logic, I am sorry. However, this patch only adds > > some code to deallocate some resources when failing to load firmware. > > > > Do you mean that I replace pnetdev with the variable - newpnetdev? > > Yes, and then the first thing the code does is check the value of > pnetdev which is totally undefined :( You are right. Apology for the previous patch. I test my old patch below in the local workspace, it works. ------------------------------------------------------------------------------------------------------------------------ --- a/drivers/staging/rtl8712/usb_intf.c +++ b/drivers/staging/rtl8712/usb_intf.c @@ -597,9 +597,9 @@ static void r871xu_dev_remove(struct usb_interface *pusb_intf) /* never exit with a firmware callback pending */ wait_for_completion(&padapter->rtl8712_fw_ready); - pnetdev = usb_get_intfdata(pusb_intf); + struct net_device *newpnetdev = usb_get_intfdata(pusb_intf); usb_set_intfdata(pusb_intf, NULL); - if (!pnetdev) + if (!newpnetdev) goto firmware_load_fail; release_firmware(padapter->fw); if (drvpriv.drv_registered) @@ -625,6 +625,14 @@ static void r871xu_dev_remove(struct usb_interface *pusb_intf) */ if (udev->state != USB_STATE_NOTATTACHED) usb_reset_device(udev); + if (pnetdev) { + struct _adapter *padapter = netdev_priv(pnetdev); + /* Stop driver mlme relation timer */ + //r8712_stop_drv_timers(padapter); + //r871x_dev_unload(padapter); + r8712_free_drv_sw(padapter); + /* udev is already freed in failed fireware loading */ + } } ------------------------------------------------------------------------------------------------------------------------ However, the compiler complains the declaration of newpnetdev. So I moved the declaration to the beginning, but I forget to initialize both two variables. :( I will revise this problem and test it in my local workspace. If it works, I will resend a v2 patch. BTW, should I uncomment "r8712_stop_drv_timers" and "r871x_dev_unload"? I am not very sure about its functionability. > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv @ 2021-05-24 11:49 Dongliang Mu 2021-05-25 11:03 ` Dan Carpenter 0 siblings, 1 reply; 13+ messages in thread From: Dongliang Mu @ 2021-05-24 11:49 UTC (permalink / raw) To: Larry.Finger, florian.c.schilhabel, gregkh, rkovhaev, straube.linux, linux-staging, linux-kernel Cc: Dongliang Mu The commit b4383c971bc5 ("staging: rtl8712: handle firmware load failure") adds a goto statement when failing to load the firmware, however, it fails to deallocate the resources (e.g., struct urb) allocated in the function r8712_init_drv_sw. Fix this by invoking r8712_free_drv_sw() before the goto statement. backtrace: [<00000000e0748eb7>] kmalloc include/linux/slab.h:557 [inline] [<00000000e0748eb7>] usb_alloc_urb+0x66/0xe0 drivers/usb/core/urb.c:74 [<00000000fe5a9432>] r8712_os_recvbuf_resource_alloc+0x1b/0x80 drivers/staging/rtl8712/recv_linux.c:46 [<00000000923fed72>] r8712_init_recv_priv+0x96/0x210 drivers/staging/rtl8712/rtl8712_recv.c:54 [<000000000038512f>] _r8712_init_recv_priv+0x134/0x150 drivers/staging/rtl8712/rtl871x_recv.c:78 [<0000000066e70a4e>] r8712_init_drv_sw+0xa0/0x1d0 drivers/staging/rtl8712/os_intfs.c:312 [<000000001d2974c0>] r871xu_drv_init.cold+0x104/0x7d1 drivers/staging/rtl8712/usb_intf.c:391 [<000000001d449ce2>] usb_probe_interface+0x177/0x370 drivers/usb/core/driver.c:396 [<00000000cd123d34>] really_probe+0x159/0x4a0 drivers/base/dd.c:554 [<00000000364585cc>] driver_probe_device+0x84/0x100 drivers/base/dd.c:740 [<0000000048b74bde>] __device_attach_driver+0xee/0x110 drivers/base/dd.c:846 [<00000000c358ab15>] bus_for_each_drv+0xb7/0x100 drivers/base/bus.c:431 [<00000000bfa9b076>] __device_attach+0x122/0x250 drivers/base/dd.c:914 [<0000000048fe302a>] bus_probe_device+0xc6/0xe0 drivers/base/bus.c:491 [<000000002ceae175>] device_add+0x5be/0xc30 drivers/base/core.c:3109 [<00000000e4813a0d>] usb_set_configuration+0x9d9/0xb90 drivers/usb/core/message.c:2164 [<00000000cbb8c98f>] usb_generic_driver_probe+0x8c/0xc0 drivers/usb/core/generic.c:238 Fixes: b4383c971bc5 ("staging: rtl8712: handle firmware load failure") Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> --- drivers/staging/rtl8712/usb_intf.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/staging/rtl8712/usb_intf.c b/drivers/staging/rtl8712/usb_intf.c index dc21e7743349..fd5da3a04b4e 100644 --- a/drivers/staging/rtl8712/usb_intf.c +++ b/drivers/staging/rtl8712/usb_intf.c @@ -599,8 +599,10 @@ static void r871xu_dev_remove(struct usb_interface *pusb_intf) wait_for_completion(&padapter->rtl8712_fw_ready); pnetdev = usb_get_intfdata(pusb_intf); usb_set_intfdata(pusb_intf, NULL); - if (!pnetdev) + if (!pnetdev) { + r8712_free_drv_sw(padapter); goto firmware_load_fail; + } release_firmware(padapter->fw); if (drvpriv.drv_registered) padapter->surprise_removed = true; -- 2.25.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv 2021-05-24 11:49 Dongliang Mu @ 2021-05-25 11:03 ` Dan Carpenter 2021-05-25 14:32 ` 慕冬亮 0 siblings, 1 reply; 13+ messages in thread From: Dan Carpenter @ 2021-05-25 11:03 UTC (permalink / raw) To: Dongliang Mu Cc: Larry.Finger, florian.c.schilhabel, gregkh, rkovhaev, straube.linux, linux-staging, linux-kernel This is a syzbot warning, right? If so, it needs a Reported-by tag. I don't really understand why rtl871x_load_fw_cb() calls: usb_put_dev(udev); usb_set_intfdata(usb_intf, NULL); That just seems like a layering violation and it keeps causing bugs and I think everything would be simpler if we deleted that. That way we could remove both checks for if pnetdev is NULL. [PATCH 10/x] staging: rtl8712: delete bogus clean up code in firmware callback This clean up is supposed to be done in r871xu_dev_remove(). Setting the usb USB interface leads to leaks which syzbot detects. <stack trace> Reported-by: Sysbot blah blah Patch 10 because their are some other easier patches which could be done first. Always do the easiest patch first in case a patch set needs to be changed, it's easier to change the latter patches. Your patch fixes one sysbot warning but then syzbot will complain about something else because there are a bunch of other things which need to be freed as well. Of course, it's complicated to know which things need to be freed and which not because the code is really bad. It's better to just re-write the cleanup code and fix everything at once. I always encourage everyone to follow the "free the most recently allocated resource" system of error handling. This is the only *systematic* way I know to do error handling. Everything else is ad-hoc, impossible to review and has proven bug prone in real life. The rules are: 1) Every function cleans up it's own allocations. Never partially allocate things. Never leave the caller to clean up your resources. 2) Keep track in your head of the most recently allocated resource. Keeping track of just one thing is a lot easier than keeping track of everything. 3) Use good label names which say what the labels free. err_free_netdev: free_netdev(netdev); 4) Every allocator function has a mirror free function. How it works in real life is like this: int my_probe(...) { a = alloc(); if (!a) return -ENOMEM; b = alloc(); if (!b) { ret = -ENOMEM; goto free_a; // <-- a is most recent allocation } c = alloc(); if (!c) { ret = -ENOMEM; goto free_b; } return 0; free_b: free(b); free_a: free(a); return ret; } Then the mirror function basically writes its self. You just have to cut and paste the clean up code and add a kfree(c) to the start. void my_release(...) { free(c); free(b); free(a); } Once we move all the frees to the correct place and in the right order then it becomes simpler. drivers/staging/rtl8712/usb_intf.c 345 static int r871xu_drv_init(struct usb_interface *pusb_intf, 346 const struct usb_device_id *pdid) 347 { 348 uint status; Keep status for status = padapter->dvobj_init() but everything else should be int ret. Eventually "status" will be deleted. 349 struct _adapter *padapter = NULL; 350 struct dvobj_priv *pdvobjpriv; 351 struct net_device *pnetdev; 352 struct usb_device *udev; 353 354 /* In this probe function, O.S. will provide the usb interface pointer 355 * to driver. We have to increase the reference count of the usb device 356 * structure by using the usb_get_dev function. 357 */ 358 udev = interface_to_usbdev(pusb_intf); 359 usb_get_dev(udev); ^^^^^^^^^^^^^^^^^ First resource allocation/thing to be freed. Is this really required? I'm not sure. Anyway, it's harmless. 360 pintf = pusb_intf; 361 /* step 1. */ Delete all these step 1,2,3... comments. The authors were trying to keep track of what they had allocated but unfortunately writing it down did not help them. 362 pnetdev = r8712_init_netdev(); 363 if (!pnetdev) 364 goto error; if (!pnetdev) { ret = -ENOMEM; goto put_dev; } r8712_init_netdev() needs a free function. Right now the free_netdev() is hidden in r8712_free_drv_sw() which is the wrong place. void r8712_free_netdev(struct net_device *dev) { free_netdev(dev); } "pnetdev" is now our current resource. [PATCH 6/x] staging: rtl8712: create r8712_free_netdev() function This is a release function for r8712_init_netdev(). I changed the free_netdev() in r871xu_drv_init() to use this and I moved the free_netdev() out of r8712_free_drv_sw() and into the r871xu_dev_remove() function where it belongs. 365 padapter = netdev_priv(pnetdev); 366 disable_ht_for_spec_devid(pdid, padapter); 367 pdvobjpriv = &padapter->dvobjpriv; 368 pdvobjpriv->padapter = padapter; 369 padapter->dvobjpriv.pusbdev = udev; 370 padapter->pusb_intf = pusb_intf; 371 usb_set_intfdata(pusb_intf, pnetdev); 372 SET_NETDEV_DEV(pnetdev, &pusb_intf->dev); 373 pnetdev->dev.type = &wlan_type; 374 /* step 2. */ 375 padapter->dvobj_init = r8712_usb_dvobj_init; 376 padapter->dvobj_deinit = r8712_usb_dvobj_deinit; 377 padapter->halpriv.hal_bus_init = r8712_usb_hal_bus_init; 378 padapter->dvobjpriv.inirp_init = r8712_usb_inirp_init; 379 padapter->dvobjpriv.inirp_deinit = r8712_usb_inirp_deinit; These function pointers are garbage. We should try to move this code to calling the functions directly and delete the pointers from the struct. 380 /* step 3. 381 * initialize the dvobj_priv 382 */ 383 if (!padapter->dvobj_init) { 384 goto error; We set ->dvobj_init on line 375 so it can't be NULL. Delete this. [PATCH 1/x] staging: rtl8712: delete impossible NULL check 385 } else { 386 status = padapter->dvobj_init(padapter); 387 if (status != _SUCCESS) 388 goto error; Since we know that ->dvobj_init is r8712_usb_dvobj_init() then lets call that directly. status = r8712_usb_dvobj_init(padapter); if (status != _SUCCESS) { ret = -ENOMEM; goto free_netdev; } [PATCH 2/x] staging: rtl8712: Get rid of ->dvobj_init/deinit function pointers The "padapter->dvobj_init/->dvobj_deinit" pointers are not required. We can call the functions directly. This usb_dvobj (dumb name) is now our current resource. Unfortunately the r8712_usb_dvobj_deinit() function is an empty stub function. It should be: static void r8712_usb_dvobj_deinit(struct _adapter *padapter) { r8712_free_io_queue(padapter); } Currently the call to r8712_free_io_queue() is hidden inside the r8712_free_drv_sw() function but that's the wrong place for it. [PATCH 8/x] staging: rtl8712: implement r8712_usb_dvobj_deinit() The r8712_usb_dvobj_deinit() function is supposed to clean up from r8712_usb_dvobj_deinit(). Currently that is open coded in r8712_free_drv_sw(), but it should be implemented as a separate function and called from r871xu_dev_remove(). 389 } 390 /* step 4. */ 391 status = r8712_init_drv_sw(padapter); 392 if (status) 393 goto error; ret = r8712_init_drv_sw(padapter); if (ret) goto free_usb_dvobj; The r8712_init_drv_sw() function does not clean up after itself on error. [PATCH 3/x] staging: rtl8712: fix leaks in r8712_init_drv_sw(). The r8712_free_drv_sw() exists but it is not a mirror of the r8712_init_drv_sw() function. As we've noticed, it frees some things which it should not but it also leaves timers running so presumably that leads to a use after free. [PATCH 9/x] staging: rtl8712: re-write r8712_free_drv_sw() The r8712_free_drv_sw() should clean up everything allocated in r8712_init_drv_sw() but it does not. Some of it was done in r8712_stop_drv_timers() and so I have moved it here and deleted that code. PATCH 9 is slightly risky. Be careful not to introduce any double frees! 394 /* step 5. read efuse/eeprom data and get mac_addr */ 395 { 396 int i, offset; 397 u8 mac[6]; 398 u8 tmpU1b, AutoloadFail, eeprom_CustomerID; 399 u8 *pdata = padapter->eeprompriv.efuse_eeprom_data; Declare this at the top. Pull the code in one tab. [PATCH 4/x] staging: rtl8712: pull code in one tab 400 401 tmpU1b = r8712_read8(padapter, EE_9346CR);/*CR9346*/ 402 403 /* To check system boot selection.*/ 404 dev_info(&udev->dev, "r8712u: Boot from %s: Autoload %s\n", 405 (tmpU1b & _9356SEL) ? "EEPROM" : "EFUSE", 406 (tmpU1b & _EEPROM_EN) ? "OK" : "Failed"); 407 408 /* To check autoload success or not.*/ 409 if (tmpU1b & _EEPROM_EN) { 410 AutoloadFail = true; Put the rest of this if statement after the "AutoloadFail = true;" into a separate function. Set AutoloadFail = false at the top of the function: bool AutoloadFail = false; [PATCH 5/x] staging: rtl8712: move code to a separate function 411 /* The following operations prevent Efuse leakage by 412 * turning on 2.5V. 413 */ 414 tmpU1b = r8712_read8(padapter, EFUSE_TEST + 3); 415 r8712_write8(padapter, EFUSE_TEST + 3, tmpU1b | 0x80); 416 msleep(20); 417 r8712_write8(padapter, EFUSE_TEST + 3, 418 (tmpU1b & (~BIT(7)))); 419 420 /* Retrieve Chip version. 421 * Recognize IC version by Reg0x4 BIT15. 422 */ 423 tmpU1b = (u8)((r8712_read32(padapter, PMC_FSM) >> 15) & 424 0x1F); 425 if (tmpU1b == 0x3) 426 padapter->registrypriv.chip_version = 427 RTL8712_3rdCUT; 428 else 429 padapter->registrypriv.chip_version = 429 padapter->registrypriv.chip_version = 430 (tmpU1b >> 1) + 1; 431 switch (padapter->registrypriv.chip_version) { 432 case RTL8712_1stCUT: 433 case RTL8712_2ndCUT: 434 case RTL8712_3rdCUT: 435 break; 436 default: 437 padapter->registrypriv.chip_version = 438 RTL8712_2ndCUT; 439 break; 440 } 441 442 for (i = 0, offset = 0; i < 128; i += 8, offset++) 443 r8712_efuse_pg_packet_read(padapter, offset, 444 &pdata[i]); 445 446 if (!r8712_initmac || !mac_pton(r8712_initmac, mac)) { 447 /* Use the mac address stored in the Efuse 448 * offset = 0x12 for usb in efuse 449 */ 450 ether_addr_copy(mac, &pdata[0x12]); 451 } 452 eeprom_CustomerID = pdata[0x52]; 453 switch (eeprom_CustomerID) { 454 case EEPROM_CID_ALPHA: 455 padapter->eeprompriv.CustomerID = 456 RT_CID_819x_ALPHA; 457 break; 458 case EEPROM_CID_CAMEO: 459 padapter->eeprompriv.CustomerID = 460 RT_CID_819x_CAMEO; 461 break; 462 case EEPROM_CID_SITECOM: 463 padapter->eeprompriv.CustomerID = 464 RT_CID_819x_Sitecom; 465 break; 466 case EEPROM_CID_COREGA: 467 padapter->eeprompriv.CustomerID = 468 RT_CID_COREGA; 469 break; 470 case EEPROM_CID_Senao: 471 padapter->eeprompriv.CustomerID = 472 RT_CID_819x_Senao; 473 break; 474 case EEPROM_CID_EDIMAX_BELKIN: 475 padapter->eeprompriv.CustomerID = 476 RT_CID_819x_Edimax_Belkin; 477 break; 478 case EEPROM_CID_SERCOMM_BELKIN: 479 padapter->eeprompriv.CustomerID = 480 RT_CID_819x_Sercomm_Belkin; 481 break; 482 case EEPROM_CID_WNC_COREGA: 483 padapter->eeprompriv.CustomerID = 484 RT_CID_819x_WNC_COREGA; 485 break; 486 case EEPROM_CID_WHQL: 487 break; 488 case EEPROM_CID_NetCore: 489 padapter->eeprompriv.CustomerID = 490 RT_CID_819x_Netcore; 491 break; 492 case EEPROM_CID_CAMEO1: 493 padapter->eeprompriv.CustomerID = 494 RT_CID_819x_CAMEO1; 495 break; 496 case EEPROM_CID_CLEVO: 497 padapter->eeprompriv.CustomerID = 498 RT_CID_819x_CLEVO; 499 break; 500 default: 501 padapter->eeprompriv.CustomerID = 502 RT_CID_DEFAULT; 503 break; 504 } 505 dev_info(&udev->dev, "r8712u: CustomerID = 0x%.4x\n", 506 padapter->eeprompriv.CustomerID); 507 /* Led mode */ 508 switch (padapter->eeprompriv.CustomerID) { 509 case RT_CID_DEFAULT: 510 case RT_CID_819x_ALPHA: 510 case RT_CID_819x_ALPHA: 511 case RT_CID_819x_CAMEO: 512 padapter->ledpriv.LedStrategy = SW_LED_MODE1; 513 padapter->ledpriv.bRegUseLed = true; 514 break; 515 case RT_CID_819x_Sitecom: 516 padapter->ledpriv.LedStrategy = SW_LED_MODE2; 517 padapter->ledpriv.bRegUseLed = true; 518 break; 519 case RT_CID_COREGA: 520 case RT_CID_819x_Senao: 521 padapter->ledpriv.LedStrategy = SW_LED_MODE3; 522 padapter->ledpriv.bRegUseLed = true; 523 break; 524 case RT_CID_819x_Edimax_Belkin: 525 padapter->ledpriv.LedStrategy = SW_LED_MODE4; 526 padapter->ledpriv.bRegUseLed = true; 527 break; 528 case RT_CID_819x_Sercomm_Belkin: 529 padapter->ledpriv.LedStrategy = SW_LED_MODE5; 530 padapter->ledpriv.bRegUseLed = true; 531 break; 532 case RT_CID_819x_WNC_COREGA: 533 padapter->ledpriv.LedStrategy = SW_LED_MODE6; 534 padapter->ledpriv.bRegUseLed = true; 535 break; 536 default: 537 padapter->ledpriv.LedStrategy = SW_LED_MODE0; 538 padapter->ledpriv.bRegUseLed = false; 539 break; 540 } 541 } else { 542 AutoloadFail = false; 543 } 544 if (((mac[0] == 0xff) && (mac[1] == 0xff) && 545 (mac[2] == 0xff) && (mac[3] == 0xff) && 546 (mac[4] == 0xff) && (mac[5] == 0xff)) || 547 ((mac[0] == 0x00) && (mac[1] == 0x00) && 548 (mac[2] == 0x00) && (mac[3] == 0x00) && 549 (mac[4] == 0x00) && (mac[5] == 0x00)) || 550 (!AutoloadFail)) { 551 mac[0] = 0x00; 552 mac[1] = 0xe0; 553 mac[2] = 0x4c; 554 mac[3] = 0x87; 555 mac[4] = 0x00; 556 mac[5] = 0x00; 557 } 558 if (r8712_initmac) { 559 /* Make sure the user did not select a multicast 560 * address by setting bit 1 of first octet. 561 */ 562 mac[0] &= 0xFE; 563 dev_info(&udev->dev, 564 "r8712u: MAC Address from user = %pM\n", mac); 565 } else { 566 dev_info(&udev->dev, 567 "r8712u: MAC Address from efuse = %pM\n", mac); 568 } 569 ether_addr_copy(pnetdev->dev_addr, mac); 570 } 571 /* step 6. Load the firmware asynchronously */ 572 if (rtl871x_load_fw(padapter)) 573 goto error; The big indent block didn't allocate anything so our most recent allocation is still drv_sw. ret = rtl871x_load_fw(padapter); if (ret) goto free_drv_sw; 574 spin_lock_init(&padapter->lock_rx_ff0_filter); 575 mutex_init(&padapter->mutex_start); 576 return 0; 577 error: 578 usb_put_dev(udev); 579 usb_set_intfdata(pusb_intf, NULL); 580 if (padapter && padapter->dvobj_deinit) 581 padapter->dvobj_deinit(padapter); 582 if (pnetdev) 583 free_netdev(pnetdev); 584 return -ENODEV; 585 } dvobj_deinit() is a no-op as discussed. This also doesn't release the drv_sw. So there are some leaks. When we fix and implement the mirrored clean up functions it becomes: return 0; free_drv_sw: r8712_free_drv_sw(padapter); free_usb_dvobj: r8712_usb_dvobj_deinit(padapter); free_netdev: r8712_free_netdev(pnetdev); put_dev: usb_put_dev(udev); usb_set_intfdata(pusb_intf, NULL); return ret; } Now we can implement a remove function. It's complicated because we don't know if the firmware loaded successfully and if the network device is registered. We don't really need to test if (adapter->fw) becaue release_firmware(NULL) is a no-op but I did it anyway. Based on what we know so far, this is what it should look like: static void r871xu_dev_remove(struct usb_interface *pusb_intf) { struct net_device *pnetdev = usb_get_intfdata(pusb_intf); struct usb_device *udev = interface_to_usbdev(pusb_intf); struct _adapter *padapter = netdev_priv(pnetdev); /* never exit with a firmware callback pending */ wait_for_completion(&padapter->rtl8712_fw_ready); if (pnetdev->reg_state != NETREG_UNINITIALIZED) unregister_netdev(pnetdev); /* will call netdev_close() */ if (adapter->fw) release_firmware(padapter->fw); r8712_free_drv_sw(padapter); r8712_usb_dvobj_deinit(padapter); r8712_free_netdev(pnetdev); usb_put_dev(udev); usb_set_intfdata(pusb_intf, NULL); } The kernel's r871xu_dev_remove() look different so there are some remaining questions. 585 static void r871xu_dev_remove(struct usb_interface *pusb_intf) 586 { 587 struct net_device *pnetdev = usb_get_intfdata(pusb_intf); 588 struct usb_device *udev = interface_to_usbdev(pusb_intf); 589 590 if (pnetdev) { These checks are no longer required now that we deleted the two lines from rtl871x_load_fw_cb(). 591 struct _adapter *padapter = netdev_priv(pnetdev); 592 593 /* never exit with a firmware callback pending */ 594 wait_for_completion(&padapter->rtl8712_fw_ready); 595 pnetdev = usb_get_intfdata(pusb_intf); This assignment is not required becuse "pnetdev" remains the same. 596 usb_set_intfdata(pusb_intf, NULL); 597 if (!pnetdev) 598 goto firmware_load_fail; 599 release_firmware(padapter->fw); 600 if (drvpriv.drv_registered) 601 padapter->surprise_removed = true; All the "padapter->surprise_removed" code is bogus, but it needs to be audited and deleted before any other fixes to the error handling can be done. [PATCH 7/x] staging: rtl8712: delete padapter->surprise_removed 602 if (pnetdev->reg_state != NETREG_UNINITIALIZED) 603 unregister_netdev(pnetdev); /* will call netdev_close() */ 604 flush_scheduled_work(); 605 udelay(1); This is a layering violation. If it's really required then it needs to be done in the correct location... 606 /* Stop driver mlme relation timer */ 607 r8712_stop_drv_timers(padapter); Once r8712_free_drv_sw() is fixed I think it will take care of the timers. 608 r871x_dev_unload(padapter); The r871x_dev_unload() stuff is suposed to be move to netdev_close(), I think? 609 r8712_free_drv_sw(padapter); 610 611 /* decrease the reference count of the usb device structure 612 * when disconnect 613 */ Pointless comment. 614 usb_put_dev(udev); 615 } 616 firmware_load_fail: 617 /* If we didn't unplug usb dongle and remove/insert module, driver 618 * fails on sitesurvey for the first time when device is up. 619 * Reset usb port for sitesurvey fail issue. 620 */ 621 if (udev->state != USB_STATE_NOTATTACHED) 622 usb_reset_device(udev); This feels wrong. Also it feels like using "udev" after calling usb_put_dev(udev) would be a use after free, but I think the stuff with usb_get/put_dev() is not really required so it doesn't matter. Anyway, leave this stuff. Even though, we might not like it we can't change it without a way to test it using real hardware. That's the same for the flush_scheduled_work() and the udelay(1). We don't like it but we can't test it. 623 } It would probably take a 15 patch series to fix this code. The ordering is important but slightly tricky so be careful with it. regards, dan carpenter ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv 2021-05-25 11:03 ` Dan Carpenter @ 2021-05-25 14:32 ` 慕冬亮 0 siblings, 0 replies; 13+ messages in thread From: 慕冬亮 @ 2021-05-25 14:32 UTC (permalink / raw) To: Dan Carpenter Cc: Larry.Finger, florian.c.schilhabel, Greg KH, rkovhaev, straube.linux, linux-staging, linux-kernel On Tue, May 25, 2021 at 7:04 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > This is a syzbot warning, right? If so, it needs a Reported-by tag. Yes, https://syzkaller.appspot.com/bug?id=3a325b8389fc41c1bc94de0f4ac437ed13cce584 > > I don't really understand why rtl871x_load_fw_cb() calls: I am also confused about the invocation of rtl871x_load_fw_cb. In my local reproduction, if the firmware (rtlwifi/rtl8712u.bin) is missing, the memory leak occurs, otherwise, no memory leak is reported. However, my testing in the syzbot dashboard shows there is no invocation of rtl871x_load_fw_cb. If that's the fact, then I don't quite understand how this memory leak occurred. I doubt the bug I reproduced in the local environment may be not the bug in the syzbot dashboard. So I did not add Reported-by tag > > usb_put_dev(udev); > usb_set_intfdata(usb_intf, NULL); > > That just seems like a layering violation and it keeps causing bugs and > I think everything would be simpler if we deleted that. That way we > could remove both checks for if pnetdev is NULL. > The following description of patches is **really** friendly for newbies like me, especially for rules for allocation and deallocation. I will keep them in mind when developing patches in the future. Thanks very much. I really appreciate it, > [PATCH 10/x] staging: rtl8712: delete bogus clean up code in firmware callback > > This clean up is supposed to be done in r871xu_dev_remove(). > Setting the usb USB interface leads to leaks which syzbot detects. > <stack trace> > Reported-by: Sysbot blah blah > > Patch 10 because their are some other easier patches which could be > done first. Always do the easiest patch first in case a patch set needs > to be changed, it's easier to change the latter patches. > > Your patch fixes one sysbot warning but then syzbot will complain about > something else because there are a bunch of other things which need to > be freed as well. Of course, it's complicated to know which things need > to be freed and which not because the code is really bad. It's better > to just re-write the cleanup code and fix everything at once. > > I always encourage everyone to follow the "free the most recently > allocated resource" system of error handling. This is the only > *systematic* way I know to do error handling. Everything else is > ad-hoc, impossible to review and has proven bug prone in real life. > > The rules are: > 1) Every function cleans up it's own allocations. Never partially > allocate things. Never leave the caller to clean up your resources. > 2) Keep track in your head of the most recently allocated resource. > Keeping track of just one thing is a lot easier than keeping track of > everything. > 3) Use good label names which say what the labels free. > > err_free_netdev: > free_netdev(netdev); > > 4) Every allocator function has a mirror free function. > > How it works in real life is like this: > > int my_probe(...) > { > a = alloc(); > if (!a) > return -ENOMEM; > > b = alloc(); > if (!b) { > ret = -ENOMEM; > goto free_a; // <-- a is most recent allocation > } > > c = alloc(); > if (!c) { > ret = -ENOMEM; > goto free_b; > } > > return 0; > > free_b: > free(b); > free_a: > free(a); > > return ret; > } > > Then the mirror function basically writes its self. You just have to > cut and paste the clean up code and add a kfree(c) to the start. > > void my_release(...) > { > free(c); > free(b); > free(a); > } > > Once we move all the frees to the correct place and in the right order > then it becomes simpler. > > drivers/staging/rtl8712/usb_intf.c > 345 static int r871xu_drv_init(struct usb_interface *pusb_intf, > 346 const struct usb_device_id *pdid) > 347 { > 348 uint status; > > Keep status for status = padapter->dvobj_init() but everything else > should be int ret. Eventually "status" will be deleted. > > 349 struct _adapter *padapter = NULL; > 350 struct dvobj_priv *pdvobjpriv; > 351 struct net_device *pnetdev; > 352 struct usb_device *udev; > 353 > 354 /* In this probe function, O.S. will provide the usb interface pointer > 355 * to driver. We have to increase the reference count of the usb device > 356 * structure by using the usb_get_dev function. > 357 */ > 358 udev = interface_to_usbdev(pusb_intf); > 359 usb_get_dev(udev); > ^^^^^^^^^^^^^^^^^ > First resource allocation/thing to be freed. Is this really required? > I'm not sure. Anyway, it's harmless. > > 360 pintf = pusb_intf; > 361 /* step 1. */ > > Delete all these step 1,2,3... comments. The authors were trying to > keep track of what they had allocated but unfortunately writing it down > did not help them. > > 362 pnetdev = r8712_init_netdev(); > 363 if (!pnetdev) > 364 goto error; > > if (!pnetdev) { > ret = -ENOMEM; > goto put_dev; > } > > r8712_init_netdev() needs a free function. Right now the free_netdev() > is hidden in r8712_free_drv_sw() which is the wrong place. > > void r8712_free_netdev(struct net_device *dev) > { > free_netdev(dev); > } > > "pnetdev" is now our current resource. > > [PATCH 6/x] staging: rtl8712: create r8712_free_netdev() function > > This is a release function for r8712_init_netdev(). I changed > the free_netdev() in r871xu_drv_init() to use this and I moved > the free_netdev() out of r8712_free_drv_sw() and into the > r871xu_dev_remove() function where it belongs. > > 365 padapter = netdev_priv(pnetdev); > 366 disable_ht_for_spec_devid(pdid, padapter); > 367 pdvobjpriv = &padapter->dvobjpriv; > 368 pdvobjpriv->padapter = padapter; > 369 padapter->dvobjpriv.pusbdev = udev; > 370 padapter->pusb_intf = pusb_intf; > 371 usb_set_intfdata(pusb_intf, pnetdev); > 372 SET_NETDEV_DEV(pnetdev, &pusb_intf->dev); > 373 pnetdev->dev.type = &wlan_type; > 374 /* step 2. */ > 375 padapter->dvobj_init = r8712_usb_dvobj_init; > 376 padapter->dvobj_deinit = r8712_usb_dvobj_deinit; > 377 padapter->halpriv.hal_bus_init = r8712_usb_hal_bus_init; > 378 padapter->dvobjpriv.inirp_init = r8712_usb_inirp_init; > 379 padapter->dvobjpriv.inirp_deinit = r8712_usb_inirp_deinit; > > These function pointers are garbage. We should try to move this code > to calling the functions directly and delete the pointers from the > struct. > > 380 /* step 3. > 381 * initialize the dvobj_priv > 382 */ > 383 if (!padapter->dvobj_init) { > 384 goto error; > > > We set ->dvobj_init on line 375 so it can't be NULL. Delete this. > > [PATCH 1/x] staging: rtl8712: delete impossible NULL check > > 385 } else { > 386 status = padapter->dvobj_init(padapter); > 387 if (status != _SUCCESS) > 388 goto error; > > Since we know that ->dvobj_init is r8712_usb_dvobj_init() then lets call > that directly. > > status = r8712_usb_dvobj_init(padapter); > if (status != _SUCCESS) { > ret = -ENOMEM; > goto free_netdev; > } > > [PATCH 2/x] staging: rtl8712: Get rid of ->dvobj_init/deinit function pointers > > The "padapter->dvobj_init/->dvobj_deinit" pointers are not required. > We can call the functions directly. > > This usb_dvobj (dumb name) is now our current resource. Unfortunately > the r8712_usb_dvobj_deinit() function is an empty stub function. It > should be: > > static void r8712_usb_dvobj_deinit(struct _adapter *padapter) > { > r8712_free_io_queue(padapter); > } > > Currently the call to r8712_free_io_queue() is hidden inside the > r8712_free_drv_sw() function but that's the wrong place for it. > > [PATCH 8/x] staging: rtl8712: implement r8712_usb_dvobj_deinit() > > The r8712_usb_dvobj_deinit() function is supposed to clean up from > r8712_usb_dvobj_deinit(). Currently that is open coded in > r8712_free_drv_sw(), but it should be implemented as a separate > function and called from r871xu_dev_remove(). > > 389 } > 390 /* step 4. */ > 391 status = r8712_init_drv_sw(padapter); > 392 if (status) > 393 goto error; > > ret = r8712_init_drv_sw(padapter); > if (ret) > goto free_usb_dvobj; > > The r8712_init_drv_sw() function does not clean up after itself on > error. > > [PATCH 3/x] staging: rtl8712: fix leaks in r8712_init_drv_sw(). > > The r8712_free_drv_sw() exists but it is not a mirror of the > r8712_init_drv_sw() function. As we've noticed, it frees some things > which it should not but it also leaves timers running so presumably > that leads to a use after free. > > [PATCH 9/x] staging: rtl8712: re-write r8712_free_drv_sw() > > The r8712_free_drv_sw() should clean up everything allocated in > r8712_init_drv_sw() but it does not. Some of it was done in > r8712_stop_drv_timers() and so I have moved it here and deleted > that code. > > PATCH 9 is slightly risky. Be careful not to introduce any double > frees! > > 394 /* step 5. read efuse/eeprom data and get mac_addr */ > 395 { > 396 int i, offset; > 397 u8 mac[6]; > 398 u8 tmpU1b, AutoloadFail, eeprom_CustomerID; > 399 u8 *pdata = padapter->eeprompriv.efuse_eeprom_data; > > Declare this at the top. Pull the code in one tab. > > [PATCH 4/x] staging: rtl8712: pull code in one tab > > 400 > 401 tmpU1b = r8712_read8(padapter, EE_9346CR);/*CR9346*/ > 402 > 403 /* To check system boot selection.*/ > 404 dev_info(&udev->dev, "r8712u: Boot from %s: Autoload %s\n", > 405 (tmpU1b & _9356SEL) ? "EEPROM" : "EFUSE", > 406 (tmpU1b & _EEPROM_EN) ? "OK" : "Failed"); > 407 > 408 /* To check autoload success or not.*/ > 409 if (tmpU1b & _EEPROM_EN) { > 410 AutoloadFail = true; > > Put the rest of this if statement after the "AutoloadFail = true;" into > a separate function. Set AutoloadFail = false at the top of the > function: > > bool AutoloadFail = false; > > [PATCH 5/x] staging: rtl8712: move code to a separate function > > > 411 /* The following operations prevent Efuse leakage by > 412 * turning on 2.5V. > 413 */ > 414 tmpU1b = r8712_read8(padapter, EFUSE_TEST + 3); > 415 r8712_write8(padapter, EFUSE_TEST + 3, tmpU1b | 0x80); > 416 msleep(20); > 417 r8712_write8(padapter, EFUSE_TEST + 3, > 418 (tmpU1b & (~BIT(7)))); > 419 > 420 /* Retrieve Chip version. > 421 * Recognize IC version by Reg0x4 BIT15. > 422 */ > 423 tmpU1b = (u8)((r8712_read32(padapter, PMC_FSM) >> 15) & > 424 0x1F); > 425 if (tmpU1b == 0x3) > 426 padapter->registrypriv.chip_version = > 427 RTL8712_3rdCUT; > 428 else > 429 padapter->registrypriv.chip_version = > 429 padapter->registrypriv.chip_version = > 430 (tmpU1b >> 1) + 1; > 431 switch (padapter->registrypriv.chip_version) { > 432 case RTL8712_1stCUT: > 433 case RTL8712_2ndCUT: > 434 case RTL8712_3rdCUT: > 435 break; > 436 default: > 437 padapter->registrypriv.chip_version = > 438 RTL8712_2ndCUT; > 439 break; > 440 } > 441 > 442 for (i = 0, offset = 0; i < 128; i += 8, offset++) > 443 r8712_efuse_pg_packet_read(padapter, offset, > 444 &pdata[i]); > 445 > 446 if (!r8712_initmac || !mac_pton(r8712_initmac, mac)) { > 447 /* Use the mac address stored in the Efuse > 448 * offset = 0x12 for usb in efuse > 449 */ > 450 ether_addr_copy(mac, &pdata[0x12]); > 451 } > 452 eeprom_CustomerID = pdata[0x52]; > 453 switch (eeprom_CustomerID) { > 454 case EEPROM_CID_ALPHA: > 455 padapter->eeprompriv.CustomerID = > 456 RT_CID_819x_ALPHA; > 457 break; > 458 case EEPROM_CID_CAMEO: > 459 padapter->eeprompriv.CustomerID = > 460 RT_CID_819x_CAMEO; > 461 break; > 462 case EEPROM_CID_SITECOM: > 463 padapter->eeprompriv.CustomerID = > 464 RT_CID_819x_Sitecom; > 465 break; > 466 case EEPROM_CID_COREGA: > 467 padapter->eeprompriv.CustomerID = > 468 RT_CID_COREGA; > 469 break; > 470 case EEPROM_CID_Senao: > 471 padapter->eeprompriv.CustomerID = > 472 RT_CID_819x_Senao; > 473 break; > 474 case EEPROM_CID_EDIMAX_BELKIN: > 475 padapter->eeprompriv.CustomerID = > 476 RT_CID_819x_Edimax_Belkin; > 477 break; > 478 case EEPROM_CID_SERCOMM_BELKIN: > 479 padapter->eeprompriv.CustomerID = > 480 RT_CID_819x_Sercomm_Belkin; > 481 break; > 482 case EEPROM_CID_WNC_COREGA: > 483 padapter->eeprompriv.CustomerID = > 484 RT_CID_819x_WNC_COREGA; > 485 break; > 486 case EEPROM_CID_WHQL: > 487 break; > 488 case EEPROM_CID_NetCore: > 489 padapter->eeprompriv.CustomerID = > 490 RT_CID_819x_Netcore; > 491 break; > 492 case EEPROM_CID_CAMEO1: > 493 padapter->eeprompriv.CustomerID = > 494 RT_CID_819x_CAMEO1; > 495 break; > 496 case EEPROM_CID_CLEVO: > 497 padapter->eeprompriv.CustomerID = > 498 RT_CID_819x_CLEVO; > 499 break; > 500 default: > 501 padapter->eeprompriv.CustomerID = > 502 RT_CID_DEFAULT; > 503 break; > 504 } > 505 dev_info(&udev->dev, "r8712u: CustomerID = 0x%.4x\n", > 506 padapter->eeprompriv.CustomerID); > 507 /* Led mode */ > 508 switch (padapter->eeprompriv.CustomerID) { > 509 case RT_CID_DEFAULT: > 510 case RT_CID_819x_ALPHA: > 510 case RT_CID_819x_ALPHA: > 511 case RT_CID_819x_CAMEO: > 512 padapter->ledpriv.LedStrategy = SW_LED_MODE1; > 513 padapter->ledpriv.bRegUseLed = true; > 514 break; > 515 case RT_CID_819x_Sitecom: > 516 padapter->ledpriv.LedStrategy = SW_LED_MODE2; > 517 padapter->ledpriv.bRegUseLed = true; > 518 break; > 519 case RT_CID_COREGA: > 520 case RT_CID_819x_Senao: > 521 padapter->ledpriv.LedStrategy = SW_LED_MODE3; > 522 padapter->ledpriv.bRegUseLed = true; > 523 break; > 524 case RT_CID_819x_Edimax_Belkin: > 525 padapter->ledpriv.LedStrategy = SW_LED_MODE4; > 526 padapter->ledpriv.bRegUseLed = true; > 527 break; > 528 case RT_CID_819x_Sercomm_Belkin: > 529 padapter->ledpriv.LedStrategy = SW_LED_MODE5; > 530 padapter->ledpriv.bRegUseLed = true; > 531 break; > 532 case RT_CID_819x_WNC_COREGA: > 533 padapter->ledpriv.LedStrategy = SW_LED_MODE6; > 534 padapter->ledpriv.bRegUseLed = true; > 535 break; > 536 default: > 537 padapter->ledpriv.LedStrategy = SW_LED_MODE0; > 538 padapter->ledpriv.bRegUseLed = false; > 539 break; > 540 } > 541 } else { > 542 AutoloadFail = false; > 543 } > 544 if (((mac[0] == 0xff) && (mac[1] == 0xff) && > 545 (mac[2] == 0xff) && (mac[3] == 0xff) && > 546 (mac[4] == 0xff) && (mac[5] == 0xff)) || > 547 ((mac[0] == 0x00) && (mac[1] == 0x00) && > 548 (mac[2] == 0x00) && (mac[3] == 0x00) && > 549 (mac[4] == 0x00) && (mac[5] == 0x00)) || > 550 (!AutoloadFail)) { > 551 mac[0] = 0x00; > 552 mac[1] = 0xe0; > 553 mac[2] = 0x4c; > 554 mac[3] = 0x87; > 555 mac[4] = 0x00; > 556 mac[5] = 0x00; > 557 } > 558 if (r8712_initmac) { > 559 /* Make sure the user did not select a multicast > 560 * address by setting bit 1 of first octet. > 561 */ > 562 mac[0] &= 0xFE; > 563 dev_info(&udev->dev, > 564 "r8712u: MAC Address from user = %pM\n", mac); > 565 } else { > 566 dev_info(&udev->dev, > 567 "r8712u: MAC Address from efuse = %pM\n", mac); > 568 } > 569 ether_addr_copy(pnetdev->dev_addr, mac); > 570 } > 571 /* step 6. Load the firmware asynchronously */ > 572 if (rtl871x_load_fw(padapter)) > 573 goto error; > > The big indent block didn't allocate anything so our most recent > allocation is still drv_sw. > > ret = rtl871x_load_fw(padapter); > if (ret) > goto free_drv_sw; > > 574 spin_lock_init(&padapter->lock_rx_ff0_filter); > 575 mutex_init(&padapter->mutex_start); > 576 return 0; > 577 error: > 578 usb_put_dev(udev); > 579 usb_set_intfdata(pusb_intf, NULL); > 580 if (padapter && padapter->dvobj_deinit) > 581 padapter->dvobj_deinit(padapter); > 582 if (pnetdev) > 583 free_netdev(pnetdev); > 584 return -ENODEV; > 585 } > > dvobj_deinit() is a no-op as discussed. This also doesn't release > the drv_sw. So there are some leaks. When we fix and implement the > mirrored clean up functions it becomes: > > return 0; > > free_drv_sw: > r8712_free_drv_sw(padapter); > free_usb_dvobj: > r8712_usb_dvobj_deinit(padapter); > free_netdev: > r8712_free_netdev(pnetdev); > put_dev: > usb_put_dev(udev); > usb_set_intfdata(pusb_intf, NULL); > > return ret; > } > > Now we can implement a remove function. It's complicated because > we don't know if the firmware loaded successfully and if the network > device is registered. We don't really need to test if (adapter->fw) > becaue release_firmware(NULL) is a no-op but I did it anyway. > > Based on what we know so far, this is what it should look like: > > static void r871xu_dev_remove(struct usb_interface *pusb_intf) > { > struct net_device *pnetdev = usb_get_intfdata(pusb_intf); > struct usb_device *udev = interface_to_usbdev(pusb_intf); > struct _adapter *padapter = netdev_priv(pnetdev); > > /* never exit with a firmware callback pending */ > wait_for_completion(&padapter->rtl8712_fw_ready); > if (pnetdev->reg_state != NETREG_UNINITIALIZED) > unregister_netdev(pnetdev); /* will call netdev_close() */ > if (adapter->fw) > release_firmware(padapter->fw); > r8712_free_drv_sw(padapter); > r8712_usb_dvobj_deinit(padapter); > r8712_free_netdev(pnetdev); > usb_put_dev(udev); > usb_set_intfdata(pusb_intf, NULL); > } > > The kernel's r871xu_dev_remove() look different so there are some > remaining questions. > > 585 static void r871xu_dev_remove(struct usb_interface *pusb_intf) > 586 { > 587 struct net_device *pnetdev = usb_get_intfdata(pusb_intf); > 588 struct usb_device *udev = interface_to_usbdev(pusb_intf); > 589 > 590 if (pnetdev) { > > These checks are no longer required now that we deleted the two lines > from rtl871x_load_fw_cb(). > > 591 struct _adapter *padapter = netdev_priv(pnetdev); > 592 > 593 /* never exit with a firmware callback pending */ > 594 wait_for_completion(&padapter->rtl8712_fw_ready); > 595 pnetdev = usb_get_intfdata(pusb_intf); > > This assignment is not required becuse "pnetdev" remains the same. > > 596 usb_set_intfdata(pusb_intf, NULL); > 597 if (!pnetdev) > 598 goto firmware_load_fail; > 599 release_firmware(padapter->fw); > 600 if (drvpriv.drv_registered) > 601 padapter->surprise_removed = true; > > All the "padapter->surprise_removed" code is bogus, but it needs to > be audited and deleted before any other fixes to the error handling can > be done. > > [PATCH 7/x] staging: rtl8712: delete padapter->surprise_removed > > > 602 if (pnetdev->reg_state != NETREG_UNINITIALIZED) > 603 unregister_netdev(pnetdev); /* will call netdev_close() */ > 604 flush_scheduled_work(); > 605 udelay(1); > > This is a layering violation. If it's really required then it needs to > be done in the correct location... > > 606 /* Stop driver mlme relation timer */ > 607 r8712_stop_drv_timers(padapter); > > Once r8712_free_drv_sw() is fixed I think it will take care of the > timers. > > 608 r871x_dev_unload(padapter); > > The r871x_dev_unload() stuff is suposed to be move to netdev_close(), I > think? > > 609 r8712_free_drv_sw(padapter); > 610 > 611 /* decrease the reference count of the usb device structure > 612 * when disconnect > 613 */ > > Pointless comment. > > 614 usb_put_dev(udev); > 615 } > 616 firmware_load_fail: > 617 /* If we didn't unplug usb dongle and remove/insert module, driver > 618 * fails on sitesurvey for the first time when device is up. > 619 * Reset usb port for sitesurvey fail issue. > 620 */ > 621 if (udev->state != USB_STATE_NOTATTACHED) > 622 usb_reset_device(udev); > > This feels wrong. Also it feels like using "udev" after calling > usb_put_dev(udev) would be a use after free, but I think the stuff > with usb_get/put_dev() is not really required so it doesn't matter. > > Anyway, leave this stuff. Even though, we might not like it we can't > change it without a way to test it using real hardware. That's the > same for the flush_scheduled_work() and the udelay(1). We don't like it > but we can't test it. > > 623 } > > It would probably take a 15 patch series to fix this code. The ordering > is important but slightly tricky so be careful with it. > > regards, > dan carpenter ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv @ 2021-05-25 14:32 ` 慕冬亮 0 siblings, 0 replies; 13+ messages in thread From: 慕冬亮 @ 2021-05-25 14:32 UTC (permalink / raw) To: Dan Carpenter Cc: Larry.Finger, florian.c.schilhabel, Greg KH, rkovhaev, straube.linux, linux-staging, linux-kernel On Tue, May 25, 2021 at 7:04 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > This is a syzbot warning, right? If so, it needs a Reported-by tag. Yes, https://syzkaller.appspot.com/bug?id=3a325b8389fc41c1bc94de0f4ac437ed13cce584 > > I don't really understand why rtl871x_load_fw_cb() calls: I am also confused about the invocation of rtl871x_load_fw_cb. In my local reproduction, if the firmware (rtlwifi/rtl8712u.bin) is missing, the memory leak occurs, otherwise, no memory leak is reported. However, my testing in the syzbot dashboard shows there is no invocation of rtl871x_load_fw_cb. If that's the fact, then I don't quite understand how this memory leak occurred. I doubt the bug I reproduced in the local environment may be not the bug in the syzbot dashboard. So I did not add Reported-by tag > > usb_put_dev(udev); > usb_set_intfdata(usb_intf, NULL); > > That just seems like a layering violation and it keeps causing bugs and > I think everything would be simpler if we deleted that. That way we > could remove both checks for if pnetdev is NULL. > The following description of patches is **really** friendly for newbies like me, especially for rules for allocation and deallocation. I will keep them in mind when developing patches in the future. Thanks very much. I really appreciate it, > [PATCH 10/x] staging: rtl8712: delete bogus clean up code in firmware callback > > This clean up is supposed to be done in r871xu_dev_remove(). > Setting the usb USB interface leads to leaks which syzbot detects. > <stack trace> > Reported-by: Sysbot blah blah > > Patch 10 because their are some other easier patches which could be > done first. Always do the easiest patch first in case a patch set needs > to be changed, it's easier to change the latter patches. > > Your patch fixes one sysbot warning but then syzbot will complain about > something else because there are a bunch of other things which need to > be freed as well. Of course, it's complicated to know which things need > to be freed and which not because the code is really bad. It's better > to just re-write the cleanup code and fix everything at once. > > I always encourage everyone to follow the "free the most recently > allocated resource" system of error handling. This is the only > *systematic* way I know to do error handling. Everything else is > ad-hoc, impossible to review and has proven bug prone in real life. > > The rules are: > 1) Every function cleans up it's own allocations. Never partially > allocate things. Never leave the caller to clean up your resources. > 2) Keep track in your head of the most recently allocated resource. > Keeping track of just one thing is a lot easier than keeping track of > everything. > 3) Use good label names which say what the labels free. > > err_free_netdev: > free_netdev(netdev); > > 4) Every allocator function has a mirror free function. > > How it works in real life is like this: > > int my_probe(...) > { > a = alloc(); > if (!a) > return -ENOMEM; > > b = alloc(); > if (!b) { > ret = -ENOMEM; > goto free_a; // <-- a is most recent allocation > } > > c = alloc(); > if (!c) { > ret = -ENOMEM; > goto free_b; > } > > return 0; > > free_b: > free(b); > free_a: > free(a); > > return ret; > } > > Then the mirror function basically writes its self. You just have to > cut and paste the clean up code and add a kfree(c) to the start. > > void my_release(...) > { > free(c); > free(b); > free(a); > } > > Once we move all the frees to the correct place and in the right order > then it becomes simpler. > > drivers/staging/rtl8712/usb_intf.c > 345 static int r871xu_drv_init(struct usb_interface *pusb_intf, > 346 const struct usb_device_id *pdid) > 347 { > 348 uint status; > > Keep status for status = padapter->dvobj_init() but everything else > should be int ret. Eventually "status" will be deleted. > > 349 struct _adapter *padapter = NULL; > 350 struct dvobj_priv *pdvobjpriv; > 351 struct net_device *pnetdev; > 352 struct usb_device *udev; > 353 > 354 /* In this probe function, O.S. will provide the usb interface pointer > 355 * to driver. We have to increase the reference count of the usb device > 356 * structure by using the usb_get_dev function. > 357 */ > 358 udev = interface_to_usbdev(pusb_intf); > 359 usb_get_dev(udev); > ^^^^^^^^^^^^^^^^^ > First resource allocation/thing to be freed. Is this really required? > I'm not sure. Anyway, it's harmless. > > 360 pintf = pusb_intf; > 361 /* step 1. */ > > Delete all these step 1,2,3... comments. The authors were trying to > keep track of what they had allocated but unfortunately writing it down > did not help them. > > 362 pnetdev = r8712_init_netdev(); > 363 if (!pnetdev) > 364 goto error; > > if (!pnetdev) { > ret = -ENOMEM; > goto put_dev; > } > > r8712_init_netdev() needs a free function. Right now the free_netdev() > is hidden in r8712_free_drv_sw() which is the wrong place. > > void r8712_free_netdev(struct net_device *dev) > { > free_netdev(dev); > } > > "pnetdev" is now our current resource. > > [PATCH 6/x] staging: rtl8712: create r8712_free_netdev() function > > This is a release function for r8712_init_netdev(). I changed > the free_netdev() in r871xu_drv_init() to use this and I moved > the free_netdev() out of r8712_free_drv_sw() and into the > r871xu_dev_remove() function where it belongs. > > 365 padapter = netdev_priv(pnetdev); > 366 disable_ht_for_spec_devid(pdid, padapter); > 367 pdvobjpriv = &padapter->dvobjpriv; > 368 pdvobjpriv->padapter = padapter; > 369 padapter->dvobjpriv.pusbdev = udev; > 370 padapter->pusb_intf = pusb_intf; > 371 usb_set_intfdata(pusb_intf, pnetdev); > 372 SET_NETDEV_DEV(pnetdev, &pusb_intf->dev); > 373 pnetdev->dev.type = &wlan_type; > 374 /* step 2. */ > 375 padapter->dvobj_init = r8712_usb_dvobj_init; > 376 padapter->dvobj_deinit = r8712_usb_dvobj_deinit; > 377 padapter->halpriv.hal_bus_init = r8712_usb_hal_bus_init; > 378 padapter->dvobjpriv.inirp_init = r8712_usb_inirp_init; > 379 padapter->dvobjpriv.inirp_deinit = r8712_usb_inirp_deinit; > > These function pointers are garbage. We should try to move this code > to calling the functions directly and delete the pointers from the > struct. > > 380 /* step 3. > 381 * initialize the dvobj_priv > 382 */ > 383 if (!padapter->dvobj_init) { > 384 goto error; > > > We set ->dvobj_init on line 375 so it can't be NULL. Delete this. > > [PATCH 1/x] staging: rtl8712: delete impossible NULL check > > 385 } else { > 386 status = padapter->dvobj_init(padapter); > 387 if (status != _SUCCESS) > 388 goto error; > > Since we know that ->dvobj_init is r8712_usb_dvobj_init() then lets call > that directly. > > status = r8712_usb_dvobj_init(padapter); > if (status != _SUCCESS) { > ret = -ENOMEM; > goto free_netdev; > } > > [PATCH 2/x] staging: rtl8712: Get rid of ->dvobj_init/deinit function pointers > > The "padapter->dvobj_init/->dvobj_deinit" pointers are not required. > We can call the functions directly. > > This usb_dvobj (dumb name) is now our current resource. Unfortunately > the r8712_usb_dvobj_deinit() function is an empty stub function. It > should be: > > static void r8712_usb_dvobj_deinit(struct _adapter *padapter) > { > r8712_free_io_queue(padapter); > } > > Currently the call to r8712_free_io_queue() is hidden inside the > r8712_free_drv_sw() function but that's the wrong place for it. > > [PATCH 8/x] staging: rtl8712: implement r8712_usb_dvobj_deinit() > > The r8712_usb_dvobj_deinit() function is supposed to clean up from > r8712_usb_dvobj_deinit(). Currently that is open coded in > r8712_free_drv_sw(), but it should be implemented as a separate > function and called from r871xu_dev_remove(). > > 389 } > 390 /* step 4. */ > 391 status = r8712_init_drv_sw(padapter); > 392 if (status) > 393 goto error; > > ret = r8712_init_drv_sw(padapter); > if (ret) > goto free_usb_dvobj; > > The r8712_init_drv_sw() function does not clean up after itself on > error. > > [PATCH 3/x] staging: rtl8712: fix leaks in r8712_init_drv_sw(). > > The r8712_free_drv_sw() exists but it is not a mirror of the > r8712_init_drv_sw() function. As we've noticed, it frees some things > which it should not but it also leaves timers running so presumably > that leads to a use after free. > > [PATCH 9/x] staging: rtl8712: re-write r8712_free_drv_sw() > > The r8712_free_drv_sw() should clean up everything allocated in > r8712_init_drv_sw() but it does not. Some of it was done in > r8712_stop_drv_timers() and so I have moved it here and deleted > that code. > > PATCH 9 is slightly risky. Be careful not to introduce any double > frees! > > 394 /* step 5. read efuse/eeprom data and get mac_addr */ > 395 { > 396 int i, offset; > 397 u8 mac[6]; > 398 u8 tmpU1b, AutoloadFail, eeprom_CustomerID; > 399 u8 *pdata = padapter->eeprompriv.efuse_eeprom_data; > > Declare this at the top. Pull the code in one tab. > > [PATCH 4/x] staging: rtl8712: pull code in one tab > > 400 > 401 tmpU1b = r8712_read8(padapter, EE_9346CR);/*CR9346*/ > 402 > 403 /* To check system boot selection.*/ > 404 dev_info(&udev->dev, "r8712u: Boot from %s: Autoload %s\n", > 405 (tmpU1b & _9356SEL) ? "EEPROM" : "EFUSE", > 406 (tmpU1b & _EEPROM_EN) ? "OK" : "Failed"); > 407 > 408 /* To check autoload success or not.*/ > 409 if (tmpU1b & _EEPROM_EN) { > 410 AutoloadFail = true; > > Put the rest of this if statement after the "AutoloadFail = true;" into > a separate function. Set AutoloadFail = false at the top of the > function: > > bool AutoloadFail = false; > > [PATCH 5/x] staging: rtl8712: move code to a separate function > > > 411 /* The following operations prevent Efuse leakage by > 412 * turning on 2.5V. > 413 */ > 414 tmpU1b = r8712_read8(padapter, EFUSE_TEST + 3); > 415 r8712_write8(padapter, EFUSE_TEST + 3, tmpU1b | 0x80); > 416 msleep(20); > 417 r8712_write8(padapter, EFUSE_TEST + 3, > 418 (tmpU1b & (~BIT(7)))); > 419 > 420 /* Retrieve Chip version. > 421 * Recognize IC version by Reg0x4 BIT15. > 422 */ > 423 tmpU1b = (u8)((r8712_read32(padapter, PMC_FSM) >> 15) & > 424 0x1F); > 425 if (tmpU1b == 0x3) > 426 padapter->registrypriv.chip_version = > 427 RTL8712_3rdCUT; > 428 else > 429 padapter->registrypriv.chip_version = > 429 padapter->registrypriv.chip_version = > 430 (tmpU1b >> 1) + 1; > 431 switch (padapter->registrypriv.chip_version) { > 432 case RTL8712_1stCUT: > 433 case RTL8712_2ndCUT: > 434 case RTL8712_3rdCUT: > 435 break; > 436 default: > 437 padapter->registrypriv.chip_version = > 438 RTL8712_2ndCUT; > 439 break; > 440 } > 441 > 442 for (i = 0, offset = 0; i < 128; i += 8, offset++) > 443 r8712_efuse_pg_packet_read(padapter, offset, > 444 &pdata[i]); > 445 > 446 if (!r8712_initmac || !mac_pton(r8712_initmac, mac)) { > 447 /* Use the mac address stored in the Efuse > 448 * offset = 0x12 for usb in efuse > 449 */ > 450 ether_addr_copy(mac, &pdata[0x12]); > 451 } > 452 eeprom_CustomerID = pdata[0x52]; > 453 switch (eeprom_CustomerID) { > 454 case EEPROM_CID_ALPHA: > 455 padapter->eeprompriv.CustomerID = > 456 RT_CID_819x_ALPHA; > 457 break; > 458 case EEPROM_CID_CAMEO: > 459 padapter->eeprompriv.CustomerID = > 460 RT_CID_819x_CAMEO; > 461 break; > 462 case EEPROM_CID_SITECOM: > 463 padapter->eeprompriv.CustomerID = > 464 RT_CID_819x_Sitecom; > 465 break; > 466 case EEPROM_CID_COREGA: > 467 padapter->eeprompriv.CustomerID = > 468 RT_CID_COREGA; > 469 break; > 470 case EEPROM_CID_Senao: > 471 padapter->eeprompriv.CustomerID = > 472 RT_CID_819x_Senao; > 473 break; > 474 case EEPROM_CID_EDIMAX_BELKIN: > 475 padapter->eeprompriv.CustomerID = > 476 RT_CID_819x_Edimax_Belkin; > 477 break; > 478 case EEPROM_CID_SERCOMM_BELKIN: > 479 padapter->eeprompriv.CustomerID = > 480 RT_CID_819x_Sercomm_Belkin; > 481 break; > 482 case EEPROM_CID_WNC_COREGA: > 483 padapter->eeprompriv.CustomerID = > 484 RT_CID_819x_WNC_COREGA; > 485 break; > 486 case EEPROM_CID_WHQL: > 487 break; > 488 case EEPROM_CID_NetCore: > 489 padapter->eeprompriv.CustomerID = > 490 RT_CID_819x_Netcore; > 491 break; > 492 case EEPROM_CID_CAMEO1: > 493 padapter->eeprompriv.CustomerID = > 494 RT_CID_819x_CAMEO1; > 495 break; > 496 case EEPROM_CID_CLEVO: > 497 padapter->eeprompriv.CustomerID = > 498 RT_CID_819x_CLEVO; > 499 break; > 500 default: > 501 padapter->eeprompriv.CustomerID = > 502 RT_CID_DEFAULT; > 503 break; > 504 } > 505 dev_info(&udev->dev, "r8712u: CustomerID = 0x%.4x\n", > 506 padapter->eeprompriv.CustomerID); > 507 /* Led mode */ > 508 switch (padapter->eeprompriv.CustomerID) { > 509 case RT_CID_DEFAULT: > 510 case RT_CID_819x_ALPHA: > 510 case RT_CID_819x_ALPHA: > 511 case RT_CID_819x_CAMEO: > 512 padapter->ledpriv.LedStrategy = SW_LED_MODE1; > 513 padapter->ledpriv.bRegUseLed = true; > 514 break; > 515 case RT_CID_819x_Sitecom: > 516 padapter->ledpriv.LedStrategy = SW_LED_MODE2; > 517 padapter->ledpriv.bRegUseLed = true; > 518 break; > 519 case RT_CID_COREGA: > 520 case RT_CID_819x_Senao: > 521 padapter->ledpriv.LedStrategy = SW_LED_MODE3; > 522 padapter->ledpriv.bRegUseLed = true; > 523 break; > 524 case RT_CID_819x_Edimax_Belkin: > 525 padapter->ledpriv.LedStrategy = SW_LED_MODE4; > 526 padapter->ledpriv.bRegUseLed = true; > 527 break; > 528 case RT_CID_819x_Sercomm_Belkin: > 529 padapter->ledpriv.LedStrategy = SW_LED_MODE5; > 530 padapter->ledpriv.bRegUseLed = true; > 531 break; > 532 case RT_CID_819x_WNC_COREGA: > 533 padapter->ledpriv.LedStrategy = SW_LED_MODE6; > 534 padapter->ledpriv.bRegUseLed = true; > 535 break; > 536 default: > 537 padapter->ledpriv.LedStrategy = SW_LED_MODE0; > 538 padapter->ledpriv.bRegUseLed = false; > 539 break; > 540 } > 541 } else { > 542 AutoloadFail = false; > 543 } > 544 if (((mac[0] == 0xff) && (mac[1] == 0xff) && > 545 (mac[2] == 0xff) && (mac[3] == 0xff) && > 546 (mac[4] == 0xff) && (mac[5] == 0xff)) || > 547 ((mac[0] == 0x00) && (mac[1] == 0x00) && > 548 (mac[2] == 0x00) && (mac[3] == 0x00) && > 549 (mac[4] == 0x00) && (mac[5] == 0x00)) || > 550 (!AutoloadFail)) { > 551 mac[0] = 0x00; > 552 mac[1] = 0xe0; > 553 mac[2] = 0x4c; > 554 mac[3] = 0x87; > 555 mac[4] = 0x00; > 556 mac[5] = 0x00; > 557 } > 558 if (r8712_initmac) { > 559 /* Make sure the user did not select a multicast > 560 * address by setting bit 1 of first octet. > 561 */ > 562 mac[0] &= 0xFE; > 563 dev_info(&udev->dev, > 564 "r8712u: MAC Address from user = %pM\n", mac); > 565 } else { > 566 dev_info(&udev->dev, > 567 "r8712u: MAC Address from efuse = %pM\n", mac); > 568 } > 569 ether_addr_copy(pnetdev->dev_addr, mac); > 570 } > 571 /* step 6. Load the firmware asynchronously */ > 572 if (rtl871x_load_fw(padapter)) > 573 goto error; > > The big indent block didn't allocate anything so our most recent > allocation is still drv_sw. > > ret = rtl871x_load_fw(padapter); > if (ret) > goto free_drv_sw; > > 574 spin_lock_init(&padapter->lock_rx_ff0_filter); > 575 mutex_init(&padapter->mutex_start); > 576 return 0; > 577 error: > 578 usb_put_dev(udev); > 579 usb_set_intfdata(pusb_intf, NULL); > 580 if (padapter && padapter->dvobj_deinit) > 581 padapter->dvobj_deinit(padapter); > 582 if (pnetdev) > 583 free_netdev(pnetdev); > 584 return -ENODEV; > 585 } > > dvobj_deinit() is a no-op as discussed. This also doesn't release > the drv_sw. So there are some leaks. When we fix and implement the > mirrored clean up functions it becomes: > > return 0; > > free_drv_sw: > r8712_free_drv_sw(padapter); > free_usb_dvobj: > r8712_usb_dvobj_deinit(padapter); > free_netdev: > r8712_free_netdev(pnetdev); > put_dev: > usb_put_dev(udev); > usb_set_intfdata(pusb_intf, NULL); > > return ret; > } > > Now we can implement a remove function. It's complicated because > we don't know if the firmware loaded successfully and if the network > device is registered. We don't really need to test if (adapter->fw) > becaue release_firmware(NULL) is a no-op but I did it anyway. > > Based on what we know so far, this is what it should look like: > > static void r871xu_dev_remove(struct usb_interface *pusb_intf) > { > struct net_device *pnetdev = usb_get_intfdata(pusb_intf); > struct usb_device *udev = interface_to_usbdev(pusb_intf); > struct _adapter *padapter = netdev_priv(pnetdev); > > /* never exit with a firmware callback pending */ > wait_for_completion(&padapter->rtl8712_fw_ready); > if (pnetdev->reg_state != NETREG_UNINITIALIZED) > unregister_netdev(pnetdev); /* will call netdev_close() */ > if (adapter->fw) > release_firmware(padapter->fw); > r8712_free_drv_sw(padapter); > r8712_usb_dvobj_deinit(padapter); > r8712_free_netdev(pnetdev); > usb_put_dev(udev); > usb_set_intfdata(pusb_intf, NULL); > } > > The kernel's r871xu_dev_remove() look different so there are some > remaining questions. > > 585 static void r871xu_dev_remove(struct usb_interface *pusb_intf) > 586 { > 587 struct net_device *pnetdev = usb_get_intfdata(pusb_intf); > 588 struct usb_device *udev = interface_to_usbdev(pusb_intf); > 589 > 590 if (pnetdev) { > > These checks are no longer required now that we deleted the two lines > from rtl871x_load_fw_cb(). > > 591 struct _adapter *padapter = netdev_priv(pnetdev); > 592 > 593 /* never exit with a firmware callback pending */ > 594 wait_for_completion(&padapter->rtl8712_fw_ready); > 595 pnetdev = usb_get_intfdata(pusb_intf); > > This assignment is not required becuse "pnetdev" remains the same. > > 596 usb_set_intfdata(pusb_intf, NULL); > 597 if (!pnetdev) > 598 goto firmware_load_fail; > 599 release_firmware(padapter->fw); > 600 if (drvpriv.drv_registered) > 601 padapter->surprise_removed = true; > > All the "padapter->surprise_removed" code is bogus, but it needs to > be audited and deleted before any other fixes to the error handling can > be done. > > [PATCH 7/x] staging: rtl8712: delete padapter->surprise_removed > > > 602 if (pnetdev->reg_state != NETREG_UNINITIALIZED) > 603 unregister_netdev(pnetdev); /* will call netdev_close() */ > 604 flush_scheduled_work(); > 605 udelay(1); > > This is a layering violation. If it's really required then it needs to > be done in the correct location... > > 606 /* Stop driver mlme relation timer */ > 607 r8712_stop_drv_timers(padapter); > > Once r8712_free_drv_sw() is fixed I think it will take care of the > timers. > > 608 r871x_dev_unload(padapter); > > The r871x_dev_unload() stuff is suposed to be move to netdev_close(), I > think? > > 609 r8712_free_drv_sw(padapter); > 610 > 611 /* decrease the reference count of the usb device structure > 612 * when disconnect > 613 */ > > Pointless comment. > > 614 usb_put_dev(udev); > 615 } > 616 firmware_load_fail: > 617 /* If we didn't unplug usb dongle and remove/insert module, driver > 618 * fails on sitesurvey for the first time when device is up. > 619 * Reset usb port for sitesurvey fail issue. > 620 */ > 621 if (udev->state != USB_STATE_NOTATTACHED) > 622 usb_reset_device(udev); > > This feels wrong. Also it feels like using "udev" after calling > usb_put_dev(udev) would be a use after free, but I think the stuff > with usb_get/put_dev() is not really required so it doesn't matter. > > Anyway, leave this stuff. Even though, we might not like it we can't > change it without a way to test it using real hardware. That's the > same for the flush_scheduled_work() and the udelay(1). We don't like it > but we can't test it. > > 623 } > > It would probably take a 15 patch series to fix this code. The ordering > is important but slightly tricky so be careful with it. > > regards, > dan carpenter ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-05-25 14:33 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-21 12:08 [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv Dongliang Mu 2021-05-21 12:09 ` 慕冬亮 2021-05-21 12:09 ` 慕冬亮 2021-05-21 12:18 ` Greg KH 2021-05-21 12:24 ` 慕冬亮 2021-05-21 12:24 ` 慕冬亮 2021-05-21 13:16 ` Greg KH 2021-05-21 13:42 ` 慕冬亮 2021-05-21 13:42 ` 慕冬亮 2021-05-24 11:49 Dongliang Mu 2021-05-25 11:03 ` Dan Carpenter 2021-05-25 14:32 ` 慕冬亮 2021-05-25 14:32 ` 慕冬亮
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.