* [PATCH v2] skbuff: align sk_buff::cb to 64 bit
@ 2010-01-30 0:38 Felix Fietkau
2010-01-30 7:07 ` Eric Dumazet
0 siblings, 1 reply; 7+ messages in thread
From: Felix Fietkau @ 2010-01-30 0:38 UTC (permalink / raw)
To: netdev; +Cc: Lennert Buytenhek, David Daney
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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] skbuff: align sk_buff::cb to 64 bit
2010-01-30 0:38 [PATCH v2] skbuff: align sk_buff::cb to 64 bit Felix Fietkau
@ 2010-01-30 7:07 ` Eric Dumazet
2010-02-01 18:26 ` David Daney
0 siblings, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2010-01-30 7:07 UTC (permalink / raw)
To: Felix Fietkau; +Cc: netdev, Lennert Buytenhek, David Daney
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] skbuff: align sk_buff::cb to 64 bit
2010-01-30 7:07 ` Eric Dumazet
@ 2010-02-01 18:26 ` David Daney
2010-02-01 18:37 ` Felix Fietkau
0 siblings, 1 reply; 7+ messages in thread
From: David Daney @ 2010-02-01 18:26 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Felix Fietkau, netdev, Lennert Buytenhek
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] skbuff: align sk_buff::cb to 64 bit
2010-02-01 18:26 ` David Daney
@ 2010-02-01 18:37 ` Felix Fietkau
2010-02-12 20:13 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: Felix Fietkau @ 2010-02-01 18:37 UTC (permalink / raw)
To: David Daney; +Cc: Eric Dumazet, netdev, Lennert Buytenhek
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] skbuff: align sk_buff::cb to 64 bit
2010-02-01 18:37 ` Felix Fietkau
@ 2010-02-12 20:13 ` David Miller
2010-02-23 21:45 ` [PATCH v3] skbuff: align sk_buff::cb to 64 bit and close some potential holes Felix Fietkau
0 siblings, 1 reply; 7+ messages in thread
From: David Miller @ 2010-02-12 20:13 UTC (permalink / raw)
To: nbd; +Cc: ddaney, eric.dumazet, netdev, buytenh
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? :-)
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3] skbuff: align sk_buff::cb to 64 bit and close some potential holes
2010-02-12 20:13 ` David Miller
@ 2010-02-23 21:45 ` Felix Fietkau
2010-02-27 11:17 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: Felix Fietkau @ 2010-02-23 21:45 UTC (permalink / raw)
To: David Miller; +Cc: ddaney, eric.dumazet, netdev, buytenh
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,
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] skbuff: align sk_buff::cb to 64 bit and close some potential holes
2010-02-23 21:45 ` [PATCH v3] skbuff: align sk_buff::cb to 64 bit and close some potential holes Felix Fietkau
@ 2010-02-27 11:17 ` David Miller
0 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2010-02-27 11:17 UTC (permalink / raw)
To: nbd; +Cc: ddaney, eric.dumazet, netdev, buytenh
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-02-27 11:17 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-01-30 0:38 [PATCH v2] skbuff: align sk_buff::cb to 64 bit Felix Fietkau
2010-01-30 7:07 ` Eric Dumazet
2010-02-01 18:26 ` David Daney
2010-02-01 18:37 ` Felix Fietkau
2010-02-12 20:13 ` David Miller
2010-02-23 21:45 ` [PATCH v3] skbuff: align sk_buff::cb to 64 bit and close some potential holes Felix Fietkau
2010-02-27 11:17 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.