diff mbox

[v2] skbuff: align sk_buff::cb to 64 bit

Message ID 4B637F85.8080809@openwrt.org
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Felix Fietkau Jan. 30, 2010, 12:38 a.m. UTC
The alignment requirement for 64-bit load/store instructions on ARM is
implementation defined. Some CPUs (such as Marvell Feroceon) do not
generate an exception, if such an instruction is executed with an
address that is not 64 bit aligned. In such a case, the Feroceon
corrupts adjacent memory, which showed up
in my tests as a crash in the rx path of ath9k that only occured with
CONFIG_XFRM set. This crash happened, because the first field of the
mac80211 rx status info in the cb is an u64, and changing it corrupted
the skb->sp field.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
Cc: stable@kernel.org
---
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Eric Dumazet Jan. 30, 2010, 7:07 a.m. UTC | #1
Le samedi 30 janvier 2010 à 01:38 +0100, Felix Fietkau a écrit :
> The alignment requirement for 64-bit load/store instructions on ARM is
> implementation defined. Some CPUs (such as Marvell Feroceon) do not
> generate an exception, if such an instruction is executed with an
> address that is not 64 bit aligned. In such a case, the Feroceon
> corrupts adjacent memory, which showed up
> in my tests as a crash in the rx path of ath9k that only occured with
> CONFIG_XFRM set. This crash happened, because the first field of the
> mac80211 rx status info in the cb is an u64, and changing it corrupted
> the skb->sp field.
> 
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> Cc: stable@kernel.org
> ---
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -329,7 +329,7 @@ struct sk_buff {
>  	 * want to keep them across layers you have to do a skb_clone()
>  	 * first. This is owned by whoever has the skb queued ATM.
>  	 */
> -	char			cb[48];
> +	char			cb[48] __aligned(8);
>   	unsigned int		len,
>  				data_len;
> 
> --

Without a detailed analysis of holes added on x86_32 and/or x86_64, I
guess this patch is not acceptable as is.

You certainly can find a better way to do this, without adding holes in
sk_buff structure. Size matters a lot :)

Thanks


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Daney Feb. 1, 2010, 6:26 p.m. UTC | #2
Eric Dumazet wrote:
> Le samedi 30 janvier 2010 à 01:38 +0100, Felix Fietkau a écrit :
>> The alignment requirement for 64-bit load/store instructions on ARM is
>> implementation defined. Some CPUs (such as Marvell Feroceon) do not
>> generate an exception, if such an instruction is executed with an
>> address that is not 64 bit aligned. In such a case, the Feroceon
>> corrupts adjacent memory, which showed up
>> in my tests as a crash in the rx path of ath9k that only occured with
>> CONFIG_XFRM set. This crash happened, because the first field of the
>> mac80211 rx status info in the cb is an u64, and changing it corrupted
>> the skb->sp field.
>>
>> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
>> Cc: stable@kernel.org
>> ---
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -329,7 +329,7 @@ struct sk_buff {
>>  	 * want to keep them across layers you have to do a skb_clone()
>>  	 * first. This is owned by whoever has the skb queued ATM.
>>  	 */
>> -	char			cb[48];
>> +	char			cb[48] __aligned(8);
>>   	unsigned int		len,
>>  				data_len;
>>
>> --
> 
> Without a detailed analysis of holes added on x86_32 and/or x86_64, I
> guess this patch is not acceptable as is.
> 
> You certainly can find a better way to do this, without adding holes in
> sk_buff structure. Size matters a lot :)
> 

Can't we just move cb[] up so that it comes after an even number of 
pointers under all configs?

Then perhaps add __aligned(8) to the entire structure instead of just 
this field.

Alternatively, could you fix the driver so that it adds the necessary 
alignment to its use of the cb[] array?

How common it it to have sizeof(void *) == 4 *and* require 8-byte 
alignment on other things?  cb[] is fairly large, can you afford to burn 
4 bytes for alignment purposes in your driver?


David Daney
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felix Fietkau Feb. 1, 2010, 6:37 p.m. UTC | #3
On 2010-02-01 7:26 PM, David Daney wrote:
> Eric Dumazet wrote:
>> Le samedi 30 janvier 2010 à 01:38 +0100, Felix Fietkau a écrit :
>>> The alignment requirement for 64-bit load/store instructions on ARM is
>>> implementation defined. Some CPUs (such as Marvell Feroceon) do not
>>> generate an exception, if such an instruction is executed with an
>>> address that is not 64 bit aligned. In such a case, the Feroceon
>>> corrupts adjacent memory, which showed up
>>> in my tests as a crash in the rx path of ath9k that only occured with
>>> CONFIG_XFRM set. This crash happened, because the first field of the
>>> mac80211 rx status info in the cb is an u64, and changing it corrupted
>>> the skb->sp field.
>>>
>>> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
>>> Cc: stable@kernel.org
>>> ---
>>> --- a/include/linux/skbuff.h
>>> +++ b/include/linux/skbuff.h
>>> @@ -329,7 +329,7 @@ struct sk_buff {
>>>  	 * want to keep them across layers you have to do a skb_clone()
>>>  	 * first. This is owned by whoever has the skb queued ATM.
>>>  	 */
>>> -	char			cb[48];
>>> +	char			cb[48] __aligned(8);
>>>   	unsigned int		len,
>>>  				data_len;
>>>
>>> --
>> 
>> Without a detailed analysis of holes added on x86_32 and/or x86_64, I
>> guess this patch is not acceptable as is.
>> 
>> You certainly can find a better way to do this, without adding holes in
>> sk_buff structure. Size matters a lot :)
>> 
> 
> Can't we just move cb[] up so that it comes after an even number of 
> pointers under all configs?
> 
> Then perhaps add __aligned(8) to the entire structure instead of just 
> this field.
Makes sense, I'll send a patch for that.

> Alternatively, could you fix the driver so that it adds the necessary 
> alignment to its use of the cb[] array?
> 
> How common it it to have sizeof(void *) == 4 *and* require 8-byte 
> alignment on other things?  cb[] is fairly large, can you afford to burn 
> 4 bytes for alignment purposes in your driver?
No, I can't afford to burn a single byte on this, in some places
mac80211 uses all of the cb[] area up to the last byte.

- Felix
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Feb. 12, 2010, 8:13 p.m. UTC | #4
From: Felix Fietkau <nbd@openwrt.org>
Date: Mon, 01 Feb 2010 19:37:45 +0100

> On 2010-02-01 7:26 PM, David Daney wrote:
>> Then perhaps add __aligned(8) to the entire structure instead of just 
>> this field.
> Makes sense, I'll send a patch for that.

Did that patch ever materialize? :-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -329,7 +329,7 @@  struct sk_buff {
 	 * want to keep them across layers you have to do a skb_clone()
 	 * first. This is owned by whoever has the skb queued ATM.
 	 */
-	char			cb[48];
+	char			cb[48] __aligned(8);
  	unsigned int		len,
 				data_len;