Re: [PATCH] tcp: fastopen: check rsk_drop_req() in tcp_fastopen_create_child()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Missing `bh_unlock_sock(child)` before `sock_put(child)` causes a locked spinlock and preemption/softirq-disable leak.
- [High] Returning NULL from `tcp_fastopen_create_child()` fails to abort the request, causing a rejected MPTCP MP_JOIN request to incorrectly fall back to standard SYN processing, leading to a spurious SYN-ACK and resource waste.
--

commit c3cab231705bf3a0b9db9f8531e40d30d546e830
Author: Yilin Zhang <yilinzhang@moonshot.ai>

tcp: fastopen: check rsk_drop_req() in tcp_fastopen_create_child()

This commit addresses a use-after-free vulnerability by checking
rsk_drop_req(req) in tcp_fastopen_create_child(). If an MPTCP MP_JOIN SYN
with a valid Fast Open cookie triggers the fatal fallback in
subflow_syn_recv_sock(), the request and child socket are now dropped.

> diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
> index 471c78be55137..b8b3abbee253d 100644
> --- a/net/ipv4/tcp_fastopen.c
> +++ b/net/ipv4/tcp_fastopen.c
> @@ -337,6 +337,11 @@ static struct sock *tcp_fastopen_create_child(struct sock *sk,
>  	if (!child)
>  		return NULL;
>  
> +	if (own_req && rsk_drop_req(req)) {
> +		sock_put(child);

[Severity: Critical]
Does this code leak the socket lock?

When a child socket is created and returned by syn_recv_sock(), it is
returned with the socket spinlock held. If we call sock_put(child) here
without first calling bh_unlock_sock(child), does this leave the spinlock
locked and softirqs disabled, leading to a soft lockup?

> +		return NULL;

[Severity: High]
Does returning NULL here cause the dropped request to be incorrectly retained?

When NULL is returned, tcp_fastopen_create_child() signals a normal Fast Open
failure rather than a hard drop. tcp_conn_request() will then assume Fast Open
failed, proceed to the standard SYN processing path, link the dropped request
into the SYN queue, and send a SYN-ACK:

net/ipv4/tcp_input.c:tcp_conn_request() {
    ...
	} else {
		tcp_rsk(req)->tfo_listener = false;
		if (!want_cookie &&
		    unlikely(!inet_csk_reqsk_queue_hash_add(sk, req))) {
			reqsk_free(req);
			dst_release(dst);
			return 0;
		}
		af_ops->send_synack(sk, dst, &fl, req, &foc,
				    !want_cookie ? TCP_SYNACK_NORMAL :
						   TCP_SYNACK_COOKIE,
				    skb);
    ...
}

Should this path return an error pointer or use a drop mechanism to ensure the
request is completely aborted, rather than triggering the fallback path?

> +	}
> +
>  	spin_lock(&queue->fastopenq.lock);
>  	queue->fastopenq.qlen++;
>  	spin_unlock(&queue->fastopenq.lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902121247.3248539-1-yilinzhang@moonshot.ai?part=1