[PATCHv2 RFC net-next] net/vxlan: Fix kernel unaligned access in __vxlan_find_mac

[PATCHv2 RFC net-next] net/vxlan: Fix kernel unaligned access in __vxlan_find_mac

[Sowmini Varadhan]

__vxlan_find_mac invokes ether_addr_equal on the eth_addr field,
which triggers unaligned access messages, so rearrange vxlan_fdb
to avoid this in the most non-intrusive way.

Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
---
v2: Alexander Duyck comments: make eth_addr[] 64b aligned.

 drivers/net/vxlan.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 34c519e..ec86a11 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -106,9 +106,9 @@ struct vxlan_fdb {
 	unsigned long	  updated;	/* jiffies */
 	unsigned long	  used;
 	struct list_head  remotes;
+	u8		  eth_addr[ETH_ALEN];
 	u16		  state;	/* see ndm_state */
 	u8		  flags;	/* see ndm_flags */
-	u8		  eth_addr[ETH_ALEN];
 };
 
 /* Pseudo network device */
-- 
1.7.1

Re: [PATCHv2 RFC net-next] net/vxlan: Fix kernel unaligned access in __vxlan_find_mac

[Joe Perches]

On Fri, 2015-07-17 at 22:00 +0200, Sowmini Varadhan wrote:
> __vxlan_find_mac invokes ether_addr_equal on the eth_addr field,
> which triggers unaligned access messages, so rearrange vxlan_fdb
> to avoid this in the most non-intrusive way.

What arch does this?

Re: [PATCHv2 RFC net-next] net/vxlan: Fix kernel unaligned access in __vxlan_find_mac

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Fri, 17 Jul 2015 16:07:02 -0700

> On Fri, 2015-07-17 at 22:00 +0200, Sowmini Varadhan wrote:
>> __vxlan_find_mac invokes ether_addr_equal on the eth_addr field,
>> which triggers unaligned access messages, so rearrange vxlan_fdb
>> to avoid this in the most non-intrusive way.
> 
> What arch does this?

Sparc, MIPS, etc.

Re: [PATCHv2 RFC net-next] net/vxlan: Fix kernel unaligned access in __vxlan_find_mac

[Sowmini Varadhan]

On (07/17/15 16:07), Joe Perches wrote:
> On Fri, 2015-07-17 at 22:00 +0200, Sowmini Varadhan wrote:
> > __vxlan_find_mac invokes ether_addr_equal on the eth_addr field,
> > which triggers unaligned access messages, so rearrange vxlan_fdb
> > to avoid this in the most non-intrusive way.
> 
> What arch does this?

sparc.

BTW, I was also getting a lot of alignment errors from 
vxlan_xmit_skb (vxh comes out unaligned) for the IPv6 path.
I did not have time to investigate/fix this correctly- not sure
if this is specific to v6.

--Sowmini

Re: [PATCHv2 RFC net-next] net/vxlan: Fix kernel unaligned access in __vxlan_find_mac

[Joe Perches]

On Fri, 2015-07-17 at 18:18 -0700, David Miller wrote:
> From: Joe Perches <joe@perches.com> Date: Fri, 17 Jul 2015 16:07:02 -0700
> > On Fri, 2015-07-17 at 22:00 +0200, Sowmini Varadhan wrote:
> >> __vxlan_find_mac invokes ether_addr_equal on the eth_addr field,
> >> which triggers unaligned access messages, so rearrange vxlan_fdb
> >> to avoid this in the most non-intrusive way.

> > What arch does this?

> Sparc, MIPS, etc.

It seems that this code has had unaligned accesses
on this field even before compare_ether_addr was
converted to ether_addr_equal.

Is sparc64 the only one that emits / ratelimits that
unaligned access message?  I looked a little, but I
didn't find a fixup message when MIPS does unaligned
accesses.  Are all the other arches silent when
fixing up unaligned accesses?  Maye adding a generic
debug only ratelimited message might help remove
more of these.  As it's not fatal, naybe the sparc64
message should be KERN_DEBUG/pr_debug.

---

 arch/sparc/kernel/unaligned_64.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/sparc/kernel/unaligned_64.c b/arch/sparc/kernel/unaligned_64.c
