Re: [PATCH 1/1] [PATCH] l2tp: fix IPv6 xmit frag failure for oversized non-GSO packets

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
l2tp: fix IPv6 xmit frag failure for oversized non-GSO packets

This patch sets skb->ignore_df from ip6_sk_ignore_df(sk) inside ip6_xmit()
so that oversized non-GSO L2TPv3-over-IPv6 outer packets follow the
fragmentation path instead of being dropped with EMSGSIZE.

> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index c14adcdd4396..f5370554cfb5 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -359,6 +359,7 @@ int ip6_xmit(const struct sock *sk, struct sk_buff *skb, struct flowi6 *fl6,
>  	skb->protocol = htons(ETH_P_IPV6);
>  	skb->priority = priority;
>  	skb->mark = mark;
> +	skb->ignore_df = ip6_sk_ignore_df(sk);

[High]
Can this unconditional assignment clobber an skb->ignore_df that the
caller deliberately set before invoking ip6_xmit()?

For example, sctp_v6_xmit() in net/sctp/ipv6.c does:

	if (!(t->param_flags & SPP_PMTUD_ENABLE))
		skb->ignore_df = 1;
	...
	res = ip6_xmit(sk, skb, fl6, sk->sk_mark,
		       rcu_dereference(np->opt),
		       tclass, READ_ONCE(sk->sk_priority));

If the SCTP socket has pmtudisc set to IPV6_PMTUDISC_DO, _PROBE, or
_INTERFACE via setsockopt(IPV6_MTU_DISCOVER), ip6_sk_ignore_df() returns
false:

	static inline bool ip6_sk_ignore_df(const struct sock *sk)
	{
		u8 pmtudisc = READ_ONCE(inet6_sk(sk)->pmtudisc);

		return pmtudisc < IPV6_PMTUDISC_DO ||
		       pmtudisc == IPV6_PMTUDISC_OMIT;
	}

In that combination, won't the new write reset ignore_df back to 0 and
defeat the per-transport SPP_PMTUD_ENABLE-off setting, sending the
packet down the EMSGSIZE / IPSTATS_MIB_FRAGFAILS branch that the patch
is trying to avoid?

The same concern applies to L2TP itself. l2tp_xmit_queue() in
net/l2tp/l2tp_core.c does:

	skb->ignore_df = 1;
	skb_dst_drop(skb);
	if (l2tp_sk_is_v6(tunnel->sock))
		err = inet6_csk_xmit(tunnel->sock, skb, NULL);

inet6_csk_xmit() does not touch skb->ignore_df before reaching
ip6_xmit(). If the tunnel socket is configured with
IPV6_PMTUDISC_DO, doesn't the new line in ip6_xmit() also overwrite the
ignore_df=1 that l2tp_xmit_queue() just set?

Other ip6_xmit() callers (TCP via inet6_csk_xmit/tcp_v6_send_synack/
tcp_v6_send_response, XFRM NAT keepalive via nat_keepalive_send_ipv6)
inherit this behaviour change too. Would an OR-merge such as

	skb->ignore_df = skb->ignore_df || ip6_sk_ignore_df(sk);

or fixing this in the offending L2TP path be more appropriate, so the
contract of ip6_xmit() with its other callers is preserved?

[Medium]
The commit message says:

    For oversized non-GSO packets skb->ignore_df is not set, causing the
    packet to fail the transmit checks and hit the
    EMSGSIZE/IPSTATS_MIB_FRAGFAILS path instead of being fragments.

Is that accurate for the L2TPv3 data path? l2tp_xmit_queue() already
unconditionally does:

	skb->ignore_df = 1;
	...
	err = inet6_csk_xmit(tunnel->sock, skb, NULL);

and inet6_csk_xmit() does not clear it before ip6_xmit(). With the
pre-patch test

	if (likely((skb->len <= mtu) || skb->ignore_df || skb_is_gso(skb)))

ignore_df=1 should already short-circuit the MTU check on the data
path. Is the failure actually coming from a different path (for
instance an L2TPIP control-socket send that bypasses
l2tp_xmit_queue())? Could the changelog be updated to identify the
exact path that was hitting the EMSGSIZE branch?

>  	mtu = dst6_mtu(dst);
>  	if (likely((skb->len <= mtu) || skb->ignore_df || skb_is_gso(skb))) {

[Low]
After this change, ip6_xmit() and __ip_queue_xmit() have asymmetric
semantics for skb->ignore_df. The IPv4 helper only reads it:

	if (ip_dont_fragment(sk, &rt->dst) && !skb->ignore_df)

while ip6_xmit() now writes it from the socket's pmtudisc, overriding
callers. Should the two paths be aligned, or should this divergence be
documented near ip6_xmit()?

Also, two small typos in the commit message:

    the packet length does not excent the MTU
                              ^^^^^^ exceed?

    instead of being fragments.
                     ^^^^^^^^^ fragmented?

    transmited as intended.
    ^^^^^^^^^^ transmitted?