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

[Tung Quang Nguyen]

>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.
>
>> 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.

>> 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.

Agree. Please rewrite the changelog.

>
>> 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?
>

Agree. Please address the incomplete check.

--
pw-bot: cr