* Re: [PATCH net] tcp: ipv6: keep request pktopts alive until request destruction
2026-11-02 6:12 [PATCH net] tcp: ipv6: keep request pktopts alive until request destruction Yingjie Wang
@ 2026-11-02 7:03 ` Kuniyuki Iwashima
2026-11-02 7:28 ` Eric Dumazet
2026-11-03 6:58 ` netdev-bot+sashiko
1 sibling, 1 reply; 4+ messages in thread
From: Kuniyuki Iwashima @ 2026-11-02 7:03 UTC (permalink / raw)
To: Yingjie Wang
Cc: netdev, edumazet, ncardwell, davem, kuba, pabeni, horms, daniel,
martin.lau, linux-kernel, stable
On Mon, 02 Nov 2026 06:12:18 +0000, Yingjie Wang wrote:
> When IPv6 flow label reflection is enabled, tcp_v6_send_synack() reads
> ireq->pktopts to copy the flow label from the initial SYN.
>
> Request sockets can be processed without the listener lock. A
> retransmitted SYN can therefore run tcp_v6_send_synack() while a
> concurrent final ACK creates the child in tcp_v6_syn_recv_sock(). The
> latter clones ireq->pktopts for the child and immediately consumes the
> request's reference. This can free the skb while the retransmit path is
> still reading its IPv6 header, causing a use-after-free.
>
> Request migration has the same ownership problem. inet_reqsk_clone()
> copies pktopts without taking an skb reference, and reqsk_migrate_reset()
> then clears the old request's pointer. A CPU holding the old request can
> still be sending a SYNACK when the cloned request releases the only skb
> reference.
>
> Do not consume pktopts while creating the child. Give each cloned request
> its own skb reference and let every request release its reference from
> tcp_v6_reqsk_destructor().
>
> I found this while testing IPv6 TCP request-socket lifetime handling. The
> reproducer required no source changes, artificial delay, or fault
> injection beyond enabling KASAN. It triggered the use-after-free before
> this change. With this change, a 2000-round run completed without a KASAN
> report. A separate 10-round test recorded four successful request
> migrations and no KASAN report. Networking selftests were not run because
> none directly exercises this race.
>
> Fixes: e994b2f0fb92 ("tcp: do not lock listener to process SYN packets")
> Fixes: 079096f103fa ("tcp/dccp: install syn_recv requests into ehash table")
> Fixes: c905dee62232 ("tcp: Migrate TCP_NEW_SYN_RECV requests at retransmitting SYN+ACKs.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yingjie Wang <x-professor@outlook.com>
> ---
> net/ipv4/inet_connection_sock.c | 6 +++++-
> net/ipv6/tcp_ipv6.c | 7 ++++---
> 2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 93b306d91b6d..66f8ce25aa38 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -936,6 +936,11 @@ static struct request_sock *inet_reqsk_clone(struct request_sock *req,
> req->rsk_ops->obj_size - offsetof(struct sock, sk_dontcopy_end),
> /* alloc is larger than struct, see above */);
>
> +#if IS_ENABLED(CONFIG_IPV6)
> + if (inet_rsk(nreq)->pktopts)
> + skb_get(inet_rsk(nreq)->pktopts);
> +#endif
> +
> sk_node_init(&nreq_sk->sk_node);
> nreq_sk->sk_tx_queue_mapping = req_sk->sk_tx_queue_mapping;
> #ifdef CONFIG_SOCK_RX_QUEUE_MAPPING
> @@ -982,7 +987,6 @@ static void reqsk_migrate_reset(struct request_sock *req)
> req->saved_syn = NULL;
> #if IS_ENABLED(CONFIG_IPV6)
> inet_rsk(req)->ipv6_opt = NULL;
> - inet_rsk(req)->pktopts = NULL;
> #else
> inet_rsk(req)->ireq_opt = NULL;
> #endif
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index e006a30360ea..0b51cb4ad960 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1522,11 +1522,12 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff *
> if (*own_req) {
> tcp_move_syn(newtp, req);
>
> - /* Clone pktoptions received with SYN, if we own the req */
> + /* Clone pktoptions received with SYN. Keep the request's
> + * reference until the request is destroyed, as a concurrent
> + * SYN retransmit may still read it.
> + */
> if (ireq->pktopts) {
> newnp->pktoptions = skb_clone_and_charge_r(ireq->pktopts, newsk);
> - consume_skb(ireq->pktopts);
> - ireq->pktopts = NULL;
This is reported from another person in security@ mailing list
yesterday and Eric has a better fix.
pw-bot: rejected
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net] tcp: ipv6: keep request pktopts alive until request destruction
2026-11-02 6:12 [PATCH net] tcp: ipv6: keep request pktopts alive until request destruction Yingjie Wang
2026-11-02 7:03 ` Kuniyuki Iwashima
@ 2026-11-03 6:58 ` netdev-bot+sashiko
1 sibling, 0 replies; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-11-03 6:58 UTC (permalink / raw)
To: x-professor
Cc: netdev, edumazet, ncardwell, kuniyu, davem, kuba, pabeni, horms,
daniel, martin.lau, linux-kernel, stable
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 1 · High: 0 (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).
- [Critical] This patch adds a remote-triggerable use-after-free on the
IPv6 syncookie path.
Pre-existing issues:
- [High] The patch fixes the request-migration ownership race for
ireq->pktopts but leaves the same race open for the other request…
--
tcp: ipv6: keep request pktopts alive until request destruction
This patch removes the code in tcp_v6_syn_recv_sock() that dropped the
request's ireq->pktopts reference when the child was created. It also
makes inet_reqsk_clone() take its own skb reference for migrated requests,
so every request now releases pktopts from tcp_v6_reqsk_destructor(). The
aim is to close a use-after-free where a concurrent SYNACK retransmit
reads ireq->pktopts after it has been freed.
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 93b306d91b6dc..66f8ce25aa383 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -936,6 +936,11 @@ static struct request_sock *inet_reqsk_clone(struct request_sock *req,
> req->rsk_ops->obj_size - offsetof(struct sock, sk_dontcopy_end),
> /* alloc is larger than struct, see above */);
>
> +#if IS_ENABLED(CONFIG_IPV6)
> + if (inet_rsk(nreq)->pktopts)
> + skb_get(inet_rsk(nreq)->pktopts);
> +#endif
> +
> sk_node_init(&nreq_sk->sk_node);
> nreq_sk->sk_tx_queue_mapping = req_sk->sk_tx_queue_mapping;
> #ifdef CONFIG_SOCK_RX_QUEUE_MAPPING
> @@ -982,7 +987,6 @@ static void reqsk_migrate_reset(struct request_sock *req)
> req->saved_syn = NULL;
> #if IS_ENABLED(CONFIG_IPV6)
> inet_rsk(req)->ipv6_opt = NULL;
> - inet_rsk(req)->pktopts = NULL;
> #else
> inet_rsk(req)->ireq_opt = NULL;
> #endif
[Severity: High]
This is a pre-existing issue, not one introduced by this patch. The same
migration race still seems to be open for ireq->ipv6_opt, which is the
other request field that tcp_v6_send_synack() reads.
inet_reqsk_clone() copies the ipv6_opt pointer into nreq with memcpy. It
takes no reference and makes no copy. reqsk_migrate_reset(oreq) then
clears the pointer on the old request, which leaves nreq as the only
owner. Both reqsk_timer_handler() and inet_csk_complete_hashdance() go
through this path.
Another CPU may still hold oreq. It found oreq through ehash, migrated it
via reuseport_migrate_sock(), and then got a retransmitted SYN:
tcp_check_req()
inet_rtx_syn_ack()
tcp_v6_send_synack()
That CPU can be in:
net/ipv6/tcp_ipv6.c:tcp_v6_send_synack() {
...
rcu_read_lock();
opt = ireq->ipv6_opt;
if (!opt)
opt = rcu_dereference(np->opt);
err = ip6_xmit(sk, skb, fl6, skb->mark ? : READ_ONCE(sk->sk_mark),
opt, tclass, READ_ONCE(sk->sk_priority));
rcu_read_unlock();
...
}
Meanwhile nreq is released (after accept(), on listener close, or on
expiry) and runs:
static void tcp_v6_reqsk_destructor(struct request_sock *req)
{
kfree(inet_rsk(req)->ipv6_opt);
consume_skb(inet_rsk(req)->pktopts);
}
This kfree() is not deferred through RCU, so the reader's rcu_read_lock()
does not protect opt. Can ip6_xmit() then read a freed struct
ipv6_txoptions while it pushes the extension headers?
ipv6_opt is only non-NULL with CALIPSO labelling (calipso_req_setattr()).
The race also needs SO_REUSEPORT, net.ipv4.tcp_migrate_req=1 and a
listener that is closing.
This patch already reworks pktopts ownership in inet_reqsk_clone() and
reqsk_migrate_reset(). Should ipv6_opt be handled the same way there?
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index e006a30360eae..0b51cb4ad9604 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1522,11 +1522,12 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff *
> if (*own_req) {
> tcp_move_syn(newtp, req);
>
> - /* Clone pktoptions received with SYN, if we own the req */
> + /* Clone pktoptions received with SYN. Keep the request's
> + * reference until the request is destroyed, as a concurrent
> + * SYN retransmit may still read it.
> + */
> if (ireq->pktopts) {
> newnp->pktoptions = skb_clone_and_charge_r(ireq->pktopts, newsk);
> - consume_skb(ireq->pktopts);
> - ireq->pktopts = NULL;
> if (newnp->pktoptions)
> tcp_v6_restore_cb(newnp->pktoptions);
> }
[Severity: Critical]
Does removing this consume_skb() create a use-after-free on the IPv6
syncookie path?
On that path ireq->pktopts does not hold the SYN. cookie_v6_check()
stores the final ACK skb, which is still being processed, by bumping the
refcount directly:
net/ipv6/syncookies.c:cookie_v6_check() {
...
if (ipv6_opt_accepted(sk, skb, &TCP_SKB_CB(skb)->header.h6) ||
np->rxopt.bits.rxinfo || np->rxopt.bits.rxoinfo ||
np->rxopt.bits.rxhlim || np->rxopt.bits.rxohlim) {
refcount_inc(&skb->users);
ireq->pktopts = skb;
}
...
}
Before this patch, the consume_skb() above dropped skb->users back to 1
before the child processed the skb.
Now tcp_get_cookie_sock() calls inet_csk_reqsk_queue_add(sk, req, child).
The request on the accept queue still points at the in-flight ACK, and
skb->users is 2.
The same skb is then passed to the child:
tcp_v6_do_rcv()
tcp_child_process(sk, nsk, skb)
tcp_rcv_state_process()
tcp_data_queue()
For a pure ACK, tcp_data_queue() frees the skb without checking
skb->users:
if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) {
__kfree_skb(skb);
return;
}
The consume label in tcp_rcv_state_process() and tcp_collapse_one() also
use __kfree_skb() in the same way.
When the request is released later, either by reqsk_put() from accept()
or by inet_csk_listen_stop(), the destructor runs:
tcp_v6_reqsk_destructor()
consume_skb(inet_rsk(req)->pktopts);
Wouldn't this decrement the refcount of, and possibly free again, an skb
that has already been freed?
The trigger needs a remote peer that gets syncookies sent (SYN queue
overflow with the default tcp_syncookies=1, or tcp_syncookies=2) and then
completes the handshake with a pure ACK. The only local requirement is a
common IPv6 rx option on the listener, such as IPV6_RECVPKTINFO or
IPV6_RECVHOPLIMIT.
Syncookie requests are never hashed and never retransmit SYNACKs, so the
race this patch fixes cannot happen for them. Could the cookie path keep
dropping the request's pktopts reference here, or could cookie_v6_check()
store a separate clone instead of the in-flight skb?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/82c31e4d5043f3dbd11b44177cfba779d335aae3.1793599929.git.x-professor%40outlook.com
^ permalink raw reply [flat|nested] 4+ messages in thread