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

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

[Felix Fietkau]

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;

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

[Eric Dumazet]

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

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

[David Daney]

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

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

[Felix Fietkau]

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

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

[David Miller]

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

[PATCH v3] skbuff: align sk_buff::cb to 64 bit and close some potential holes

[Felix Fietkau]

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.

This patch also closes some potential pre-existing holes in the sk_buff
struct surrounding the cb[] area.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
Cc: stable@kernel.org
---
sorry that it took so long for me to post this, i completely forgot
about it, as I had other things to take care of ;)
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -315,22 +315,23 @@ struct sk_buff {
 	struct sk_buff		*next;
 	struct sk_buff		*prev;
 
-	struct sock		*sk;
 	ktime_t			tstamp;
+
+	struct sock		*sk;
 	struct net_device	*dev;
 
-	unsigned long		_skb_dst;
-#ifdef CONFIG_XFRM
-	struct	sec_path	*sp;
-#endif
 	/*
 	 * This is the control buffer. It is free to use for every
 	 * layer. Please put your private variables there. If you
 	 * 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 long		_skb_dst;
+#ifdef CONFIG_XFRM
+	struct	sec_path	*sp;
+#endif
 	unsigned int		len,
 				data_len;
 	__u16			mac_len,

Re: [PATCH v3] skbuff: align sk_buff::cb to 64 bit and close some potential holes

[David Miller]

From: Felix Fietkau <nbd@openwrt.org>
Date: Tue, 23 Feb 2010 22:45:51 +0100

> 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.
> 
> This patch also closes some potential pre-existing holes in the sk_buff
> struct surrounding the cb[] area.
> 
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> Cc: stable@kernel.org

Applied, thanks for following up on this Felix.