[PATCH] rsi: Free the unaligned pointer

[PATCH] rsi: Free the unaligned pointer

[Dan Carpenter]

The problem here is that we allocate "data".  Then we do
"data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and
not the one we allocated.

I don't know if it causes an issue in real life, but it seems like a
reasonable thing to free the same pointer that we allocated.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
index d76e69c0beaa..ca4e698ab69b 100644
--- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
+++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
@@ -652,11 +652,11 @@ static int rsi_sdio_load_data_master_write(struct rsi_hw *adapter,
 static int rsi_sdio_master_reg_read(struct rsi_hw *adapter, u32 addr,
 				    u32 *read_buf, u16 size)
 {
-	u32 addr_on_bus, *data;
+	u32 addr_on_bus, *data, *data_orig;
 	u16 ms_addr;
 	int status;
 
-	data = kzalloc(RSI_MASTER_REG_BUF_SIZE, GFP_KERNEL);
+	data = data_orig = kzalloc(RSI_MASTER_REG_BUF_SIZE, GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
@@ -709,7 +709,7 @@ static int rsi_sdio_master_reg_read(struct rsi_hw *adapter, u32 addr,
 	}
 
 err:
-	kfree(data);
+	kfree(data_orig);
 	return status;
 }
 
@@ -717,10 +717,10 @@ static int rsi_sdio_master_reg_write(struct rsi_hw *adapter,
 				     unsigned long addr,
 				     unsigned long data, u16 size)
 {
-	unsigned long *data_aligned;
+	unsigned long *data_aligned, *data_orig;
 	int status;
 
-	data_aligned = kzalloc(RSI_MASTER_REG_BUF_SIZE, GFP_KERNEL);
+	data_aligned = data_orig = kzalloc(RSI_MASTER_REG_BUF_SIZE, GFP_KERNEL);
 	if (!data_aligned)
 		return -ENOMEM;
 
@@ -743,7 +743,7 @@ static int rsi_sdio_master_reg_write(struct rsi_hw *adapter,
 		rsi_dbg(ERR_ZONE,
 			"%s: Unable to set ms word to common reg\n",
 			__func__);
-		kfree(data_aligned);
+		kfree(data_orig);
 		return -EIO;
 	}
 	addr = addr & 0xFFFF;
@@ -757,7 +757,7 @@ static int rsi_sdio_master_reg_write(struct rsi_hw *adapter,
 		rsi_dbg(ERR_ZONE,
 			"%s: Unable to do AHB reg write\n", __func__);
 
-	kfree(data_aligned);
+	kfree(data_orig);
 	return status;
 }
 

Re: [PATCH] rsi: Free the unaligned pointer

[Johannes Berg]

On Thu, 2018-04-05 at 14:23 +0300, Dan Carpenter wrote:
> The problem here is that we allocate "data".  Then we do
> "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and
> not the one we allocated.

That seems pretty pointless, since kmalloc guarantees such alignment for
sure. Better to just remove PTR_ALIGN()?

johannes

Re: [PATCH] rsi: Free the unaligned pointer

[Dan Carpenter]

On Thu, Apr 05, 2018 at 01:30:35PM +0200, Johannes Berg wrote:
> On Thu, 2018-04-05 at 14:23 +0300, Dan Carpenter wrote:
> > The problem here is that we allocate "data".  Then we do
> > "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and
> > not the one we allocated.
> 
> That seems pretty pointless, since kmalloc guarantees such alignment for
> sure. Better to just remove PTR_ALIGN()?

Yeah.  You're probably right.  I was thinking that maybe
ARCH_SLAB_MINALIGN was smaller than 8 somewhere but look it it now, I
think it's always 8 or more.

Let me resend with the ALIGN() removed.

regards,
dan carpenter

Re: [PATCH] rsi: Free the unaligned pointer

[Dan Carpenter]

On Thu, Apr 05, 2018 at 02:39:31PM +0300, Dan Carpenter wrote:
> On Thu, Apr 05, 2018 at 01:30:35PM +0200, Johannes Berg wrote:
> > On Thu, 2018-04-05 at 14:23 +0300, Dan Carpenter wrote:
> > > The problem here is that we allocate "data".  Then we do
> > > "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and
> > > not the one we allocated.
> > 
> > That seems pretty pointless, since kmalloc guarantees such alignment for
> > sure. Better to just remove PTR_ALIGN()?
> 
> Yeah.  You're probably right.  I was thinking that maybe
> ARCH_SLAB_MINALIGN was smaller than 8 somewhere but look it it now, I
> think it's always 8 or more.
> 

Perhaps on certain xtensa variants?

arch/xtensa/include/asm/processor.h:#define ARCH_SLAB_MINALIGN  XCHAL_DATA_WIDTH
arch/xtensa/variants/fsf/include/variant/core.h:#define XCHAL_DATA_WIDTH                4       /* data width in bytes */

regards,
dan carpenter

Re: [PATCH] rsi: Free the unaligned pointer

[Johannes Berg]

On Thu, 2018-04-05 at 14:41 +0300, Dan Carpenter wrote:
> On Thu, Apr 05, 2018 at 02:39:31PM +0300, Dan Carpenter wrote:
> > On Thu, Apr 05, 2018 at 01:30:35PM +0200, Johannes Berg wrote:
> > > On Thu, 2018-04-05 at 14:23 +0300, Dan Carpenter wrote:
> > > > The problem here is that we allocate "data".  Then we do
> > > > "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and
> > > > not the one we allocated.
> > > 
> > > That seems pretty pointless, since kmalloc guarantees such alignment for
> > > sure. Better to just remove PTR_ALIGN()?
> > 
> > Yeah.  You're probably right.  I was thinking that maybe
> > ARCH_SLAB_MINALIGN was smaller than 8 somewhere but look it it now, I
> > think it's always 8 or more.
> > 
> 
> Perhaps on certain xtensa variants?
> 
> arch/xtensa/include/asm/processor.h:#define ARCH_SLAB_MINALIGN  XCHAL_DATA_WIDTH
> arch/xtensa/variants/fsf/include/variant/core.h:#define XCHAL_DATA_WIDTH                4       /* data width in bytes */

That's ... interesting. The comment on the original of this says it's
supposed to be used for "better alignment" (more zero bits), and I'd
think that there's lots of code making such assumptions...

I'd argue it's an xtensa bug, if we need to deal with this everywhere
then it might get messy. Mostly we don't have to care, since pointer
alignment is sufficient in many cases, but still...

Hmm. Dunno what to do here then.

johannes

Re: [PATCH] rsi: Free the unaligned pointer

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Thu, 2018-04-05 at 14:41 +0300, Dan Carpenter wrote:
>> On Thu, Apr 05, 2018 at 02:39:31PM +0300, Dan Carpenter wrote:
>> > On Thu, Apr 05, 2018 at 01:30:35PM +0200, Johannes Berg wrote:
>> > > On Thu, 2018-04-05 at 14:23 +0300, Dan Carpenter wrote:
>> > > > The problem here is that we allocate "data".  Then we do
>> > > > "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and
>> > > > not the one we allocated.
>> > > 
>> > > That seems pretty pointless, since kmalloc guarantees such alignment for
>> > > sure. Better to just remove PTR_ALIGN()?
>> > 
>> > Yeah.  You're probably right.  I was thinking that maybe
>> > ARCH_SLAB_MINALIGN was smaller than 8 somewhere but look it it now, I
>> > think it's always 8 or more.
>> > 
>> 
>> Perhaps on certain xtensa variants?
>> 
>> arch/xtensa/include/asm/processor.h:#define ARCH_SLAB_MINALIGN  XCHAL_DATA_WIDTH
>> arch/xtensa/variants/fsf/include/variant/core.h:#define
>> XCHAL_DATA_WIDTH 4 /* data width in bytes */
>
> That's ... interesting. The comment on the original of this says it's
> supposed to be used for "better alignment" (more zero bits), and I'd
> think that there's lots of code making such assumptions...
>
> I'd argue it's an xtensa bug, if we need to deal with this everywhere
> then it might get messy. Mostly we don't have to care, since pointer
> alignment is sufficient in many cases, but still...
>
> Hmm. Dunno what to do here then.

IMHO let's just get rid of the ugly PTR_ALIGN(), I strongly doubt it was
added because of this xtensa "feature" :) If we ever get a bug report
about this we can then talk with the xtensa folks.

-- 
Kalle Valo

[PATCH v2] rsi: remove unecessary PTR_ALIGN()s

[Dan Carpenter]

The issue here is that we allocate "data" and then set
"data = PTR_ALIGN(data, 8);" and then we free the aligned pointer
instead of the original pointer.

kmalloc() pointers are already ARCH_SLAB_MINALIGN aligned which is 8 or
more on everything except certain Xtensa variants.  We decided that if
the Xtensa people ever notice a bug here then we'll tell them the bug is
on their side.  ;)

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Instead of saving the original pointer, just remove the ALIGN()s

diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
index d76e69c0beaa..8ef00582c6ea 100644
--- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
+++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
@@ -660,8 +660,6 @@ static int rsi_sdio_master_reg_read(struct rsi_hw *adapter, u32 addr,
 	if (!data)
 		return -ENOMEM;
 
-	data = PTR_ALIGN(data, 8);
-
 	ms_addr = (addr >> 16);
 	status = rsi_sdio_master_access_msword(adapter, ms_addr);
 	if (status < 0) {
@@ -724,8 +722,6 @@ static int rsi_sdio_master_reg_write(struct rsi_hw *adapter,
 	if (!data_aligned)
 		return -ENOMEM;
 
-	data_aligned = PTR_ALIGN(data_aligned, 8);
-
 	if (size == 2) {
 		*data_aligned = ((data << 16) | (data & 0xFFFF));
 	} else if (size == 1) {

Re: [PATCH v2] rsi: remove unecessary PTR_ALIGN()s

[Arend van Spriel]

On 4/6/2018 10:37 AM, Dan Carpenter wrote:
> The issue here is that we allocate "data" and then set
> "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer
> instead of the original pointer.
>
> kmalloc() pointers are already ARCH_SLAB_MINALIGN aligned which is 8 or
> more on everything except certain Xtensa variants.  We decided that if
> the Xtensa people ever notice a bug here then we'll tell them the bug is
> on their side.  ;)

I am not sure if it was decided to be a xtensa bug, but just to ignore 
the issue until it would arise. Anyway, not sure if the last sentence is 
useful in the commit message.

Regards,
Arend

Re: [PATCH v2] rsi: remove unecessary PTR_ALIGN()s

[Kalle Valo]

Arend van Spriel <arend.vanspriel@broadcom.com> writes:

> On 4/6/2018 10:37 AM, Dan Carpenter wrote:
>> The issue here is that we allocate "data" and then set
>> "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer
>> instead of the original pointer.
>>
>> kmalloc() pointers are already ARCH_SLAB_MINALIGN aligned which is 8 or
>> more on everything except certain Xtensa variants.  We decided that if
>> the Xtensa people ever notice a bug here then we'll tell them the bug is
>> on their side.  ;)
>
> I am not sure if it was decided to be a xtensa bug, but just to ignore
> the issue until it would arise.

IIRC on other architectures the allocation is already aligned properly
so there shouldn't be any functional changes with this patch, expect on
xtensa of course.

> Anyway, not sure if the last sentence is useful in the commit message.

IMHO it is useful as it gives a summary of our discussion, just with a
humorous tone :)

-- 
Kalle Valo

Re: [v2] rsi: remove unecessary PTR_ALIGN()s

[Kalle Valo]

Dan Carpenter <dan.carpenter@oracle.com> wrote:

> The issue here is that we allocate "data" and then set
> "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer
> instead of the original pointer.
> 
> kmalloc() pointers are already ARCH_SLAB_MINALIGN aligned which is 8 or
> more on everything except certain Xtensa variants.  We decided that if
> the Xtensa people ever notice a bug here then we'll tell them the bug is
> on their side.  ;)
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Patch applied to wireless-drivers-next.git, thanks.

350fcdb83457 rsi: remove unecessary PTR_ALIGN()s

-- 
https://patchwork.kernel.org/patch/10325781/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches