* [PATCH] skbuff: align sk_buff::cb to 64 bit
@ 2010-01-29 22:09 Felix Fietkau
2010-01-29 22:42 ` David Daney
2010-01-30 1:41 ` Lennert Buytenhek
0 siblings, 2 replies; 6+ messages in thread
From: Felix Fietkau @ 2010-01-29 22:09 UTC (permalink / raw)
To: netdev; +Cc: Lennert Buytenhek
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] __attribute__((aligned(8)));
unsigned int len,
data_len;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] skbuff: align sk_buff::cb to 64 bit
2010-01-29 22:09 [PATCH] skbuff: align sk_buff::cb to 64 bit Felix Fietkau
@ 2010-01-29 22:42 ` David Daney
2010-01-29 22:54 ` Edgar E. Iglesias
2010-01-29 23:09 ` Felix Fietkau
2010-01-30 1:41 ` Lennert Buytenhek
1 sibling, 2 replies; 6+ messages in thread
From: David Daney @ 2010-01-29 22:42 UTC (permalink / raw)
To: Felix Fietkau; +Cc: netdev, Lennert Buytenhek
Felix Fietkau wrote:
> 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] __attribute__((aligned(8)));
>
s/__attribute__((aligned(8)))/__aligned(8)/
Could the same thing be achieved by swapping the order of the dev and
tstamp fields instead of adding the alignment attribute?
What is the sizeof(void *) on this thing?
David Daney
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] skbuff: align sk_buff::cb to 64 bit
2010-01-29 22:42 ` David Daney
@ 2010-01-29 22:54 ` Edgar E. Iglesias
2010-01-29 23:14 ` Felix Fietkau
2010-01-29 23:09 ` Felix Fietkau
1 sibling, 1 reply; 6+ messages in thread
From: Edgar E. Iglesias @ 2010-01-29 22:54 UTC (permalink / raw)
To: David Daney; +Cc: Felix Fietkau, netdev, Lennert Buytenhek
On Fri, Jan 29, 2010 at 02:42:03PM -0800, David Daney wrote:
> Felix Fietkau wrote:
> > 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] __attribute__((aligned(8)));
> >
>
>
> s/__attribute__((aligned(8)))/__aligned(8)/
>
> Could the same thing be achieved by swapping the order of the dev and
> tstamp fields instead of adding the alignment attribute?
>
> What is the sizeof(void *) on this thing?
I'd guess the problem is with the accessor to the thing.
something in the opaque cb ptr its beeing accessed wroingly causing
overwrites.
Cheers,
ed
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] skbuff: align sk_buff::cb to 64 bit
2010-01-29 22:42 ` David Daney
2010-01-29 22:54 ` Edgar E. Iglesias
@ 2010-01-29 23:09 ` Felix Fietkau
1 sibling, 0 replies; 6+ messages in thread
From: Felix Fietkau @ 2010-01-29 23:09 UTC (permalink / raw)
To: David Daney; +Cc: netdev, Lennert Buytenhek
On 2010-01-29 11:42 PM, David Daney wrote:
> Felix Fietkau wrote:
>> 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] __attribute__((aligned(8)));
>>
>
>
> s/__attribute__((aligned(8)))/__aligned(8)/
OK. Will send a v2.
> Could the same thing be achieved by swapping the order of the dev and
> tstamp fields instead of adding the alignment attribute?
>
> What is the sizeof(void *) on this thing?
sizeof(void *) == 4
This is a 32 bit arch, which happens to have 64-bit load/store ops,
which the compiler emits where appropriate.
Reordering the dev and tstamp does not help, as the cb gets moved by 4
bytes, depending on whether CONFIG_XFRM is set or unset.
As far as I know, ARM EABI requires an 8 byte alignment anyway, so I
think it's better to make this explicit, since cb[] is meant to be used
as opaque storage for other data structures.
- Felix
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] skbuff: align sk_buff::cb to 64 bit
2010-01-29 22:54 ` Edgar E. Iglesias
@ 2010-01-29 23:14 ` Felix Fietkau
0 siblings, 0 replies; 6+ messages in thread
From: Felix Fietkau @ 2010-01-29 23:14 UTC (permalink / raw)
To: Edgar E. Iglesias; +Cc: David Daney, netdev, Lennert Buytenhek
On 2010-01-29 11:54 PM, Edgar E. Iglesias wrote:
> On Fri, Jan 29, 2010 at 02:42:03PM -0800, David Daney wrote:
>> Felix Fietkau wrote:
>> > 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] __attribute__((aligned(8)));
>> >
> I'd guess the problem is with the accessor to the thing.
> something in the opaque cb ptr its beeing accessed wroingly causing
> overwrites.
Nope, the problem is not with the accessor, I verified that. The start
address of the datastructure that gets put into skb->cb correctly points
at the start of the cb, and it is only aligned to 4 byte, not 8. Also,
this issue only happens on CPUs that do not throw an unaligned exception
for this instruction.
- Felix
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] skbuff: align sk_buff::cb to 64 bit
2010-01-29 22:09 [PATCH] skbuff: align sk_buff::cb to 64 bit Felix Fietkau
2010-01-29 22:42 ` David Daney
@ 2010-01-30 1:41 ` Lennert Buytenhek
1 sibling, 0 replies; 6+ messages in thread
From: Lennert Buytenhek @ 2010-01-30 1:41 UTC (permalink / raw)
To: Felix Fietkau; +Cc: netdev
On Fri, Jan 29, 2010 at 11:09:37PM +0100, Felix Fietkau wrote:
> 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.
Actually, only older Marvell CPU cores have this behavior.
The ARMv5 Architecture Reference Manual permits the behavior of not
throwing alignment exceptions for doubleword (64bit) load/stores to
4mod8 aligned addresses, and early Feroceons indeed don't, but
anything recent will throw an alignment exception.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-01-30 1:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-01-29 22:09 [PATCH] skbuff: align sk_buff::cb to 64 bit Felix Fietkau
2010-01-29 22:42 ` David Daney
2010-01-29 22:54 ` Edgar E. Iglesias
2010-01-29 23:14 ` Felix Fietkau
2010-01-29 23:09 ` Felix Fietkau
2010-01-30 1:41 ` Lennert Buytenhek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).