skbuff: align sk_buff::cb to 64 bit

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 <[email protected]>

Cc [email protected]

---

Quoted 24 lines · 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 <[email protected]>

Cc [email protected]

---

include/linux/skbuff.h +1 -1

@@ -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;
 

Quoted 1 line

--

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

Quoted 25 lines · Eric Dumazet

Quoted 24 lines · 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 <[email protected]>

Cc [email protected]

---

include/linux/skbuff.h +1 -1

@@ -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;
 

Quoted 8 lines

Quoted 1 line

--

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?

Quoted 2 lines

Then perhaps add __aligned(8) to the entire structure instead of just this field.

Makes sense, I'll send a patch for that.

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 <[email protected]>

Cc [email protected]