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

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

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()
grp->call
sk->sk_write_space() when tipc_group_filter_msg() switches the group
sk->from
closed to open. The aim is to unblock group broadcast senders when the last
outstanding acker leaves.

quoted

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?

Agree. Please rewrite the changelog.

quoted

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-

quoted

group_is_open is set.

Agree. Please rewrite the changelog.

quoted

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,

quoted

 	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?

Agree. Please address the incomplete check.

--
pw-bot: cr