Re: [PATCH net-next 02/13] amt: send the Relay Advertisement over IPv6

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] amt_send_ctrl_v6() in drivers/net/amt.c never counts a transmit
  failure after the route lookup.
- [Low] amt_route6() builds the flowi6 for ip6_dst_lookup_flow() with
  memset() and never sets fl6.flowlabel.
- [Low] The new IPv6 branch of amt_discovery_handler() rejects a
  Discovery whose outer destination is multicast, so that it does not…

--

amt: send the Relay Advertisement over IPv6

This adds the 24-byte IPv6 form of the Relay Advertisement. It also adds a
new amt_send_ctrl_v6() helper, which routes with ip6_dst_lookup_flow() via
amt_route6() and sends with udp_tunnel6_xmit_skb(). On an IPv6 device,
amt_discovery_handler() answers a Discovery from the Discovery's destination
address, and drops a Discovery whose destination is multicast.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 423ed7745fc7d..a550f84e86344 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -609,6 +609,78 @@ static void amt_update_relay_status(struct amt_tunnel_list *tunnel,
>  	spin_unlock_bh(&tunnel->lock);
>  }
>  
> +static struct dst_entry *amt_route6(struct amt_dev *amt, struct sock *sk,
> +				    const struct in6_addr *saddr,
> +				    const struct in6_addr *daddr,
> +				    __be16 sport, __be16 dport)
> +{
> +	struct flowi6 fl6;
> +
> +	memset(&fl6, 0, sizeof(fl6));
> +	fl6.flowi6_oif		= amt->stream_dev->ifindex;
> +	fl6.flowi6_proto	= IPPROTO_UDP;
> +	fl6.daddr		= *daddr;
> +	fl6.saddr		= *saddr;
> +	fl6.fl6_dport		= dport;
> +	fl6.fl6_sport		= sport;

[Severity: Low]
fl6.flowlabel stays at zero here, so the route lookup uses DSCP 0.
amt_send_ctrl_v6() then sends the packet with AMT_TOS (0xc0, DSCP 48) as
the traffic class. Would a DSCP-based IPv6 policy rule match this traffic?

fib6_rule_match() in net/ipv6/fib6_rules.c checks the flow key:

	if ((r->dscp ^ ip6_dscp(fl6->flowlabel)) & r->dscp_mask)
		return 0;

A rule like "ip -6 rule add dscp 48 table X" would therefore not match. The
Advertisement could then be routed by another table, or fail to route,
even though the packet on the wire carries DSCP 48. The later IPv6 senders
in the series share amt_route6() and would behave the same way.

udp_tunnel6_dst_lookup() in net/ipv6/ip6_udp_tunnel.c builds the key from
the traffic class it sends:

	fl6.flowlabel = ip6_make_flowinfo(dsfield, key->label);

Should amt_route6() do the same with AMT_TOS? The existing IPv4 amt
control senders have the same mismatch: they pass tos 0 to
ip_route_output_ports() and then write iph->tos = AMT_TOS.

> +
> +	return ip6_dst_lookup_flow(amt->net, sk, &fl6, NULL);
> +}
> +
> +/* Send an AMT control message from @saddr over the IPv6 outer transport.
> + * Returns 0 once the message is handed to the IPv6 stack.
> + */
> +static int amt_send_ctrl_v6(struct amt_dev *amt, const struct in6_addr *saddr,
> +			    const struct in6_addr *daddr,
> +			    __be16 sport, __be16 dport,
> +			    const void *msg, unsigned int len)
> +{

[ ... ]

> +	skb_reserve(skb, hlen);
> +	skb_put_data(skb, msg, len);
> +	skb->priority = TC_PRIO_CONTROL;
> +	udp_tunnel6_xmit_skb(dst, sk, skb, NULL, saddr, daddr, AMT_TOS,
> +			     ip6_dst_hoplimit(dst), 0, sport, dport, false, 0);
> +	err = 0;

[Severity: Low]
Do output-path drops go uncounted in tx_errors here? dev is passed as NULL,
and ip6tunnel_xmit() in include/net/ip6_tunnel.h only looks at the
ip6_local_out() result when dev is set:

	err = ip6_local_out(skb_dst_dev_net(skb), sk, skb);

	if (dev) {
		if (unlikely(net_xmit_eval(err)))
			pkt_len = -1;
		iptunnel_xmit_stats(dev, pkt_len);
	}

err is then set to 0 here whatever happened.

The IPv4 sender of the same message, amt_send_advertisement(), counts
these drops:

	err = ip_local_out(amt->net, sk, skb);
	if (unlikely(net_xmit_eval(err)))
		amt->dev->stats.tx_errors++;

So these drops raise tx_errors on an IPv4 relay but not on an IPv6 one: a
netfilter OUTPUT drop, a neighbour or qdisc drop, or the ip6tunnel_xmit()
recursion-limit drop. On IPv6 only route lookup and skb allocation failures
are counted.

amt_send_discovery_v6() and amt_send_request_v6(), added later in the
series, reuse this helper. They would also get err == 0 for every packet
that reaches udp_tunnel6_xmit_skb().

The commit message says the NULL netdev is deliberate, to keep control
traffic out of the tunnel stats. Could the ip6_local_out() result still be
checked here so tx_errors behaves the same as on IPv4?

> +out:
> +	rcu_read_unlock_bh();
> +	return err;
> +}

[ ... ]

> @@ -2701,6 +2791,26 @@ static bool amt_discovery_handler(struct amt_dev *amt, struct sk_buff *skb)
>  	if (amtd->reserved || amtd->version)
>  		return true;
>  

[ ... ]

> +	if (amt_v6(amt)) {
> +		const struct ipv6hdr *ip6h = ipv6_hdr(skb);
> +
> +		/* The socket bound to :: also receives a Discovery sent to
> +		 * a group, and a multicast address can never be a source
> +		 * (RFC 4291 s2.7), so such a Discovery is not answered.
> +		 */
> +		if (ipv6_addr_is_multicast(&ip6h->daddr))
> +			return true;
> +
> +		amt_send_advertisement_v6(amt, amtd->nonce, &ip6h->daddr,
> +					  &ip6h->saddr, udph->source);

[Severity: Low]
What happens when the Discovery's outer source is ::? Only the destination
is checked, so ip6h->saddr becomes the Advertisement's destination without
any check.

ip6_rcv_core() drops a multicast or loopback source, but not an
unspecified one. UDPv6 delivery to the relay socket bound to :: does not
check it either. The path would be:

amt_discovery_handler()
  amt_send_advertisement_v6(..., daddr = ::, ...)
    amt_send_ctrl_v6()
      amt_route6()               fl6.daddr = ::
        ip6_dst_lookup_flow()
      udp_tunnel6_xmit_skb()     outer daddr = ::

ip6_dst_lookup_tail() treats a zero daddr as the default router. So the
lookup succeeds whenever stream_dev has a default route, and the relay
sends a packet with destination ::, which RFC 4291 s2.5.2 forbids.

Should a Discovery with ipv6_addr_any(&ip6h->saddr) be dropped too, like
the multicast destination case?

> +		return false;
> +	}
> +
>  	amt_send_advertisement(amt, amtd->nonce, iph->saddr, udph->source);
>  
>  	return false;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009122426.551178-1-omar%40blockcast.net