[PATCH] tipc: fix broadcast sender hang and leak on last acker departure

[PATCH] tipc: fix broadcast sender hang and leak on last acker departure

[Henry Martin]

TIPC group broadcast senders are blocked in poll()/write() until all
members have acked the replicast.  When a departing member held the
last outstanding ack, tipc_group_delete_member() decrements
grp->bc_ackers to zero but never restores *grp->open nor wakes the
blocked senders; write() then sleeps until the send timeout and
poll() is never woken at all.  Commit 99cc2a62e07a ("tipc: reject
invalid and unexpected GRP_ACK_MSG to prevent bc_ackers underflow")
noted this as a pre-existing issue.

Restore the open state exactly on the 1->0 acker transition in the
delete path, mirroring the GRP_ACK_MSG completion path, and invoke
sk->sk_write_space() from tipc_sk_filter_rcv() when the group just
became writable again.

This vulnerability was discovered by Tencent CodeBuddy Security.

Cc: stable@vger.kernel.org
Fixes: 2f487712b893 ("tipc: guarantee that group broadcast doesn't bypass group unicast")
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
---
 net/tipc/group.c  |  9 ++++++---
 net/tipc/socket.c | 12 +++++++++++-
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/net/tipc/group.c b/net/tipc/group.c
index 74f6d3dac0784..e20eb612f82ce 100644
--- a/net/tipc/group.c
+++ b/net/tipc/group.c
@@ -340,9 +340,12 @@ static void tipc_group_delete_member(struct tipc_group *grp,
 	rb_erase(&m->tree_node, &grp->members);
 	grp->member_cnt--;

-	/* Check if we were waiting for replicast ack from this member */
-	if (grp->bc_ackers && less(m->bc_acked, grp->bc_snd_nxt - 1))
-		grp->bc_ackers--;
+	/* If this member held the last outstanding replicast ack,
+	 * restore the open state so blocked broadcast senders can proceed.
+	 */
+	if (grp->bc_ackers && less(m->bc_acked, grp->bc_snd_nxt - 1) &&
+	    !--grp->bc_ackers)
+		*grp->open = true;

 	list_del_init(&m->list);
 	list_del_init(&m->small_win);
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index d5d70eb230b56..65cc0c4964b31 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2356,8 +2356,18 @@ static void tipc_sk_filter_rcv(struct sock *sk, struct sk_buff *skb,
 	if (unlikely(!msg_isdata(hdr)))
 		tipc_sk_proto_rcv(sk, &inputq, xmitq);

-	if (unlikely(grp))
+	if (unlikely(grp)) {
+		bool was_open = tsk->group_is_open;
+
 		tipc_group_filter_msg(grp, &inputq, xmitq);
+		/* A departing member may have held the last outstanding
+		 * replicast ack, which can restore the open state inside
+		 * tipc_group_filter_msg(); wake up senders blocked in
+		 * poll()/write() if the group just became writable.
+		 */
+		if (!was_open && tsk->group_is_open)
+			sk->sk_write_space(sk);
+	}

 	if (unlikely(!grp) && mtyp == TIPC_MCAST_MSG)
 		tipc_mcast_filter_msg(net, &tsk->mc_method.deferredq, &inputq);
--
2.43.7

Re: [PATCH] tipc: fix broadcast sender hang and leak on last acker departure

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH] tipc: fix broadcast sender hang and leak on last acker departure

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 1 (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).

- [Medium] The new open-restore in tipc_group_delete_member()
  (net/tipc/group.c:346-348) depends on less(m->bc_acked,…
- [Low] The commit message does not match the patch.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but a node-down
  withdrawal event can get stuck behind a deferred unicast, so the…
- [Low] This problem wasn't introduced by this patch, but
  tsk->group_is_open (aliased as *grp->open) is written with plain…

--

tipc: fix broadcast sender hang and leak on last acker departure

This patch makes tipc_group_delete_member() restore *grp->open when
grp->bc_ackers drops from 1 to 0. It also makes tipc_sk_filter_rcv() call
sk->sk_write_space() when tipc_group_filter_msg() switches the group from
closed to open. The aim is to unblock group broadcast senders when the
last outstanding acker leaves.

> tipc: fix broadcast sender hang and leak on last acker departure

[Severity: Low]
Which leak does this patch fix? The diff only adds the reopen and the
wakeup. The kfree(m) and the bc_ackers decrement in
tipc_group_delete_member() were already there.

This patch carries Cc: stable and a Fixes: tag. Could "leak" be dropped
from the subject so it doesn't mislead backporters?

> blocked senders; write() then sleeps until the send timeout and
> poll() is never woken at all.

Is the write() part accurate? tipc_wait_for_cond() sleeps in wait_woken()
with woken_wake_function(), and that function doesn't filter on the poll
key:

net/tipc/socket.c:tipc_wait_for_cond() {
    ...
	add_wait_queue(sk_sleep(sk_), &wait_);
	release_sock(sk_);
	*(timeo_) = wait_woken(&wait_, TASK_INTERRUPTIBLE, *(timeo_));
    ...
}

So any tipc_data_ready() on the socket wakes the writer and re-runs
tipc_group_bc_cong(). One example is delivery of the TIPC_WITHDRAWN member
event when TIPC_GROUP_MEMBER_EVTS is set.

If no such wakeup comes, the default blocking sk_sndtimeo is
MAX_SCHEDULE_TIMEOUT, so there is no bounded send timeout either.

The poll() part looks correct, since tipc_poll() reports EPOLLOUT only
when tsk->group_is_open is set.

> diff --git a/net/tipc/group.c b/net/tipc/group.c
> index 74f6d3dac0784..e20eb612f82ce 100644
> --- a/net/tipc/group.c
> +++ b/net/tipc/group.c
> @@ -340,9 +340,12 @@ static void tipc_group_delete_member(struct tipc_group *grp,
>  	rb_erase(&m->tree_node, &grp->members);
>  	grp->member_cnt--;
>  
> -	/* Check if we were waiting for replicast ack from this member */
> -	if (grp->bc_ackers && less(m->bc_acked, grp->bc_snd_nxt - 1))
> -		grp->bc_ackers--;
> +	/* If this member held the last outstanding replicast ack,
> +	 * restore the open state so blocked broadcast senders can proceed.
> +	 */
> +	if (grp->bc_ackers && less(m->bc_acked, grp->bc_snd_nxt - 1) &&
> +	    !--grp->bc_ackers)
> +		*grp->open = true;

[Severity: Medium]
Can this reopen the group while a counted acker is still outstanding?

The less(m->bc_acked, grp->bc_snd_nxt - 1) check also matches members
that were never counted in bc_ackers. tipc_group_update_bc_members() only
counts a member, and refreshes its bc_acked, when tipc_group_is_receiver()
is true:

net/tipc/group.c:tipc_group_update_bc_members() {
    ...
		if (tipc_group_is_receiver(m)) {
			tipc_group_update_member(m, len);
			m->bc_acked = prev;
			ackers++;
		}
    ...
}

tipc_group_is_receiver() excludes MBR_JOINING and MBR_LEAVING. A member in
either state keeps a stale bc_acked.

For example, say members X and Y are both JOINED:

  - A non-ack broadcast at seqno 10 sets X.bc_acked = Y.bc_acked = 9 and
    bc_snd_nxt = 11.
  - X becomes MBR_LEAVING. This can happen when TIPC_WITHDRAWN arrives
    with the node still up and before GRP_LEAVE_MSG, or when the leave
    event is still waiting in m->deferredq.
  - The local socket sends a unicast. tipc_send_group_anycast()/unicast
    then sets method->rcast and method->mandatory, so the next broadcast
    goes out as an ack-requesting replicast at seqno 11. Only Y is
    counted: bc_ackers = 1, Y.bc_acked = 10, bc_snd_nxt = 12.
  - X's leave event is delivered, and tipc_group_filter_msg() calls
    tipc_group_delete_member(X).

At that point less(9, 11) is true. So --bc_ackers reaches 0 and *grp->open
is set to true, even though Y hasn't acked yet.

The new check in tipc_sk_filter_rcv() then sees this:

	if (!was_open && tsk->group_is_open)
		sk->sk_write_space(sk);

The writer blocked in tipc_send_group_bcast() wakes up and
tipc_group_bc_cong() returns false. The next broadcast can then go out,
possibly over the broadcast link, before Y has acked the replicast.

Doesn't that break the unicast/broadcast ordering that commit 2f487712b893
added the bc_ackers gate for? Y's later GRP_ACK_MSG is also dropped by the
!grp->bc_ackers check in tipc_group_proto_rcv().

The miscount itself predates this patch. Before it, a false zero only
took effect on the next send or the next unrelated wakeup. With this
patch it causes an immediate wakeup and EPOLLOUT. The new comment ("If
this member held the last outstanding replicast ack") also doesn't hold
in this case.

Would it be better to record whether the member was counted when the
replicast was sent, for example with a per-member pending-ack flag,
instead of comparing a possibly stale bc_acked?

[Severity: Medium]
This is a pre-existing issue and not one introduced by this patch. Can
the departure path that leads here get stuck behind a deferred unicast,
so that this reopen never runs?

A member's group unicast carries msg_grp_bc_seqno = N+1. It can arrive
before that member's broadcast N, which travels on a different link. The
unicast is then deferred in m->deferredq with m->bc_rcv_nxt == N.

If the peer node fails before broadcast N arrives, tipc_group_member_evt()
creates the withdrawal event with seqno m->bc_rcv_nxt:

net/tipc/group.c:tipc_group_member_evt() {
    ...
		if (!tipc_node_is_up(net, node))
			tipc_group_create_event(grp, m, TIPC_WITHDRAWN,
						m->bc_rcv_nxt, inputq);
    ...
}

tipc_group_sort_msg() only sorts BCAST/MCAST messages, so the event is
appended behind the deferred unicast:

	/* Unicasts are never bypassed, - always add to tail */
	__skb_queue_tail(defq, skb);

tipc_group_filter_msg() then peeks the unicast at the head of the queue
and stops:

		if (more(msg_grp_bc_seqno(hdr), m->bc_rcv_nxt))
			break;

Broadcast N is lost along with the node, so tipc_group_delete_member() is
never called for this member, and the stale member stays in the tree. If
that member was the last outstanding acker, bc_ackers stays non-zero and
group broadcasts on this socket stay blocked.

Should the node-down case be handled before the event is queued behind
messages that can no longer be delivered?

[Severity: Low]
This isn't a bug introduced by this patch, but tsk->group_is_open (which
*grp->open points to) is written with plain stores under the socket lock.
tipc_poll() reads it without the lock:

net/tipc/socket.c:tipc_poll() {
    ...
	case TIPC_OPEN:
		if (tsk->group_is_open && !tsk->cong_link_cnt)
			revents |= EPOLLOUT;
    ...
}

This new store is one more writer of the same kind as the ones in
tipc_group_create(), tipc_group_cong(), tipc_group_bc_cong(),
tipc_group_open() and the GRP_ACK_MSG handler. The new was_open read in
tipc_sk_filter_rcv() runs under the socket lock, so it is fine.

Could KCSAN report this as a data race? If so, READ_ONCE()/WRITE_ONCE()
annotations on group_is_open would cover all of these accesses.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009055601.2304158-1-bsdhenrymartin%40gmail.com