Re: [PATCH net] net: mscc: ocelot: fix all IPv6 getting trapped to CPU when PTP timestamping is used

[Simon Horman]

On Tue, Feb 07, 2023 at 08:31:17PM +0200, Vladimir Oltean wrote:
> While running this selftest which usually passes:
> 
> ~/selftests/drivers/net/dsa# ./local_termination.sh eno0 swp0
> TEST: swp0: Unicast IPv4 to primary MAC address                     [ OK ]
> TEST: swp0: Unicast IPv4 to macvlan MAC address                     [ OK ]
> TEST: swp0: Unicast IPv4 to unknown MAC address                     [ OK ]
> TEST: swp0: Unicast IPv4 to unknown MAC address, promisc            [ OK ]
> TEST: swp0: Unicast IPv4 to unknown MAC address, allmulti           [ OK ]
> TEST: swp0: Multicast IPv4 to joined group                          [ OK ]
> TEST: swp0: Multicast IPv4 to unknown group                         [ OK ]
> TEST: swp0: Multicast IPv4 to unknown group, promisc                [ OK ]
> TEST: swp0: Multicast IPv4 to unknown group, allmulti               [ OK ]
> TEST: swp0: Multicast IPv6 to joined group                          [ OK ]
> TEST: swp0: Multicast IPv6 to unknown group                         [ OK ]
> TEST: swp0: Multicast IPv6 to unknown group, promisc                [ OK ]
> TEST: swp0: Multicast IPv6 to unknown group, allmulti               [ OK ]
> 
> if I start PTP timestamping then run it again (debug prints added by me),
> the unknown IPv6 MC traffic is seen by the CPU port even when it should
> have been dropped:
> 
> ~/selftests/drivers/net/dsa# ptp4l -i swp0 -2 -P -m
> ptp4l[225.410]: selected /dev/ptp1 as PTP clock
> [  225.445746] mscc_felix 0000:00:00.5: ocelot_l2_ptp_trap_add: port 0 adding L2 PTP trap
> [  225.453815] mscc_felix 0000:00:00.5: ocelot_ipv4_ptp_trap_add: port 0 adding IPv4 PTP event trap
> [  225.462703] mscc_felix 0000:00:00.5: ocelot_ipv4_ptp_trap_add: port 0 adding IPv4 PTP general trap
> [  225.471768] mscc_felix 0000:00:00.5: ocelot_ipv6_ptp_trap_add: port 0 adding IPv6 PTP event trap
> [  225.480651] mscc_felix 0000:00:00.5: ocelot_ipv6_ptp_trap_add: port 0 adding IPv6 PTP general trap
> ptp4l[225.488]: port 1: INITIALIZING to LISTENING on INIT_COMPLETE
> ptp4l[225.488]: port 0: INITIALIZING to LISTENING on INIT_COMPLETE
> ^C
> ~/selftests/drivers/net/dsa# ./local_termination.sh eno0 swp0
> TEST: swp0: Unicast IPv4 to primary MAC address                     [ OK ]
> TEST: swp0: Unicast IPv4 to macvlan MAC address                     [ OK ]
> TEST: swp0: Unicast IPv4 to unknown MAC address                     [ OK ]
> TEST: swp0: Unicast IPv4 to unknown MAC address, promisc            [ OK ]
> TEST: swp0: Unicast IPv4 to unknown MAC address, allmulti           [ OK ]
> TEST: swp0: Multicast IPv4 to joined group                          [ OK ]
> TEST: swp0: Multicast IPv4 to unknown group                         [ OK ]
> TEST: swp0: Multicast IPv4 to unknown group, promisc                [ OK ]
> TEST: swp0: Multicast IPv4 to unknown group, allmulti               [ OK ]
> TEST: swp0: Multicast IPv6 to joined group                          [ OK ]
> TEST: swp0: Multicast IPv6 to unknown group                         [FAIL]
>         reception succeeded, but should have failed
> TEST: swp0: Multicast IPv6 to unknown group, promisc                [ OK ]
> TEST: swp0: Multicast IPv6 to unknown group, allmulti               [ OK ]
> 
> The PGID_MCIPV6 is configured correctly to not flood to the CPU,
> I checked that.
> 
> Furthermore, when I disable back PTP RX timestamping (ptp4l doesn't do
> that when it exists), packets are RX filtered again as they should be:
> 
> ~/selftests/drivers/net/dsa# hwstamp_ctl -i swp0 -r 0
> [  218.202854] mscc_felix 0000:00:00.5: ocelot_l2_ptp_trap_del: port 0 removing L2 PTP trap
> [  218.212656] mscc_felix 0000:00:00.5: ocelot_ipv4_ptp_trap_del: port 0 removing IPv4 PTP event trap
> [  218.222975] mscc_felix 0000:00:00.5: ocelot_ipv4_ptp_trap_del: port 0 removing IPv4 PTP general trap
> [  218.233133] mscc_felix 0000:00:00.5: ocelot_ipv6_ptp_trap_del: port 0 removing IPv6 PTP event trap
> [  218.242251] mscc_felix 0000:00:00.5: ocelot_ipv6_ptp_trap_del: port 0 removing IPv6 PTP general trap
> current settings:
> tx_type 1
> rx_filter 12
> new settings:
> tx_type 1
> rx_filter 0
> ~/selftests/drivers/net/dsa# ./local_termination.sh eno0 swp0
> TEST: swp0: Unicast IPv4 to primary MAC address                     [ OK ]
> TEST: swp0: Unicast IPv4 to macvlan MAC address                     [ OK ]
> TEST: swp0: Unicast IPv4 to unknown MAC address                     [ OK ]
> TEST: swp0: Unicast IPv4 to unknown MAC address, promisc            [ OK ]
> TEST: swp0: Unicast IPv4 to unknown MAC address, allmulti           [ OK ]
> TEST: swp0: Multicast IPv4 to joined group                          [ OK ]
> TEST: swp0: Multicast IPv4 to unknown group                         [ OK ]
> TEST: swp0: Multicast IPv4 to unknown group, promisc                [ OK ]
> TEST: swp0: Multicast IPv4 to unknown group, allmulti               [ OK ]
> TEST: swp0: Multicast IPv6 to joined group                          [ OK ]
> TEST: swp0: Multicast IPv6 to unknown group                         [ OK ]
> TEST: swp0: Multicast IPv6 to unknown group, promisc                [ OK ]
> TEST: swp0: Multicast IPv6 to unknown group, allmulti               [ OK ]
> 
> So it's clear that something in the PTP RX trapping logic went wrong.
> 
> Looking a bit at the code, I can see that there are 4 typos, which
> populate "ipv4" VCAP IS2 key filter fields for IPv6 keys.
> 
> VCAP IS2 keys of type OCELOT_VCAP_KEY_IPV4 and OCELOT_VCAP_KEY_IPV6 are
> handled by is2_entry_set(). OCELOT_VCAP_KEY_IPV4 looks at
> &filter->key.ipv4, and OCELOT_VCAP_KEY_IPV6 at &filter->key.ipv6.
> Simply put, when we populate the wrong key field, &filter->key.ipv6
> fields "proto.mask" and "proto.value" remain all zeroes (or "don't care").
> So is2_entry_set() will enter the "else" of this "if" condition:
> 
> 	if (msk == 0xff && (val == IPPROTO_TCP || val == IPPROTO_UDP))
> 
> and proceed to ignore the "proto" field. The resulting rule will match
> on all IPv6 traffic, trapping it to the CPU.
> 
> This is the reason why the local_termination.sh selftest sees it,
> because control traps are stronger than the PGID_MCIPV6 used for
> flooding (from the forwarding data path).
> 
> But the problem is in fact much deeper. We trap all IPv6 traffic to the
> CPU, but if we're bridged, we set skb->offload_fwd_mark = 1, so software
> forwarding will not take place and IPv6 traffic will never reach its
> destination.
> 
> The fix is simple - correct the typos.
> 
> I was intentionally inaccurate in the commit message about the breakage
> occurring when any PTP timestamping is enabled. In fact it only happens
> when L4 timestamping is requested (HWTSTAMP_FILTER_PTP_V2_EVENT or
> HWTSTAMP_FILTER_PTP_V2_L4_EVENT). But ptp4l requests a larger RX
> timestamping filter than it needs for "-2": HWTSTAMP_FILTER_PTP_V2_EVENT.
> I wanted people skimming through git logs to not think that the bug
> doesn't affect them because they only use ptp4l in L2 mode.
> 
> Fixes: 96ca08c05838 ("net: mscc: ocelot: set up traps for PTP packets")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>

Reviewed-by: Simon Horman <simon.horman@xxxxxxxxxxxx>