index 62098a8..6b7aeb7 100644
--- a/arch/sparc/kernel/unaligned_64.c
+++ b/arch/sparc/kernel/unaligned_64.c
@@ -294,13 +294,9 @@ static void kernel_mna_trap_fault(int fixup_tstate_asi)
 
 static void log_unaligned(struct pt_regs *regs)
 {
-	static DEFINE_RATELIMIT_STATE(ratelimit, 5 * HZ, 5);
+	pr_debug_ratelimited("Kernel unaligned access at TPC[%lx] %pS\n",
+			     regs->tpc, (void *)regs->tpc);
 
-	if (__ratelimit(&ratelimit)) {
-		printk("Kernel unaligned access at TPC[%lx] %pS\n",
-		       regs->tpc, (void *) regs->tpc);
-	}
-}
 
 asmlinkage void kernel_unaligned_trap(struct pt_regs *regs, unsigned int insn)
 {

Re: [PATCHv2 RFC net-next] net/vxlan: Fix kernel unaligned access in __vxlan_find_mac

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Sat, 18 Jul 2015 11:06:26 -0700

> As it's not fatal, naybe the sparc64 message should be
> KERN_DEBUG/pr_debug.

It's not fatal insofar as it doesn't crash the system.

But performance wise is _IS_ fatal, and I want the kernel to bark
at the user so that they report the issue or fix it sooner rather
than later.

Re: [PATCHv2 RFC net-next] net/vxlan: Fix kernel unaligned access in __vxlan_find_mac

[Sowmini Varadhan]

On 07/18/2015 08:06 PM, Joe Perches wrote:

> It seems that this code has had unaligned accesses
> on this field even before compare_ether_addr was
> converted to ether_addr_equal.
>
> Is sparc64 the only one that emits / ratelimits that
> unaligned access message?  I looked a little, but I
> didn't find a fixup message when MIPS does unaligned
> accesses.  Are all the other arches silent when
> fixing up unaligned accesses?  Maye adding a generic
> debug only ratelimited message might help remove
> more of these.  As it's not fatal, naybe the sparc64
> message should be KERN_DEBUG/pr_debug.

I'm confused, are we suggesting that we "fix" the unaligned
access by snuffing out the message that complains loudly and correctly
about it?

See also: large block comment above __pksb_trim
about correctly using skb_reserve(). Evidently not being
correctly done for the IPv6-vxlan code path (and possibly
for other encaps too?)

--Sowmini

Re: [PATCHv2 RFC net-next] net/vxlan: Fix kernel unaligned access in __vxlan_find_mac

[David Miller]

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Sun, 19 Jul 2015 14:01:34 +0200

> On 07/18/2015 08:06 PM, Joe Perches wrote:
> 
>> It seems that this code has had unaligned accesses
>> on this field even before compare_ether_addr was
>> converted to ether_addr_equal.
>>
>> Is sparc64 the only one that emits / ratelimits that
>> unaligned access message?  I looked a little, but I
>> didn't find a fixup message when MIPS does unaligned
>> accesses.  Are all the other arches silent when
>> fixing up unaligned accesses?  Maye adding a generic
>> debug only ratelimited message might help remove
>> more of these.  As it's not fatal, naybe the sparc64
>> message should be KERN_DEBUG/pr_debug.
> 
> I'm confused, are we suggesting that we "fix" the unaligned
> access by snuffing out the message that complains loudly and correctly
> about it?
> 
> See also: large block comment above __pksb_trim
> about correctly using skb_reserve(). Evidently not being
> correctly done for the IPv6-vxlan code path (and possibly
> for other encaps too?)

We should fix the unaligned accesses, rather than quiet the
warning.

Re: [PATCHv2 RFC net-next] net/vxlan: Fix kernel unaligned access in __vxlan_find_mac

[Joe Perches]

On Sun, 2015-07-19 at 11:38 -0700, David Miller wrote:
> From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> Date: Sun, 19 Jul 2015 14:01:34 +0200
> 
> > On 07/18/2015 08:06 PM, Joe Perches wrote:
> > 
> >> It seems that this code has had unaligned accesses
> >> on this field even before compare_ether_addr was
> >> converted to ether_addr_equal.
> >>
> >> Is sparc64 the only one that emits / ratelimits that
> >> unaligned access message?  I looked a little, but I
> >> didn't find a fixup message when MIPS does unaligned
> >> accesses.  Are all the other arches silent when
> >> fixing up unaligned accesses?  Maye adding a generic
> >> debug only ratelimited message might help remove
> >> more of these.  As it's not fatal, naybe the sparc64
> >> message should be KERN_DEBUG/pr_debug.
> > 
> > I'm confused, are we suggesting that we "fix" the unaligned
> > access by snuffing out the message that complains loudly and correctly
> > about it?
> > 
> > See also: large block comment above __pksb_trim
> > about correctly using skb_reserve(). Evidently not being
> > correctly done for the IPv6-vxlan code path (and possibly
> > for other encaps too?)
> 
> We should fix the unaligned accesses, rather than quiet the
> warning.

Definitely so, the question I have is whether or not the the
message should be able to be silenced, not just ratelimited.

I think there are likely to be occasions when, given arbitrary
protocol stacking, unaligned accesses are unavoidable.

Perhaps batman might have this as an actual issue.