rsi: Free the unaligned pointer

PATCH In review Dan Carpenter 4 participants 10 messages Apr 5, 2018 – Apr 24, 2018

Subsystems (2)

Kernel janitors Redpine wireless driver

lore

Share & tools

lore

Dan Carpenter

author

2018-04-05 11:23 UTC #0 lore raw

PATCH 1/1 rsi: Free the unaligned pointer

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 <[email protected]>

drivers/net/wireless/rsi/rsi_91x_sdio.c +7 -7

index d76e69c0beaa..ca4e698ab69b 100644
@@ -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;
 }

Johannes Berg

2018-04-05 11:30 UTC #1 lore raw

Quoted 3 lines · 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.

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

johannes

Dan Carpenter

author

2018-04-05 11:39 UTC #2 lore raw

Quoted 7 lines · Johannes Berg

Quoted 3 lines · 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.
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

Dan Carpenter

author

2018-04-05 11:41 UTC #3 lore raw

Quoted 6 lines · Dan Carpenter

Quoted 5 lines · Johannes Berg
Quoted 3 lines · 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.

Quoted 7 lines

Quoted 2 lines
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

Johannes Berg

2018-04-05 11:46 UTC #4 lore raw

Quoted 15 lines · Dan Carpenter

Quoted 6 lines · Dan Carpenter
Quoted 5 lines · Johannes Berg
Quoted 3 lines · 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.
Quoted 7 lines
Quoted 2 lines
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.

Quoted 4 lines

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

Kalle Valo

2018-04-05 12:23 UTC #5 lore raw

Quoted 26 lines · Johannes Berg

Quoted 15 lines · Dan Carpenter
Quoted 6 lines · Dan Carpenter
Quoted 5 lines · Johannes Berg
Quoted 3 lines · 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.
Quoted 7 lines
Quoted 2 lines
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.
Quoted 5 lines
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...

Quoted 5 lines

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.

Signature · 1 line

Kalle Valo

Dan Carpenter

author

2018-04-06 08:37 UTC #6 lore raw

PATCH v2 1/1 rsi: remove unecessary PTR_ALIGN()s

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 <[email protected]>

---
v2: Instead of saving the original pointer, just remove the ALIGN()s

drivers/net/wireless/rsi/rsi_91x_sdio.c +0 -4

index d76e69c0beaa..8ef00582c6ea 100644
@@ -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) {

Arend van Spriel

2018-04-06 08:45 UTC #7 lore raw

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

Quoted 8 lines · 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. ;)

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

Kalle Valo

2018-04-06 09:01 UTC #8 lore raw

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

Quoted 5 lines · Arend van Spriel

Quoted 4 lines · 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.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 1 line

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 :)

Signature · 1 line

Kalle Valo

Kalle Valo

2018-04-24 17:24 UTC #9 lore raw

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

In reply to Dan Carpenter

Quoted 10 lines · 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 <[email protected]>

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

350fcdb83457 rsi: remove unecessary PTR_ALIGN()s

Signature · 3 lines

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

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