Re: [PATCH net] ipv4: free inet_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 · Medium: 0 · 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).

- [High] The fix is incomplete.

--

ipv4: free inet_opt after an RCU grace period

This commit changes inet_sock_destruct() to free inet->inet_opt with
kfree_rcu() instead of kfree(). tcp_v4_syn_recv_sock() hands
ireq->ireq_opt to the child socket without copying it. A SYNACK
retransmit running at the same time can still be reading it under
rcu_read_lock() only.

> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 4ce38c99fef9e..14ce01092fda6 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -161,7 +161,7 @@ void inet_sock_destruct(struct sock *sk)
>  	WARN_ON_ONCE(sk->sk_wmem_queued);
>  	WARN_ON_ONCE(sk->sk_forward_alloc);
>  
> -	kfree(rcu_dereference_protected(inet->inet_opt, 1));
> +	kfree_rcu(rcu_dereference_protected(inet->inet_opt, 1), rcu);

[Severity: High]
Is there a second handoff of ireq_opt that still ends in a plain kfree()?

With net.ipv4.tcp_migrate_req=1 and SO_REUSEPORT listeners,
reqsk_timer_handler() migrates a request socket whose listener has been
closed:

net/ipv4/inet_connection_sock.c:reqsk_timer_handler() {
    ...
		nreq = inet_reqsk_clone(req, nsk);
    ...
		if (!inet_ehash_insert(req_to_sk(nreq), req_to_sk(oreq), NULL)) {
    ...
		reqsk_migrate_reset(oreq);
    ...
}

inet_reqsk_clone() memcpy()s the request socket, so nreq->ireq_opt points
to the same ip_options_rcu as oreq. reqsk_migrate_reset() then clears
oreq's slot with a plain NULL store, and nreq becomes the owner.

At the same time, another CPU can hold a reference on oreq in
tcp_v4_rcv(), pick a new listener through reuseport_migrate_sock(), and
handle a retransmitted SYN:

tcp_v4_rcv()
  tcp_check_req()
    tcp_rtx_synack()
      tcp_v4_send_synack()
        rcu_dereference(ireq->ireq_opt)
        ip_build_and_send_pkt()
          ip_options_build()

nreq can then be dropped, for example by an RST through
tcp_check_req()->inet_csk_reqsk_queue_drop()->reqsk_put(), or when its
own timer expires. When that happens, the options are freed without a
grace period:

net/ipv4/tcp_ipv4.c:tcp_v4_reqsk_destructor() {
	kfree(rcu_dereference_protected(inet_rsk(req)->ireq_opt, 1));
}

The reference on oreq no longer protects the options, because oreq does
not own them anymore. The reader's rcu_read_lock() gives no protection
against a plain kfree() either.

Can this lead to the same use-after-free in ip_build_and_send_pkt() and
ip_options_build()? Both read opt->optlen, so if the memory is reallocated
in between, the two reads could disagree. That could turn into an
out-of-bounds write while the SYNACK is being built.

The commit message says the other paths that replace inet_opt already
use kfree_rcu(). Should tcp_v4_reqsk_destructor() also be converted to
kfree_rcu(..., rcu) as part of this fix?

>  	dst_release(rcu_dereference_protected(sk->sk_dst_cache, 1));
>  	dst_release(rcu_dereference_protected(sk->sk_rx_dst, 1));
>  	psp_sk_assoc_free(sk);

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