[net-next,01/18] net: skb_scrub_packet(): Scrub offload_fwd_mark
diff mbox series

Message ID 20181119161006.5405-2-idosch@mellanox.com
State Accepted
Commit b5dd186d10ba59e6b5ba60e42b3b083df56df6f3
Headers show
Series
  • selftests: Add tests for VXLAN at an 802.1d bridge
Related show

Commit Message

Ido Schimmel Nov. 19, 2018, 4:11 p.m. UTC
From: Petr Machata <petrm@mellanox.com>

When a packet is trapped and the corresponding SKB marked as
already-forwarded, it retains this marking even after it is forwarded
across veth links into another bridge. There, since it ingresses the
bridge over veth, which doesn't have offload_fwd_mark, it triggers a
warning in nbp_switchdev_frame_mark().

Then nbp_switchdev_allowed_egress() decides not to allow egress from
this bridge through another veth, because the SKB is already marked, and
the mark (of 0) of course matches. Thus the packet is incorrectly
blocked.

Solve by resetting offload_fwd_mark() in skb_scrub_packet(). That
function is called from tunnels and also from veth, and thus catches the
cases where traffic is forwarded between bridges and transformed in a
way that invalidates the marking.

Signed-off-by: Petr Machata <petrm@mellanox.com>
Suggested-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 net/core/skbuff.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

David Miller Nov. 19, 2018, 7:07 p.m. UTC | #1
From: Ido Schimmel <idosch@mellanox.com>
Date: Mon, 19 Nov 2018 16:11:07 +0000

> From: Petr Machata <petrm@mellanox.com>
> 
> When a packet is trapped and the corresponding SKB marked as
> already-forwarded, it retains this marking even after it is forwarded
> across veth links into another bridge. There, since it ingresses the
> bridge over veth, which doesn't have offload_fwd_mark, it triggers a
> warning in nbp_switchdev_frame_mark().
> 
> Then nbp_switchdev_allowed_egress() decides not to allow egress from
> this bridge through another veth, because the SKB is already marked, and
> the mark (of 0) of course matches. Thus the packet is incorrectly
> blocked.
> 
> Solve by resetting offload_fwd_mark() in skb_scrub_packet(). That
> function is called from tunnels and also from veth, and thus catches the
> cases where traffic is forwarded between bridges and transformed in a
> way that invalidates the marking.
> 
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> Suggested-by: Ido Schimmel <idosch@mellanox.com>
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>

As a bug fix this seems relevant for 'net' instead of 'net-next'.
Ido Schimmel Nov. 19, 2018, 8:06 p.m. UTC | #2
On Mon, Nov 19, 2018 at 11:07:45AM -0800, David Miller wrote:
> From: Ido Schimmel <idosch@mellanox.com>
> Date: Mon, 19 Nov 2018 16:11:07 +0000
> 
> > From: Petr Machata <petrm@mellanox.com>
> > 
> > When a packet is trapped and the corresponding SKB marked as
> > already-forwarded, it retains this marking even after it is forwarded
> > across veth links into another bridge. There, since it ingresses the
> > bridge over veth, which doesn't have offload_fwd_mark, it triggers a
> > warning in nbp_switchdev_frame_mark().
> > 
> > Then nbp_switchdev_allowed_egress() decides not to allow egress from
> > this bridge through another veth, because the SKB is already marked, and
> > the mark (of 0) of course matches. Thus the packet is incorrectly
> > blocked.
> > 
> > Solve by resetting offload_fwd_mark() in skb_scrub_packet(). That
> > function is called from tunnels and also from veth, and thus catches the
> > cases where traffic is forwarded between bridges and transformed in a
> > way that invalidates the marking.
> > 
> > Signed-off-by: Petr Machata <petrm@mellanox.com>
> > Suggested-by: Ido Schimmel <idosch@mellanox.com>
> > Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> 
> As a bug fix this seems relevant for 'net' instead of 'net-next'.

This seemed really obscure/specific to this selftest so I deemed it
unnecessary for 'net'.

Agree that by the book it is 'net' material, so we'll send it there.

Thanks for the feedback
Petr Machata Nov. 19, 2018, 10:41 p.m. UTC | #3
David Miller <davem@davemloft.net> writes:

> From: Ido Schimmel <idosch@mellanox.com>
> Date: Mon, 19 Nov 2018 16:11:07 +0000
>
>> From: Petr Machata <petrm@mellanox.com>
>> 
>> When a packet is trapped and the corresponding SKB marked as
>> already-forwarded, it retains this marking even after it is forwarded
>> across veth links into another bridge. There, since it ingresses the
>> bridge over veth, which doesn't have offload_fwd_mark, it triggers a
>> warning in nbp_switchdev_frame_mark().
>> 
>> Then nbp_switchdev_allowed_egress() decides not to allow egress from
>> this bridge through another veth, because the SKB is already marked, and
>> the mark (of 0) of course matches. Thus the packet is incorrectly
>> blocked.
>> 
>> Solve by resetting offload_fwd_mark() in skb_scrub_packet(). That
>> function is called from tunnels and also from veth, and thus catches the
>> cases where traffic is forwarded between bridges and transformed in a
>> way that invalidates the marking.
>> 
>> Signed-off-by: Petr Machata <petrm@mellanox.com>
>> Suggested-by: Ido Schimmel <idosch@mellanox.com>
>> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
>
> As a bug fix this seems relevant for 'net' instead of 'net-next'.

Sure, I'll send for net.

Thanks,
Petr

Patch
diff mbox series

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index a1be7f19d998..9a8a72cefe9b 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4882,6 +4882,11 @@  void skb_scrub_packet(struct sk_buff *skb, bool xnet)
 	nf_reset(skb);
 	nf_reset_trace(skb);
 
+#ifdef CONFIG_NET_SWITCHDEV
+	skb->offload_fwd_mark = 0;
+	skb->offload_mr_fwd_mark = 0;
+#endif
+
 	if (!xnet)
 		return;