Message ID | 4B637F85.8080809@openwrt.org |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
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
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
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
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
--- 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;
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