* Re: [PATCH net v3] tipc: fix refcount panic in tipc_rcv() when receiving shared skb
2026-10-22 1:15 [PATCH net v3] tipc: fix refcount panic in tipc_rcv() when receiving shared skb Tung Nguyen
@ 2026-10-22 17:20 ` Eric Dumazet
2026-10-23 2:03 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: Eric Dumazet @ 2026-10-22 17:20 UTC (permalink / raw)
To: Tung Nguyen
Cc: netdev, davem, kuba, pabeni, jmaloy, horms, tipc-discussion,
syzbot+0017742620a9d06f06ef, Nguyen Ngoc Thang
Le jeu. 22 oct. 2026 à 03:15, Tung Nguyen <tung.quang.nguyen@est.tech> a écrit :
>
> TIPC assumes that the received skb is not shared with other users.
> However, syzbot reports a bug triggered by receiving a shared skb:
>
> ...
> RIP: 0010:pskb_expand_head+0x135a/0x13a0 net/core/skbuff.c:2308
> __pskb_pull_tail+0x111/0x1870 net/core/skbuff.c:2890
> skb_cow_data+0x2ed/0xc50 net/core/skbuff.c:5398
> tipc_aead_decrypt net/tipc/crypto.c:892 [inline]
> tipc_crypto_rcv+0x346/0x23d0 net/tipc/crypto.c:1843
> tipc_rcv+0x4b7/0x1040 net/tipc/node.c:2119
> tipc_l2_rcv_msg+0x2d9/0x470 net/tipc/bearer.c:669
> deliver_ptype_list_skb net/core/dev.c:2533 [inline]
> ...
>
> The root cause is that local delivery hooks in the kernel (e.g.,
> deliver_skb()) may increment the skb's reference count (`users`) before
> TIPC processes it on ingress.
>
> Fix this by calling skb_share_check() to ensure exclusive ownership
> before passing the skb to tipc_rcv(). As TIPC needs to modify the skb's
> data, the cloned skb needs to be uncloned in tipc_sk_conn_proto_rcv(),
> tipc_sk_filter_connect(), tipc_msg_lookup_dest() and tipc_link_tnl_rcv().
>
> Reported-by: syzbot+0017742620a9d06f06ef@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=0017742620a9d06f06ef
> Closes: https://lore.kernel.org/all/20261018145928.58317-1-ngocthang2710.1999@gmail.com/T/
> Fixes: 6e967adf798a ("tipc: relocate common functions from media to bearer")
This commit only moved recv_msg() from eth_media.c to bearer.c,
the bug was there from day 0.
Fixes: b97bf3fd8f6a ("[TIPC] Initial merge")
> Co-developed-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
> Signed-off-by: Tung Nguyen <tung.quang.nguyen@est.tech>
> ---
> v3: Use skb_share_check() to unshare the skb and unclone that skb
> when needed.
> v2: https://lore.kernel.org/all/20261020104852.90350-1-tung.quang.nguyen@est.tech/
> v1: https://lore.kernel.org/all/20261019050648.287309-1-tung.quang.nguyen@est.tech/
>
> net/tipc/bearer.c | 4 ++++
> net/tipc/link.c | 7 +++++++
> net/tipc/msg.c | 3 +++
> net/tipc/socket.c | 10 ++++++++++
> 4 files changed, 24 insertions(+)
>
> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> index 05dcd2f9e887..5e7e220fa7f5 100644
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c
> @@ -659,6 +659,10 @@ static int tipc_l2_rcv_msg(struct sk_buff *skb, struct net_device *dev,
> {
> struct tipc_bearer *b;
>
> + skb = skb_share_check(skb, GFP_ATOMIC);
> + if (!skb)
> + return NET_RX_DROP;
> +
This part is fine, and is what other L3 handlers do.
The skb_unclone() parts are addressing a different (and older) issue:
TIPC already receives cloned skbs today, as soon as a packet tap
(tcpdump) is active, since af_packet clones the shared skb.
Please split this into two patches: the skb_share_check() one
(small, easy to backport), and a second one dealing with cloned skbs.
Some comments on the second part:
> rcu_read_lock();
> b = rcu_dereference(dev->tipc_ptr) ?:
> rcu_dereference(orig_dev->tipc_ptr);
> diff --git a/net/tipc/link.c b/net/tipc/link.c
> index 6427c69f8929..d7ce384d3b89 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -1363,6 +1363,13 @@ static int tipc_link_tnl_rcv(struct tipc_link *l, struct sk_buff *skb,
> }
> kfree_skb(skb);
> } else {
> + if (skb_unclone(skb, GFP_ATOMIC) == -ENOMEM) {
> + kfree_skb(skb);
> + return tipc_link_fsm_evt(l, LINK_FAILURE_EVT);
> + }
> +
> + hdr = buf_msg(skb);
> +
> /* Set fragment type for buf_append */
> if (msg_fragm_no(hdr) == 1)
> msg_set_type(hdr, FIRST_FRAGMENT);
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index b0bba0feef56..9a222e15ed31 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -736,6 +736,9 @@ bool tipc_msg_lookup_dest(struct net *net, struct sk_buff *skb, int *err)
> sk.node = tipc_scope2node(net, scope);
> if (!tipc_nametbl_lookup_anycast(net, &ua, &sk))
> return false;
> + if (skb_unclone(skb, GFP_ATOMIC) == -ENOMEM)
> + return false;
> + msg = buf_msg(skb);
> msg_incr_reroute_cnt(msg);
> if (sk.node != self)
> msg_set_prevnode(msg, self);
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index d5d70eb230b5..9789d1ce6bd4 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -1345,6 +1345,10 @@ static void tipc_sk_conn_proto_rcv(struct tipc_sock *tsk, struct sk_buff *skb,
> tsk_peer_port(tsk));
> sk->sk_state_change(sk);
>
> + if (skb_unclone(skb, GFP_ATOMIC) == -ENOMEM)
> + goto exit;
> + hdr = buf_msg(skb);
> +
> /* State change is ignored if socket already awake,
> * - convert msg to abort msg and add to inqueue
> */
> @@ -1359,6 +1363,9 @@ static void tipc_sk_conn_proto_rcv(struct tipc_sock *tsk, struct sk_buff *skb,
> tsk->probe_unacked = false;
>
> if (mtyp == CONN_PROBE) {
> + if (skb_unclone(skb, GFP_ATOMIC) == -ENOMEM)
> + goto exit;
tipc_msg_reverse() always allocates a new skb, so there is no need
to unclone here. Simply set the type on the new skb:
if (tipc_msg_reverse(onode, &skb, TIPC_OK)) {
msg_set_type(buf_msg(skb), CONN_PROBE_REPLY);
__skb_queue_tail(xmitq, skb);
}
> + hdr = buf_msg(skb);
> msg_set_type(hdr, CONN_PROBE_REPLY);
> if (tipc_msg_reverse(onode, &skb, TIPC_OK))
> __skb_queue_tail(xmitq, skb);
> @@ -2223,6 +2230,9 @@ static bool tipc_sk_filter_connect(struct tipc_sock *tsk, struct sk_buff *skb,
> return true;
> /* Empty ACK-, - wake up sleeping connect() and drop */
> sk->sk_state_change(sk);
> + if (skb_unclone(skb, GFP_ATOMIC) == -ENOMEM)
> + break;
The break leads to the "Abort connection setup attempt" path, after
tipc_sk_finish_conn() was already called. A memory allocation failure
should not abort a connection that was just established.
msg_set_dest_droppable() is only used here to ask the caller to
silently drop the packet. Now that the skb is not shared, a bit in
TIPC_SKB_CB() (or a different return value) would avoid both the
header write and this new error path.
> + hdr = buf_msg(skb);
> msg_set_dest_droppable(hdr, 1);
> return false;
> }
> --
> 2.53.0
>
skb_unclone() returns 0 or -ENOMEM, the usual form is:
if (skb_unclone(skb, GFP_ATOMIC))
Because it is very possible that in the future skb_unclone() start
returning other error codes instead of crashing with BUG_ON()
pw-bot: cr
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net v3] tipc: fix refcount panic in tipc_rcv() when receiving shared skb
2026-10-22 1:15 [PATCH net v3] tipc: fix refcount panic in tipc_rcv() when receiving shared skb Tung Nguyen
2026-10-22 17:20 ` Eric Dumazet
@ 2026-10-23 2:03 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-10-23 2:03 UTC (permalink / raw)
To: tung.quang.nguyen
Cc: netdev, davem, kuba, edumazet, pabeni, jmaloy, horms,
tipc-discussion, syzbot+0017742620a9d06f06ef, ngocthang2710.1999
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 3 · 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] tipc: lost disconnect abort in tipc_sk_conn_proto_rcv() on
unclone error
- [Medium] In tipc_sk_filter_connect() (net/tipc/socket.c), the new
skb_unclone() failure in the empty-ACK branch (state TIPC_CONNECTING,…
- [Medium] In tipc_link_tnl_rcv() (net/tipc/link.c), the new
skb_unclone() failure path always returns tipc_link_fsm_evt(l,…
- [Medium] The commit says the skb has to be uncloned where TIPC modifies
packet data, and lists four sites.
Pre-existing issues:
- [Low] In tipc_sk_conn_proto_rcv() (net/tipc/socket.c), the
msg_errcode() branch now does `goto exit` (kfree_skb) when…
--
tipc: fix refcount panic in tipc_rcv() when receiving shared skb
This patch calls skb_share_check() in tipc_l2_rcv_msg() so that TIPC owns
the skb before it is passed to tipc_rcv(). The result may still be a clone,
so the patch adds skb_unclone() calls before the in-place header writes in
tipc_sk_conn_proto_rcv(), tipc_sk_filter_connect(), tipc_msg_lookup_dest()
and tipc_link_tnl_rcv().
Closes: https://syzkaller.appspot.com/bug?extid=0017742620a9d06f06ef
Closes: https://lore.kernel.org/all/20261018145928.58317-1-ngocthang2710.1999@gmail.com/T/
> Fix this by calling skb_share_check() to ensure exclusive ownership
> before passing the skb to tipc_rcv(). As TIPC needs to modify the skb's
> data, the cloned skb needs to be uncloned in tipc_sk_conn_proto_rcv(),
> tipc_sk_filter_connect(), tipc_msg_lookup_dest() and tipc_link_tnl_rcv().
[Severity: Medium]
Is this list complete? tipc_get_gap_ack_blks() in net/tipc/link.c also
writes into the received packet's data, in its backward compatible
unicast branch:
net/tipc/link.c:tipc_get_gap_ack_blks() {
...
} else if (uc && sz == struct_size(p, gacks, p->ugack_cnt)) {
if (p->ugack_cnt) {
p->bgack_cnt = 0;
goto ok;
}
}
...
}
Here p points into msg_data(hdr) of the incoming skb. Before this write,
the caller tipc_link_proto_rcv() only calls skb_linearize():
net/tipc/link.c:tipc_link_proto_rcv() {
...
if (skb_linearize(skb))
goto exit;
...
case STATE_MSG:
/* Validate Gap ACK blocks, drop if invalid */
glen = tipc_get_gap_ack_blks(&ga, l, hdr, true);
...
}
skb_linearize() does nothing for a linear skb that is cloned. Suppose a
peer sends a STATE_MSG whose gap ACK size matches only ugack_cnt. That
would zero bgack_cnt in data still shared with another clone holder. The
holder could be the original skb that skb_share_check() cloned, or a
packet socket clone.
Would a single skb_unshare() style copy at TIPC ingress be more robust
than adding skb_unclone() at each write site?
> diff --git a/net/tipc/link.c b/net/tipc/link.c
> index 6427c69f8929..d7ce384d3b89 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -1363,6 +1363,13 @@ static int tipc_link_tnl_rcv(struct tipc_link *l, struct sk_buff *skb,
> }
> kfree_skb(skb);
> } else {
> + if (skb_unclone(skb, GFP_ATOMIC) == -ENOMEM) {
> + kfree_skb(skb);
> + return tipc_link_fsm_evt(l, LINK_FAILURE_EVT);
> + }
[Severity: Medium]
Should this failure path skip the link failure event for the broadcast
receive link? Later in this same branch, the reassembly failure handling
exempts it:
if (!tipc_buf_append(reasm_tnlmsg, &skb)) {
/* Successful but non-complete reassembly? */
if (*reasm_tnlmsg || link_is_bc_rcvlink(l))
return 0;
pr_warn_ratelimited("Unable to reassemble tunnel msg\n");
return tipc_link_fsm_evt(l, LINK_FAILURE_EVT);
}
tipc_link_input() makes the same exemption.
Broadcast packets can reach this code through:
tipc_rcv()->tipc_node_bc_rcv()->tipc_bcast_rcv()->tipc_link_rcv()
->tipc_link_tnl_rcv()
If the bc rcvlink is established, LINK_FAILURE_EVT moves it to
LINK_RESETTING and returns TIPC_LINK_DOWN_EVT. tipc_node_bc_rcv() then
does:
if (rc & TIPC_LINK_DOWN_EVT)
tipc_node_reset_links(n);
Could a GFP_ATOMIC failure on a cloned, fragmented tunnel message that
arrives on the broadcast path then reset every link to that peer?
> +
> + hdr = buf_msg(skb);
> +
[ ... ]
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index d5d70eb230b5..9789d1ce6bd4 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -1345,6 +1345,10 @@ static void tipc_sk_conn_proto_rcv(struct tipc_sock *tsk, struct sk_buff *skb,
> tsk_peer_port(tsk));
> sk->sk_state_change(sk);
>
> + if (skb_unclone(skb, GFP_ATOMIC) == -ENOMEM)
> + goto exit;
> + hdr = buf_msg(skb);
> +
[Severity: High]
If skb_unclone() fails here, the socket has already been moved to
TIPC_DISCONNECTING. tipc_node_remove_conn() has already run and
sk_state_change() has already been called. The skb is then freed at
exit without the abort message being built or queued, and nothing on
this path sets sk->sk_err or RCV_SHUTDOWN.
What terminal condition does a reader see after that?
tipc_wait_for_rcvmsg() leaves its loop only in these cases:
- sk_receive_queue is not empty
- RCV_SHUTDOWN is set
- the timeout expires
- a signal is pending
- sock_error() returns an error
It never checks for TIPC_DISCONNECTING. The socket is no longer
TIPC_ESTABLISHED, so tipc_sk_timeout() stops probing and nothing will
abort the connection later. A reader blocked in tipc_recvmsg() or
tipc_recvstream() with an infinite timeout could stay asleep forever.
tipc_poll() reports EPOLLIN | EPOLLRDNORM | EPOLLHUP for
TIPC_DISCONNECTING whether or not the receive queue is empty. An event
loop that polls and then calls a non-blocking recvmsg() would see
EPOLLIN, get -EAGAIN back from tipc_wait_for_rcvmsg(), and spin.
This needs a GFP_ATOMIC failure on a cloned error message, but the
result is a hung or spinning application.
I am not sure the success path gets the converted abort message onto
sk_receive_queue either. tipc_sk_filter_connect() returns false for
TIPC_DISCONNECTING. Still, this new exit leaves the socket with no
terminal state at all.
Could this branch set sk->sk_err, or RCV_SHUTDOWN in sk_shutdown,
before the unclone? Then readers would wake up whether or not the abort
message survives. Alternatively, could the unclone be done before the
state change? Then a failure would just drop the message, the socket
would stay TIPC_ESTABLISHED, and probing would eventually abort it.
[Severity: Low]
This is a pre-existing issue, not one introduced by this patch. This new
exit drops the abort message after the socket is already
TIPC_DISCONNECTING, and neither sk_err nor RCV_SHUTDOWN is set.
tipc_wait_for_rcvmsg() does not check for TIPC_DISCONNECTING. Can a
reader blocked with an infinite timeout be left with no terminal
condition?
The success path seems to end the same way, though. The converted abort
message goes to inputq, and tipc_sk_filter_connect() returns false for
TIPC_DISCONNECTING:
case TIPC_OPEN:
case TIPC_DISCONNECTING:
return false;
tipc_msg_reverse() then drops it because msg_errcode() is still set. So
the abort message did not reach sk_receive_queue before this patch
either, and the new exit gives the same outcome.
In the CONN_PROBE branch below, an unclone failure drops the probe and no
CONN_PROBE_REPLY is sent. That is the same outcome as the existing
allocation failure in tipc_msg_reverse():
if (mtyp == CONN_PROBE) {
if (skb_unclone(skb, GFP_ATOMIC) == -ENOMEM)
goto exit;
hdr = buf_msg(skb);
msg_set_type(hdr, CONN_PROBE_REPLY);
if (tipc_msg_reverse(onode, &skb, TIPC_OK))
tipc_msg_reverse() already copies the header into a freshly allocated
buffer. Could the CONN_PROBE_REPLY type be set on that copy instead, so
the unclone is not needed?
[ ... ]
> @@ -2223,6 +2230,9 @@ static bool tipc_sk_filter_connect(struct tipc_sock *tsk, struct sk_buff *skb,
> return true;
> /* Empty ACK-, - wake up sleeping connect() and drop */
> sk->sk_state_change(sk);
> + if (skb_unclone(skb, GFP_ATOMIC) == -ENOMEM)
> + break;
[Severity: Medium]
Does this break leave the connection half set up? tipc_sk_finish_conn()
has already run at this point. It sets TIPC_ESTABLISHED, arms the probe
timer, purges sk_write_queue and calls tipc_node_add_conn():
net/tipc/socket.c:tipc_sk_finish_conn() {
...
sk_reset_timer(sk, &sk->sk_timer, jiffies + CONN_PROBING_INTV);
tipc_set_sk_state(sk, TIPC_ESTABLISHED);
tipc_node_add_conn(net, peer_node, tsk->portid, peer_port);
...
}
The break lands in the abort tail. That tail was written for paths that
run before tipc_sk_finish_conn():
/* Abort connection setup attempt */
tipc_set_sk_state(sk, TIPC_DISCONNECTING);
sk->sk_err = ECONNREFUSED;
sk->sk_state_change(sk);
return true;
The tail never calls tipc_node_remove_conn(). For a remote peer,
tipc_node_add_conn() allocated a tipc_sock_conn with GFP_ATOMIC and linked
it on node->conn_sks. __tipc_shutdown() removes it only for
TIPC_CONNECTING and TIPC_ESTABLISHED, so a TIPC_DISCONNECTING socket
takes the default branch on close.
Does the tipc_sock_conn leak until node_lost_contact()?
connect() would also return -ECONNREFUSED even though the peer accepted.
The peer is not notified, so it stays half open until its probing gives
up.
The function now returns true here, so tipc_sk_filter_rcv() queues the
data-less ACK on sk_receive_queue and calls sk_data_ready(). Before this
patch the ACK was dropped, as the "wake up sleeping connect() and drop"
comment says.
Other ESTABLISHED to DISCONNECTING transitions call
tipc_node_remove_conn(). Examples are the error case under
TIPC_ESTABLISHED in this function, tipc_sk_conn_proto_rcv() and
tipc_sk_check_probing_state().
Could the unclone be done before tipc_sk_finish_conn()? Or, on failure,
could the ACK just be dropped without falling into the abort tail?
> + hdr = buf_msg(skb);
> msg_set_dest_droppable(hdr, 1);
> return false;
> }
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022011539.6882-1-tung.quang.nguyen%40est.tech
^ permalink raw reply [flat|nested] 3+ messages in thread