Re: [PATCH] ipv6: fix payload_len inflation in ip6_xmit() when IPV6_RTHDRDSTOPTS is set without IPV6_RTHDR

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (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 commit message of "ipv6: fix payload_len inflation in
  ip6_xmit() when IPV6_RTHDRDSTOPTS is set without IPV6_RTHDR"…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but reported for
  completeness of the concern set: the per-socket extension-header…

--

ipv6: fix payload_len inflation in ip6_xmit() when IPV6_RTHDRDSTOPTS is set without IPV6_RTHDR

This normalizes the ipv6_txoptions passed to ip6_xmit() with
__ipv6_fixup_options() so that seg_len and head_room no longer account for
opt->dst0opt when opt->srcrt is NULL, since ipv6_push_nfrag_opts() does not
push dst0opt in that case.  The code change itself looks right and covers
every ip6_xmit() call site.  My questions below are about the changelog
text and about a related accounting mismatch elsewhere.

> ipv6: fix payload_len inflation in ip6_xmit() when IPV6_RTHDRDSTOPTS is set without IPV6_RTHDR
>
> When ipv6_fixup_options() was introduced to ignore destination options
> before a routing header (opt->dst0opt) unless a routing header
> (opt->srcrt) is also present, rawv6_sendmsg(), udpv6_sendmsg(), and
> inet6_csk_xmit() were updated to normalize `opt` via
> ipv6_fixup_options(), while other callers pass `np->opt` directly into
> ip6_xmit() (such as tcp_v6_send_response(), sctp_v6_xmit(), and
> l2tp_ip6_xmit()).

[Severity: Medium]
Could the list of affected callers be revisited?  Three of the four named
functions do not seem to match the code.

inet6_csk_xmit() in net/ipv6/inet6_connection_sock.c does not normalize
opt anywhere:

	res = ip6_xmit(sk, skb, fl6, sk->sk_mark, rcu_dereference(np->opt),
		       np->tclass, READ_ONCE(sk->sk_priority));

A grep for ipv6_fixup_options/__ipv6_fixup_options matches only
net/ipv6/ip6_output.c, net/ipv6/exthdrs.c, net/ipv6/raw.c, net/ipv6/udp.c,
include/net/ipv6.h and net/l2tp/l2tp_ip6.c.  Since inet6_csk_xmit() is the
queue_xmit handler in ipv6_specific, isn't the ordinary IPv6 TCP data path
one of the affected callers rather than an already-fixed one?

tcp_v6_send_response() passes NULL for opt:

		ip6_xmit(ctl_sk, buff, &fl6, fl6.flowi6_mark, NULL,
			 tclass, priority);

so can it inflate payload_len at all?

There is no l2tp_ip6_xmit() in the tree.  l2tp_ip6_sendmsg() already does:

	opt = ipv6_fixup_options(&opt_space, opt);
	ipc6.opt = opt;

and transmits via ip6_append_data(), with no ip6_xmit() call in
net/l2tp/l2tp_ip6.c.

Should the list instead read inet6_csk_xmit(), tcp_v6_send_synack() (which
falls back to rcu_dereference(np->opt) when ireq->ipv6_opt is NULL), and
sctp_v6_xmit()?

> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 550965058991..17d6add40864 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -295,6 +295,7 @@ int ip6_xmit(const struct sock *sk, struct sk_buff *skb, struct flowi6 *fl6,
>  	struct in6_addr *first_hop = &fl6->daddr;
>  	struct dst_entry *dst = skb_dst(skb);
>  	struct inet6_dev *idev = ip6_dst_idev(dst);
> +	struct ipv6_txoptions opt_space;
>  	struct net *net = sock_net(sk);
>  	unsigned int head_room;
>  	struct net_device *dev;
> @@ -304,6 +305,9 @@ int ip6_xmit(const struct sock *sk, struct sk_buff *skb, struct flowi6 *fl6,
>  	int ret, hlimit = -1;
>  	u32 mtu;
>  
> +	if (unlikely(opt))
> +		opt = __ipv6_fixup_options(&opt_space, opt);
> +

[Severity: Medium]
This isn't something introduced by this patch, but the change makes the
divergence more visible, so I'll mention it here.

With dst0opt suppressed, the extension header bytes ip6_xmit() emits for a
socket that has IPV6_RTHDRDSTOPTS set and no IPV6_RTHDR are
opt_flen + opt_nflen - ipv6_optlen(opt->dst0opt), because
ipv6_push_nfrag_opts() in net/ipv6/exthdrs.c only pushes dst0opt when
opt->srcrt is non-NULL.

The per-socket overhead accounting still uses the raw np->opt counters.
tcp_v6_connect():

	if (opt)
		icsk->icsk_ext_hdr_len += opt->opt_flen +
					  opt->opt_nflen;

tcp_v6_syn_recv_sock():

	if (opt)
		inet_csk(newsk)->icsk_ext_hdr_len = opt->opt_nflen +
						    opt->opt_flen;

do_ipv6_setsockopt() in net/ipv6/ipv6_sockglue.c:

			icsk->icsk_ext_hdr_len =
				psp_sk_overhead(sk) +
				opt->opt_flen + opt->opt_nflen;

and sctp_v6_ip_options_len() in net/sctp/ipv6.c:

	opt = rcu_dereference(np->opt);
	if (opt)
		len = opt->opt_flen + opt->opt_nflen;

Since inet6_csk_xmit() and sctp_v6_xmit() hand that same np->opt to
ip6_xmit(), do these sites overstate the emitted bytes by
ipv6_optlen(dst0opt), clamping the TCP MSS and lowering the SCTP
fragmentation point for bytes that never reach the wire?  The direction is
conservative, so I don't see an under-reservation, but should these be
normalized the same way?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919210620.3029622-1-benquike%40gmail.com