Re: [syzbot] [staging?] [usb?] memory leak in _r8712_init_xmit_priv (2)

[Nam Cao <namcao@linutronix.de>]

On Wed, May 22, 2024 at 06:33:57AM -0700, Nikita Zhandarovich wrote:
> On 5/20/24 10:18, Nam Cao wrote:
> > On Mon, May 20, 2024 at 07:46:41AM -0700, Nikita Zhandarovich wrote:
> >>> BUG: memory leak
> >>> unreferenced object 0xffff888107a5c000 (size 4096):
> >>>   comm "kworker/1:0", pid 22, jiffies 4294943134 (age 18.720s)
> >>>   hex dump (first 32 bytes):
> >>>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >>>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >>>   backtrace:
> >>>     [<ffffffff816337cd>] kmemleak_alloc_recursive include/linux/kmemleak.h:42 [inline]
> >>>     [<ffffffff816337cd>] slab_post_alloc_hook mm/slab.h:766 [inline]
> >>>     [<ffffffff816337cd>] slab_alloc_node mm/slub.c:3478 [inline]
> >>>     [<ffffffff816337cd>] __kmem_cache_alloc_node+0x2dd/0x3f0 mm/slub.c:3517
> >>>     [<ffffffff8157e625>] kmalloc_trace+0x25/0x90 mm/slab_common.c:1098
> >>>     [<ffffffff83cee442>] kmalloc include/linux/slab.h:600 [inline]
> >>>     [<ffffffff83cee442>] _r8712_init_xmit_priv+0x2b2/0x6e0 drivers/staging/rtl8712/rtl871x_xmit.c:130
> >>>     [<ffffffff83ce9033>] r8712_init_drv_sw+0xc3/0x290 drivers/staging/rtl8712/os_intfs.c:311
> >>>     [<ffffffff83ce7ce6>] r871xu_drv_init+0x1c6/0x920 drivers/staging/rtl8712/usb_intf.c:386
> >>>     [<ffffffff832d0f0b>] usb_probe_interface+0x16b/0x3a0 drivers/usb/core/driver.c:396
> >>>     [<ffffffff82c3bb06>] call_driver_probe drivers/base/dd.c:579 [inline]
> >>
> >> I am inclined to think that this issue might be false positive. During
> >> repro the device is initialized correctly, does some work and then
> >> exits, calling all required functions to clean things up
> >> (i.e. _free_xmit_priv()), including pxmitbuf->pallocated_buf.
> >> Kmemleak triggers disappear if you set longer intervals between
> >> scannning for them (obviously). And if all the things get cleared up
> >> when the device disconnects, isn't that correct and expected
> >> behaviour? Could the scanner just "lose track" of some of the objects
> >> here?

I think you may be right that this is false negative.

I am guessing that kmemleak scans memory for pointers in block of 8-byte.
However, this driver aligns the buffer from kmalloc() to 4 bytes, which is
not necessary because pointers from kmalloc() is at least 8-byte-aligned.
Then more pointers are stored in this 4-byte-aligned buffer. Thus, kmemleak
misses these pointers, and falsely report memory leak.

I never interacted with syzbot before, let's hope it can catch this:

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

diff --git a/drivers/staging/rtl8712/rtl871x_xmit.c b/drivers/staging/rtl8712/rtl871x_xmit.c
index 6353dbe554d3..408616e9afcf 100644
--- a/drivers/staging/rtl8712/rtl871x_xmit.c
+++ b/drivers/staging/rtl8712/rtl871x_xmit.c
@@ -117,12 +117,9 @@ int _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
 	/*init xmit_buf*/
 	_init_queue(&pxmitpriv->free_xmitbuf_queue);
 	_init_queue(&pxmitpriv->pending_xmitbuf_queue);
-	pxmitpriv->pallocated_xmitbuf =
-		kmalloc(NR_XMITBUFF * sizeof(struct xmit_buf) + 4, GFP_ATOMIC);
-	if (!pxmitpriv->pallocated_xmitbuf)
+	pxmitpriv->pxmitbuf = kmalloc(NR_XMITBUFF * sizeof(struct xmit_buf), GFP_ATOMIC);
+	if (!pxmitpriv->pxmitbuf)
 		goto clean_up_frame_buf;
-	pxmitpriv->pxmitbuf = pxmitpriv->pallocated_xmitbuf + 4 -
-			      ((addr_t)(pxmitpriv->pallocated_xmitbuf) & 3);
 	pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
 	for (i = 0; i < NR_XMITBUFF; i++) {
 		INIT_LIST_HEAD(&pxmitbuf->list);
@@ -165,8 +162,8 @@ int _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
 		for (k = 0; k < 8; k++)		/* delete xmit urb's */
 			usb_free_urb(pxmitbuf->pxmit_urb[k]);
 	}
-	kfree(pxmitpriv->pallocated_xmitbuf);
-	pxmitpriv->pallocated_xmitbuf = NULL;
+	kfree(pxmitpriv->pxmitbuf);
+	pxmitpriv->pxmitbuf = NULL;
 clean_up_frame_buf:
 	kfree(pxmitpriv->pallocated_frame_buf);
 	pxmitpriv->pallocated_frame_buf = NULL;
@@ -193,7 +190,7 @@ void _free_xmit_priv(struct xmit_priv *pxmitpriv)
 		pxmitbuf++;
 	}
 	kfree(pxmitpriv->pallocated_frame_buf);
-	kfree(pxmitpriv->pallocated_xmitbuf);
+	kfree(pxmitpriv->pxmitbuf);
 	free_hwxmits(padapter);
 }
 
diff --git a/drivers/staging/rtl8712/rtl871x_xmit.h b/drivers/staging/rtl8712/rtl871x_xmit.h
index cdcbc87a3cad..784172c385e3 100644
--- a/drivers/staging/rtl8712/rtl871x_xmit.h
+++ b/drivers/staging/rtl8712/rtl871x_xmit.h
@@ -244,7 +244,6 @@ struct	xmit_priv {
 	int cmdseq;
 	struct  __queue free_xmitbuf_queue;
 	struct  __queue pending_xmitbuf_queue;
-	u8 *pallocated_xmitbuf;
 	u8 *pxmitbuf;
 	uint free_xmitbuf_cnt;
 };