* [PATCH] staging/rtl8712: fix potential memory leak
@ 2022-05-11 11:21 Bernard Zhao
2022-05-11 11:43 ` Greg Kroah-Hartman
0 siblings, 1 reply; 3+ messages in thread
From: Bernard Zhao @ 2022-05-11 11:21 UTC (permalink / raw)
To: Larry Finger, Florian Schilhabel, Greg Kroah-Hartman,
Bernard Zhao, linux-staging, linux-kernel
Cc: Bernard Zhao
This bug is found by google syzbot, the link is:
https://syzkaller.appspot.com/bug?id=3a325b8389fc41c1bc94de0f4ac437ed13cce584
memory leak log:
BUG: memory leak
unreferenced object 0xffff88810ff9b3c0 (size 192):
comm "kworker/0:2", pid 3653, jiffies 4294942228 (age 8.250s)
hex dump (first 32 bytes):
01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 00 00 00 00 d8 b3 f9 0f 81 88 ff ff ................
backtrace:
[<00000000e0748eb7>] usb_alloc_urb+0x66/0xe0
[<00000000fe5a9432>] r8712_os_recvbuf_resource_alloc+0x1b/0x80
[<00000000923fed72>] r8712_init_recv_priv+0x96/0x210
[<000000000038512f>] _r8712_init_recv_priv+0x134/0x150
[<0000000066e70a4e>] r8712_init_drv_sw+0xa0/0x1d0
[<000000001d2974c0>] r871xu_drv_init.cold+0x104/0x7d1
[<000000001d449ce2>] usb_probe_interface+0x177/0x370
[<00000000cd123d34>] really_probe+0x159/0x4a0
[<00000000364585cc>] driver_probe_device+0x84/0x100
[<0000000048b74bde>] __device_attach_driver+0xee/0x110
[<00000000c358ab15>] bus_for_each_drv+0xb7/0x100
[<00000000bfa9b076>] __device_attach+0x122/0x250
[<0000000048fe302a>] bus_probe_device+0xc6/0xe0
[<000000002ceae175>] device_add+0x5be/0xc30
[<00000000e4813a0d>] usb_set_configuration+0x9d9/0xb90
[<00000000cbb8c98f>] usb_generic_driver_probe+0x8c/0xc0
For this issue，I see that the following call sequence causing
some memory leaks:
usb_probe_interface
r871xu_drv_init
r8712_init_drv_sw
_r8712_init_recv_priv
r8712_init_recv_priv//void type function
for (i = 0; i < NR_RECVBUFF;
if (r8712_os_recvbuf_resource_alloc(padapter, precvbuf))
r8712_os_recvbuf_resource_alloc
precvbuf->purb = usb_alloc_urb
kmalloc
break;// if error branch. Here may be some memory leak,
// break directly after r8712_os_recvbuf_resource_alloc
// fail, and no cleanup operation is done.
And also the size of the memory leak can be seen in the log is
192 bytes, I check the size of the usb_alloc_urb application is
usb_alloc_urb(0,
-> kmalloc(struct_size(urb, iso_frame_desc, iso_packets))
-> sizeof(struct urb)+iso_packets*sizeof(struct iso_frame_desc)
iso_packets is 0, so the size of the actual application is
sizeof(struct urb) -> the calculation result is 192, which matches
the size of the leak point.
After that cleanup, the precvbuf->purb maybe used for long time
So I add kmemleak_not_leak to avoid false positive report.
This patch syzbot test OK:
2022/05/11 06:15 14m zhaojunkui2008@126.com patch upstream OK
Signed-off-by: Bernard Zhao <zhaojunkui2008@126.com>
Signed-off-by: Bernard Zhao <bernard@vivo.com>
---
drivers/staging/rtl8712/rtl8712_recv.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8712/rtl8712_recv.c b/drivers/staging/rtl8712/rtl8712_recv.c
index 0ffb30f1af7e..8bf8e6d5b005 100644
--- a/drivers/staging/rtl8712/rtl8712_recv.c
+++ b/drivers/staging/rtl8712/rtl8712_recv.c
@@ -19,6 +19,7 @@
#include <linux/if_ether.h>
#include <linux/ip.h>
#include <net/cfg80211.h>
+#include <linux/kmemleak.h>
#include "osdep_service.h"
#include "drv_types.h"
@@ -51,12 +52,20 @@ void r8712_init_recv_priv(struct recv_priv *precvpriv,
for (i = 0; i < NR_RECVBUFF; i++) {
INIT_LIST_HEAD(&precvbuf->list);
spin_lock_init(&precvbuf->recvbuf_lock);
- if (r8712_os_recvbuf_resource_alloc(padapter, precvbuf))
+ if (r8712_os_recvbuf_resource_alloc(padapter, precvbuf)) {
+ int j = i;
+
+ while (j-- > 0) {
+ r8712_os_recvbuf_resource_free(padapter, precvbuf);
+ precvbuf--;
+ }
break;
+ }
precvbuf->ref_cnt = 0;
precvbuf->adapter = padapter;
list_add_tail(&precvbuf->list,
&(precvpriv->free_recv_buf_queue.queue));
+ kmemleak_not_leak(precvbuf->purb);
precvbuf++;
}
precvpriv->free_recv_buf_queue_cnt = NR_RECVBUFF;
--
2.33.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] staging/rtl8712: fix potential memory leak 2022-05-11 11:21 [PATCH] staging/rtl8712: fix potential memory leak Bernard Zhao @ 2022-05-11 11:43 ` Greg Kroah-Hartman 2022-05-11 12:22 ` z 0 siblings, 1 reply; 3+ messages in thread From: Greg Kroah-Hartman @ 2022-05-11 11:43 UTC (permalink / raw) To: Bernard Zhao Cc: Larry Finger, Florian Schilhabel, linux-staging, linux-kernel, Bernard Zhao On Wed, May 11, 2022 at 04:21:44AM -0700, Bernard Zhao wrote: > This bug is found by google syzbot, the link is: > https://syzkaller.appspot.com/bug?id=3a325b8389fc41c1bc94de0f4ac437ed13cce584 > memory leak log: > BUG: memory leak > unreferenced object 0xffff88810ff9b3c0 (size 192): > comm "kworker/0:2", pid 3653, jiffies 4294942228 (age 8.250s) > hex dump (first 32 bytes): > 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > 00 00 00 00 00 00 00 00 d8 b3 f9 0f 81 88 ff ff ................ > backtrace: > [<00000000e0748eb7>] usb_alloc_urb+0x66/0xe0 > [<00000000fe5a9432>] r8712_os_recvbuf_resource_alloc+0x1b/0x80 > [<00000000923fed72>] r8712_init_recv_priv+0x96/0x210 > [<000000000038512f>] _r8712_init_recv_priv+0x134/0x150 > [<0000000066e70a4e>] r8712_init_drv_sw+0xa0/0x1d0 > [<000000001d2974c0>] r871xu_drv_init.cold+0x104/0x7d1 > [<000000001d449ce2>] usb_probe_interface+0x177/0x370 > [<00000000cd123d34>] really_probe+0x159/0x4a0 > [<00000000364585cc>] driver_probe_device+0x84/0x100 > [<0000000048b74bde>] __device_attach_driver+0xee/0x110 > [<00000000c358ab15>] bus_for_each_drv+0xb7/0x100 > [<00000000bfa9b076>] __device_attach+0x122/0x250 > [<0000000048fe302a>] bus_probe_device+0xc6/0xe0 > [<000000002ceae175>] device_add+0x5be/0xc30 > [<00000000e4813a0d>] usb_set_configuration+0x9d9/0xb90 > [<00000000cbb8c98f>] usb_generic_driver_probe+0x8c/0xc0 > > For this issue，I see that the following call sequence causing > some memory leaks: > usb_probe_interface > r871xu_drv_init > r8712_init_drv_sw > _r8712_init_recv_priv > r8712_init_recv_priv//void type function > for (i = 0; i < NR_RECVBUFF; > if (r8712_os_recvbuf_resource_alloc(padapter, precvbuf)) > r8712_os_recvbuf_resource_alloc > precvbuf->purb = usb_alloc_urb > kmalloc > > break;// if error branch. Here may be some memory leak, > // break directly after r8712_os_recvbuf_resource_alloc > // fail, and no cleanup operation is done. > > And also the size of the memory leak can be seen in the log is > 192 bytes, I check the size of the usb_alloc_urb application is > usb_alloc_urb(0, > -> kmalloc(struct_size(urb, iso_frame_desc, iso_packets)) > -> sizeof(struct urb)+iso_packets*sizeof(struct iso_frame_desc) > iso_packets is 0, so the size of the actual application is > sizeof(struct urb) -> the calculation result is 192, which matches > the size of the leak point. > > After that cleanup, the precvbuf->purb maybe used for long time > So I add kmemleak_not_leak to avoid false positive report. > > This patch syzbot test OK: > 2022/05/11 06:15 14m zhaojunkui2008@126.com patch upstream OK > > Signed-off-by: Bernard Zhao <zhaojunkui2008@126.com> > Signed-off-by: Bernard Zhao <bernard@vivo.com> You can not sign off on the same patch by the same person multiple times as this is a legal statement. > --- > drivers/staging/rtl8712/rtl8712_recv.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/rtl8712/rtl8712_recv.c b/drivers/staging/rtl8712/rtl8712_recv.c > index 0ffb30f1af7e..8bf8e6d5b005 100644 > --- a/drivers/staging/rtl8712/rtl8712_recv.c > +++ b/drivers/staging/rtl8712/rtl8712_recv.c > @@ -19,6 +19,7 @@ > #include <linux/if_ether.h> > #include <linux/ip.h> > #include <net/cfg80211.h> > +#include <linux/kmemleak.h> > > #include "osdep_service.h" > #include "drv_types.h" > @@ -51,12 +52,20 @@ void r8712_init_recv_priv(struct recv_priv *precvpriv, > for (i = 0; i < NR_RECVBUFF; i++) { > INIT_LIST_HEAD(&precvbuf->list); > spin_lock_init(&precvbuf->recvbuf_lock); > - if (r8712_os_recvbuf_resource_alloc(padapter, precvbuf)) > + if (r8712_os_recvbuf_resource_alloc(padapter, precvbuf)) { > + int j = i; > + > + while (j-- > 0) { > + r8712_os_recvbuf_resource_free(padapter, precvbuf); > + precvbuf--; > + } > break; > + } > precvbuf->ref_cnt = 0; > precvbuf->adapter = padapter; > list_add_tail(&precvbuf->list, > &(precvpriv->free_recv_buf_queue.queue)); > + kmemleak_not_leak(precvbuf->purb); This should not be needed, that's an indication that something is really wrong in the driver. Where is the urb really freed? You should not have to say that this urb has not leaked if it really has not leaked. Clean it up properly if it needs to be cleaned up here, but that's not usually where an urb is cleaned up at all. This feels wrong, sorry. thanks, greg k-h ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re:Re: [PATCH] staging/rtl8712: fix potential memory leak 2022-05-11 11:43 ` Greg Kroah-Hartman @ 2022-05-11 12:22 ` z 0 siblings, 0 replies; 3+ messages in thread From: z @ 2022-05-11 12:22 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Larry Finger, Florian Schilhabel, linux-staging, linux-kernel, Bernard Zhao At 2022-05-11 19:43:30, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> wrote: >On Wed, May 11, 2022 at 04:21:44AM -0700, Bernard Zhao wrote: >> This bug is found by google syzbot, the link is: >> https://syzkaller.appspot.com/bug?id=3a325b8389fc41c1bc94de0f4ac437ed13cce584 >> memory leak log: >> BUG: memory leak >> unreferenced object 0xffff88810ff9b3c0 (size 192): >> comm "kworker/0:2", pid 3653, jiffies 4294942228 (age 8.250s) >> hex dump (first 32 bytes): >> 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >> 00 00 00 00 00 00 00 00 d8 b3 f9 0f 81 88 ff ff ................ >> backtrace: >> [<00000000e0748eb7>] usb_alloc_urb+0x66/0xe0 >> [<00000000fe5a9432>] r8712_os_recvbuf_resource_alloc+0x1b/0x80 >> [<00000000923fed72>] r8712_init_recv_priv+0x96/0x210 >> [<000000000038512f>] _r8712_init_recv_priv+0x134/0x150 >> [<0000000066e70a4e>] r8712_init_drv_sw+0xa0/0x1d0 >> [<000000001d2974c0>] r871xu_drv_init.cold+0x104/0x7d1 >> [<000000001d449ce2>] usb_probe_interface+0x177/0x370 >> [<00000000cd123d34>] really_probe+0x159/0x4a0 >> [<00000000364585cc>] driver_probe_device+0x84/0x100 >> [<0000000048b74bde>] __device_attach_driver+0xee/0x110 >> [<00000000c358ab15>] bus_for_each_drv+0xb7/0x100 >> [<00000000bfa9b076>] __device_attach+0x122/0x250 >> [<0000000048fe302a>] bus_probe_device+0xc6/0xe0 >> [<000000002ceae175>] device_add+0x5be/0xc30 >> [<00000000e4813a0d>] usb_set_configuration+0x9d9/0xb90 >> [<00000000cbb8c98f>] usb_generic_driver_probe+0x8c/0xc0 >> >> For this issue，I see that the following call sequence causing >> some memory leaks: >> usb_probe_interface >> r871xu_drv_init >> r8712_init_drv_sw >> _r8712_init_recv_priv >> r8712_init_recv_priv//void type function >> for (i = 0; i < NR_RECVBUFF; >> if (r8712_os_recvbuf_resource_alloc(padapter, precvbuf)) >> r8712_os_recvbuf_resource_alloc >> precvbuf->purb = usb_alloc_urb >> kmalloc >> >> break;// if error branch. Here may be some memory leak, >> // break directly after r8712_os_recvbuf_resource_alloc >> // fail, and no cleanup operation is done. >> >> And also the size of the memory leak can be seen in the log is >> 192 bytes, I check the size of the usb_alloc_urb application is >> usb_alloc_urb(0, >> -> kmalloc(struct_size(urb, iso_frame_desc, iso_packets)) >> -> sizeof(struct urb)+iso_packets*sizeof(struct iso_frame_desc) >> iso_packets is 0, so the size of the actual application is >> sizeof(struct urb) -> the calculation result is 192, which matches >> the size of the leak point. >> >> After that cleanup, the precvbuf->purb maybe used for long time >> So I add kmemleak_not_leak to avoid false positive report. >> >> This patch syzbot test OK: >> 2022/05/11 06:15 14m zhaojunkui2008@126.com patch upstream OK >> >> Signed-off-by: Bernard Zhao <zhaojunkui2008@126.com> >> Signed-off-by: Bernard Zhao <bernard@vivo.com> > >You can not sign off on the same patch by the same person multiple times >as this is a legal statement. Hi greg k-h: Thanks for pointing out my mistake, I will correct it in my future submissions. >> --- >> drivers/staging/rtl8712/rtl8712_recv.c | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/staging/rtl8712/rtl8712_recv.c b/drivers/staging/rtl8712/rtl8712_recv.c >> index 0ffb30f1af7e..8bf8e6d5b005 100644 >> --- a/drivers/staging/rtl8712/rtl8712_recv.c >> +++ b/drivers/staging/rtl8712/rtl8712_recv.c >> @@ -19,6 +19,7 @@ >> #include <linux/if_ether.h> >> #include <linux/ip.h> >> #include <net/cfg80211.h> >> +#include <linux/kmemleak.h> >> >> #include "osdep_service.h" >> #include "drv_types.h" >> @@ -51,12 +52,20 @@ void r8712_init_recv_priv(struct recv_priv *precvpriv, >> for (i = 0; i < NR_RECVBUFF; i++) { >> INIT_LIST_HEAD(&precvbuf->list); >> spin_lock_init(&precvbuf->recvbuf_lock); >> - if (r8712_os_recvbuf_resource_alloc(padapter, precvbuf)) >> + if (r8712_os_recvbuf_resource_alloc(padapter, precvbuf)) { >> + int j = i; >> + >> + while (j-- > 0) { >> + r8712_os_recvbuf_resource_free(padapter, precvbuf); >> + precvbuf--; >> + } >> break; >> + } >> precvbuf->ref_cnt = 0; >> precvbuf->adapter = padapter; >> list_add_tail(&precvbuf->list, >> &(precvpriv->free_recv_buf_queue.queue)); >> + kmemleak_not_leak(precvbuf->purb); > >This should not be needed, that's an indication that something is really >wrong in the driver. Where is the urb really freed? > >You should not have to say that this urb has not leaked if it really has >not leaked. Clean it up properly if it needs to be cleaned up here, but >that's not usually where an urb is cleaned up at all. > The really free call sequence is done in r8712_free_drv_sw, like the follow error branch: r871xu_drv_init if (status) goto dvobj_deinit r8712_free_drv_sw _r8712_free_recv_priv r8712_free_recv_priv for (i = 0; i < NR_RECVBUFF; i++) r8712_os_recvbuf_resource_free if (precvbuf->pskb) dev_kfree_skb_any(precvbuf->pskb) if (precvbuf->purb) usb_kill_urb(precvbuf->purb) usb_free_urb(precvbuf->purb) I checked the caller's error branch, they call r8712_free_drv_sw to do the cleanup job, i throught the caller is OK. And my test from sysbot shows that: Before i add follow code, the memleak is almost 62 times,after my change, the memleak number change to 7. before [ 93.070089][T10847] kmemleak: 62 new suspected memory leaks (see /sys/kernel/debug/kmemleak) after fix: [ 77.557355][ T4098] kmemleak: 7 new suspected memory leaks (see /sys/kernel/debug/kmemleak) I throught the remain 7 is the right use, so i add kmemleak_not_leak. I am not sure if there is some gap. Kindly help to correct me if I'm missing something, thanks! BR//Bernard >This feels wrong, sorry. > >thanks, > >greg k-h ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-05-11 12:55 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-11 11:21 [PATCH] staging/rtl8712: fix potential memory leak Bernard Zhao 2022-05-11 11:43 ` Greg Kroah-Hartman 2022-05-11 12:22 ` z
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).