Re: [PATCH net] tcp: ipv6: free ireq->ipv6_opt after an RCU grace period

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [High] The fix is incomplete.

Pre-existing issues:
- [High] The ipv6_opt attached to a request socket is charged to the
  listener's sk_omem_alloc and never uncharged.
- [Low] After this patch, ireq->ipv6_opt is freed after an RCU grace
  period, and the commit message says its readers 'already run under…

--

tcp: ipv6: free ireq->ipv6_opt after an RCU grace period

When a request socket is migrated, the old request socket and its clone
share ireq->ipv6_opt. If the clone is freed while another CPU is still
using the options through the old request socket, that is a
use-after-free. The patch frees ipv6_opt with kfree_rcu() in
tcp_v6_reqsk_destructor(), as was already done for the IPv4 ireq_opt.

Closes: https://lore.kernel.org/netdev/179369559062.488856.15441115725436277776@kernel.org/

> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index e006a30360eae..517f34166cda6 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -607,7 +607,7 @@ static int tcp_v6_send_synack(const struct sock *sk, struct dst_entry *dst,
>  
>  static void tcp_v6_reqsk_destructor(struct request_sock *req)
>  {
> -	kfree(inet_rsk(req)->ipv6_opt);
> +	kfree_rcu(inet_rsk(req)->ipv6_opt, rcu);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but it
is on the line being changed. Is the listener's sk_omem_alloc charge for
ipv6_opt ever released?

calipso_req_setattr() charges the listener, sk_to_full_sk(req), through
ipv6_renew_options()->sock_kmalloc(). It only uncharges the options that
it replaces:

net/ipv6/calipso.c:calipso_req_setattr() {
    ...
	txopts = ipv6_renew_options(sk, req_inet->ipv6_opt, IPV6_HOPOPTS, new);
    ...
	txopts = xchg(&req_inet->ipv6_opt, txopts);
	if (txopts) {
		atomic_sub(txopts->tot_len, &sk->sk_omem_alloc);
		txopt_put(txopts);
	}
    ...
}

calipso_req_delattr() uncharges in the same way. tcp_v6_reqsk_destructor()
does the final free of ipv6_opt with no atomic_sub(), both with kfree()
before this patch and with kfree_rcu() after it.

tcp_v6_syn_recv_sock() charges the child separately through
ipv6_dup_options(newsk, ...), so the listener's charge stays in place.

On a listener with a CALIPSO label, does sk_omem_alloc grow by roughly
80-110 bytes for each connection request? With the default optmem_max of
128KB, sock_kmalloc() would start failing after about 1300 requests. Then
ipv6_renew_options() returns -ENOBUFS, and every later SYN is dropped by
the LSM hook in tcp_v6_route_req():

	if (security_inet_conn_request(sk, skb, req))
		return NULL;

When the listener is destroyed, __sk_destruct() would also report optmem
leakage.

[Severity: Low]
This isn't a bug introduced by this patch, but the commit message says
that both readers "already run under RCU, like their IPv4 counterparts".
Now that ipv6_opt is freed after a grace period, should it get the
matching RCU annotations?

In struct inet_request_sock, ipv6_opt is a plain pointer, while the IPv4
member of the same union is __rcu:

	union {
		struct ip_options_rcu __rcu	*ireq_opt;
#if IS_ENABLED(CONFIG_IPV6)
		struct {
			struct ipv6_txoptions	*ipv6_opt;

Both IPv6 readers load it with a plain read. For example, in
tcp_v6_syn_recv_sock():

	opt = ireq->ipv6_opt;
	if (!opt)
		opt = rcu_dereference(np->opt);
	if (opt) {
		opt = ipv6_dup_options(newsk, opt);

The IPv4 readers use rcu_dereference(ireq->ireq_opt). Meanwhile,
reqsk_migrate_reset() writes NULL to the same field with a plain store,
on a request that other CPUs may still be reading.

KCSAN would report this as a data race, and sparse cannot check the RCU
usage. In theory the compiler could reload ireq->ipv6_opt after the NULL
test and pass NULL to ipv6_dup_options().

Would it make sense to add __rcu, rcu_dereference() and RCU_INIT_POINTER()
on top of the kfree_rcu() change?

>  	consume_skb(inet_rsk(req)->pktopts);

[Severity: High]
Does this fully close the window described in the commit message?

After inet_reqsk_clone(), the old request and its clone also share the
same inet_rsk(req)->pktopts skb. reqsk_migrate_reset() clears it together
with ipv6_opt:

net/ipv4/inet_connection_sock.c:reqsk_migrate_reset() {
    ...
	inet_rsk(req)->ipv6_opt = NULL;
	inet_rsk(req)->pktopts = NULL;
    ...
}

pktopts is still freed right away, both here and in tcp_v6_syn_recv_sock()
when that request wins own_req:

net/ipv6/tcp_ipv6.c:tcp_v6_syn_recv_sock() {
    ...
	if (*own_req) {
		...
		if (ireq->pktopts) {
			newnp->pktoptions = skb_clone_and_charge_r(ireq->pktopts, newsk);
			consume_skb(ireq->pktopts);
			ireq->pktopts = NULL;
    ...
}

At the same time, tcp_v6_send_synack() on the sibling request can read it
without any RCU protection:

net/ipv6/tcp_ipv6.c:tcp_v6_send_synack() {
    ...
		if (inet6_test_bit(REPFLOW, sk) && ireq->pktopts)
			fl6->flowlabel = ip6_flowlabel(ipv6_hdr(ireq->pktopts));
    ...
}

tcp_v6_init_req() keeps pktopts whenever REPFLOW (or the common rxopt
bits) is set on the listener, so this does not need CALIPSO.

With REPFLOW set, tcp_migrate_req=1 (or a BPF migrate program), and a
listener that is closing, one possible sequence is:

CPU A                                CPU C
reqsk_timer_handler(oreq)
  nreq = inet_reqsk_clone(oreq, nsk)
  tcp_rtx_synack(nsk, nreq)
    tcp_v6_send_synack()             tcp_check_req(oreq) (final ACK)
                                       tcp_v6_syn_recv_sock(oreq)
                                         own_req = true
                                         consume_skb(oreq->pktopts)
      ipv6_hdr(ireq->pktopts)  <- skb already freed

The timer's reference on oreq protects ipv6_opt in this case, but it does
not protect pktopts.

The reverse order also looks possible. CPU C handles a retransmitted SYN
on oreq and calls tcp_check_req()->tcp_rtx_synack()->tcp_v6_send_synack().
At the same time, CPU A has done inet_ehash_insert(nreq, oreq) but not yet
reqsk_migrate_reset(oreq). CPU B then processes the final ACK on nreq and
frees the shared skb in tcp_v6_syn_recv_sock().

In both cases ipv6_hdr() reads from a freed sk_buff. That can oops, or it
can put stale bits into the flow label of the SYNACK.

tcp_v6_syn_recv_sock() frees the skb directly, so switching to an RCU
deferred free would not help here. Does pktopts need its own handling,
for example an skb_get() for the clone, so that each request drops only
its own reference?

>  }
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103090011.4128420-1-edumazet%40kernel.org