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

[netdev-bot+sashiko@kernel.org]

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