| Message ID | 4B635CA1.8070803@openwrt.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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. -- 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
--- 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;
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