[PATCH] staging: rtl8712: remove unnecessary alignment of pxmitpriv->pxmitbuf

[PATCH] staging: rtl8712: remove unnecessary alignment of pxmitpriv->pxmitbuf

[Nam Cao]

This driver wants pxmitpriv->pxmitbuf to be 4-byte aligned. This is ensured
by allocating 4 more bytes than required with kmalloc(), then do the
p = p + 4 - (p & 3) trick to make sure the pointer is 4-byte aligned.

This is unnecessary. Pointers from kmalloc() are already at least
8-byte-aligned.

Remove this alignment trick to simplify the code, and also to stop wasting
4 extra bytes of dynamic memory allocator.

This also gets rid of a (false) warning from kmemleak. This 4-byte-aligned
buffer is used to store pointers from kmalloc(). For 64-bit platforms,
pointer size is 8 bytes and kmemleak only scans for pointers in 8-byte
blocks, thus it misses the pointers stored in this 4-byte-aligned buffer
and thinks that these pointers have been leaked. This is just a false
warning, not a real problem. But still, it would be nice to get rid of
these warnings.

Reported-and-tested-by: syzbot+83763e624cfec6b462cb@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-staging/000000000000809328060a8a4c1c@google.com
Signed-off-by: Nam Cao <namcao@linutronix.de>
---
Patch sent using this driver with this patch applied.

 drivers/staging/rtl8712/rtl871x_xmit.c | 13 +++++--------
 drivers/staging/rtl8712/rtl871x_xmit.h |  1 -
 2 files changed, 5 insertions(+), 9 deletions(-)

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;
 };
-- 
2.39.2

Re: [PATCH] staging: rtl8712: remove unnecessary alignment of pxmitpriv->pxmitbuf

[Dan Carpenter]

On Sat, May 25, 2024 at 09:32:29AM +0200, Nam Cao wrote:
> This driver wants pxmitpriv->pxmitbuf to be 4-byte aligned. This is ensured
> by allocating 4 more bytes than required with kmalloc(), then do the
> p = p + 4 - (p & 3) trick to make sure the pointer is 4-byte aligned.
> 
> This is unnecessary. Pointers from kmalloc() are already at least
> 8-byte-aligned.
> 
> Remove this alignment trick to simplify the code, and also to stop wasting
> 4 extra bytes of dynamic memory allocator.
> 
> This also gets rid of a (false) warning from kmemleak. This 4-byte-aligned
> buffer is used to store pointers from kmalloc(). For 64-bit platforms,
> pointer size is 8 bytes and kmemleak only scans for pointers in 8-byte
> blocks, thus it misses the pointers stored in this 4-byte-aligned buffer
> and thinks that these pointers have been leaked. This is just a false
> warning, not a real problem. But still, it would be nice to get rid of
> these warnings.

Are you sure it's a false positive?  I've always wondered what happens
when you do:

	p = kmalloc();
	kfree((char *)p + 4);

regards,
dan carpenter

Re: [PATCH] staging: rtl8712: remove unnecessary alignment of pxmitpriv->pxmitbuf

[Dan Carpenter]

On Mon, May 27, 2024 at 10:06:22AM +0300, Dan Carpenter wrote:
> On Sat, May 25, 2024 at 09:32:29AM +0200, Nam Cao wrote:
> > This driver wants pxmitpriv->pxmitbuf to be 4-byte aligned. This is ensured
> > by allocating 4 more bytes than required with kmalloc(), then do the
> > p = p + 4 - (p & 3) trick to make sure the pointer is 4-byte aligned.
> > 
> > This is unnecessary. Pointers from kmalloc() are already at least
> > 8-byte-aligned.
> > 
> > Remove this alignment trick to simplify the code, and also to stop wasting
> > 4 extra bytes of dynamic memory allocator.
> > 
> > This also gets rid of a (false) warning from kmemleak. This 4-byte-aligned
> > buffer is used to store pointers from kmalloc(). For 64-bit platforms,
> > pointer size is 8 bytes and kmemleak only scans for pointers in 8-byte
> > blocks, thus it misses the pointers stored in this 4-byte-aligned buffer
> > and thinks that these pointers have been leaked. This is just a false
> > warning, not a real problem. But still, it would be nice to get rid of
> > these warnings.
> 
> Are you sure it's a false positive?  I've always wondered what happens
> when you do:
> 
> 	p = kmalloc();
> 	kfree((char *)p + 4);

Ah, never mind, it actually frees p.

It took me a while to figure out that it's actually pointers stored in
the buffer which are reported as leaks and not the buffer itself.  This
was explained quite well in the commit message but I just misunderstood.

regards,
dan carpenter