Re: [PATCH net-next 04/13] amt: send the Membership Query over IPv6

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The IPv6 branch of amt_udp_xmit() routes Membership Queries
  without a traffic class but sends them with AMT_TOS.

--

amt: send the Membership Query over IPv6

This patch moves the route and transmit step of
amt_send_membership_query() into a new helper, amt_udp_xmit(). The helper
sends over the device's outer family: ip_route_output_key() and
udp_tunnel_xmit_skb() for IPv4, and amt_route6() and
udp_tunnel6_xmit_skb() for IPv6. The relay's General Query now uses the
IPv6 path when the device has an IPv6 outer transport.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index eaa5637674694..5e8a74cc0700e 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c

[ ... ]

> @@ -1098,6 +1103,65 @@ static void amt_req_work(struct work_struct *work)
>  				 msecs_to_jiffies(100));
>  }
>  
> +/* Route an AMT-encapsulated skb to @daddr and send it over the device's
> + * outer family. The caller has already pushed the AMT header.
> + */
> +static int amt_udp_xmit(struct amt_dev *amt, struct sock *sk,
> +			struct sk_buff *skb, const union amt_addr *daddr,
> +			__be16 sport, __be16 dport)
> +{
> +	struct rtable *rt;
> +	struct flowi4 fl4;
> +
> +	if (amt_v6(amt)) {
> +		struct dst_entry *dst;
> +
> +		dst = amt_route6(amt, sk, &amt->local_ipv6, &daddr->ip6,
> +				 sport, dport);

[ ... ]

> +		skb_scrub_packet(skb, false);
> +		udp_tunnel6_xmit_skb(dst, sk, skb, amt->dev, &amt->local_ipv6,
> +				     &daddr->ip6, AMT_TOS,
> +				     ip6_dst_hoplimit(dst), 0, sport, dport,
> +				     false, 0);

[Severity: Medium]
Should the IPv6 route lookup also use AMT_TOS? The packet goes out with
AMT_TOS as its traffic class, but the route is looked up with a traffic
class of 0.

amt_route6() clears the flowi6 and never sets the flowlabel:

drivers/net/amt.c:amt_route6() {
	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;

	return ip6_dst_lookup_flow(amt->net, sk, &fl6, NULL);
}

IPv6 policy routing takes the DSCP from fl6->flowlabel in
fib6_rule_match():

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

So a rule such as "ip -6 rule add dsfield 0xc0 table X" would never match
these Membership Queries. The lookup could then use a different table or
fail.

The IPv4 branch of the same helper does pass the DSCP into the lookup:

	fl4.flowi4_dscp        = inet_dsfield_to_dscp(AMT_TOS);

udp_tunnel6_dst_lookup() also puts the dsfield into the lookup:

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

The commit message says:

    Both use AMT_TOS, as the IPv4 path and the IPv6 control
    messages do.

That is true for the transmitted header, but not for route selection.

amt_send_ctrl_v6() (Advertisement, Discovery, Request) has the same
mismatch, since it also routes through amt_route6(). The mismatch is
still there at the end of the series.

Could amt_route6() set something like
fl6.flowlabel = ip6_make_flowinfo(AMT_TOS, 0) so the lookup matches the
header that is sent?

> +		return 0;
> +	}
> +
> +	memset(&fl4, 0, sizeof(struct flowi4));
> +	fl4.flowi4_oif         = amt->stream_dev->ifindex;
> +	fl4.daddr              = daddr->ip4;
> +	fl4.saddr              = amt->local_ip;
> +	fl4.flowi4_dscp        = inet_dsfield_to_dscp(AMT_TOS);
> +	fl4.flowi4_proto       = IPPROTO_UDP;

[ ... ]

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