RE: [PATCH net v2] tipc: fix broadcast sender hang on last acker departure

[Tung Quang Nguyen <tung.quang.nguyen@est.tech>]

>Subject: Re: [PATCH net v2] tipc: fix broadcast sender hang 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 · Low: 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 patch adds m->bc_pending so that only members counted for
>  the current replicast can reduce grp->bc_ackers, but it checks the…
>- [Low] The commit message gives the wrong reason why blocked writers
>  stay asleep, and it overstates when the blocking sendmsg() hang…
>
>--
>
>tipc: fix broadcast sender hang on last acker departure
>
>This patch restores *grp->open when the last member that owes a replicast ack
>is deleted. It also calls sk->sk_write_space() from tipc_sk_filter_rcv() when the
>group becomes writable again. A new per-member bc_pending flag limits the
>bc_ackers decrement in the delete path to members that were counted for the
>current replicast.
>
>> TIPC group broadcast senders are blocked while a replicast still has
>> outstanding member acks: poll(EPOLLOUT) only fires when
>> tsk->group_is_open, and writers keep re-sleeping on every unrelated
>> data-ready wakeup because tipc_group_bc_cong() keeps seeing *grp->open
>> == false (the default sk_sndtimeo is MAX_SCHEDULE_TIMEOUT, so there is
>> no bounded send timeout either).
>
>[Severity: Low]
>Is this description of the mechanism accurate? tipc_group_bc_cong() only
>writes *grp->open and never reads it:
>
>net/tipc/group.c:tipc_group_bc_cong() {
>	if (grp->bc_ackers) {
>		*grp->open = false;
>		return true;
>	}
>	if (list_empty(&grp->small_win))
>		return false;
>	...
>}
>
>Its return value depends only on grp->bc_ackers and the head of
>grp->small_win.
>
>> When the last outstanding acker departs before acking,
>> tipc_group_delete_member() decrements grp->bc_ackers but never
>> restores *grp->open, and the blocked senders are never woken.
>
>The old delete path already brought bc_ackers to 0 when the last owing
>member left. That member has bc_acked == S-1 and bc_snd_nxt == S+1, so the
>less() test matches.
>
>tipc_wait_for_cond() waits with woken_wake_function, which ignores the poll
>key, and tipc_data_ready() wakes every sleeper on sk_wq.
>
>So with group member events enabled, the sk_data_ready() after the leave
>event is delivered already woke the writer in tipc_send_group_bcast().
>tipc_group_bc_cong() then returned false and sendmsg() went on, returning
>EHOSTUNREACH if the group was now empty.
>
>The poll(EPOLLOUT) hang does look real in every case, because tipc_poll() only
>sets EPOLLOUT when tsk->group_is_open is true.
>
>The blocking sendmsg() hang seems to happen only when nothing wakes the
>writer after the deletion. One example is group member events being disabled.
>In that case TIPC_GRP_MEMBER_EVT is freed with deliver = false and
>sk_data_ready() is never called.
>
>The code change itself looks correct. Since this is tagged for stable, could the
>changelog describe the actual wakeup path and say when the
>sendmsg() hang occurs?

Please rewrite the changelog.

>
>> diff --git a/net/tipc/group.c b/net/tipc/group.c index
>> 74f6d3dac0784..e9c77f12c9819 100644
>> --- a/net/tipc/group.c
>> +++ b/net/tipc/group.c
>
>[ ... ]
>
>> @@ -340,9 +341,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 owed an ack for the current replicast and it
>> +	 * was the last one, restore the open state so blocked broadcast
>> +	 * senders can proceed.
>> +	 */
>> +	if (grp->bc_ackers && m->bc_pending && !--grp->bc_ackers)
>> +		*grp->open = true;
>>
>>  	list_del_init(&m->list);
>>  	list_del_init(&m->small_win);
>> @@ -406,6 +410,8 @@ void tipc_group_update_bc_members(struct
>tipc_group *grp, int len, bool ack)
>>  			m->bc_acked = prev;
>>  			ackers++;
>>  		}
>> +		if (ack)
>> +			m->bc_pending = tipc_group_is_receiver(m);
>>  	}
>>
>>  	/* Mark number of acknowledges to expect, if any */ @@ -803,6
>+809,7
>> @@ void tipc_group_proto_rcv(struct tipc_group *grp, bool *usr_wakeup,
>>  		if (acked != grp->bc_snd_nxt || m->bc_acked == acked)
>>  			return;
>>  		m->bc_acked = acked;
>> +		m->bc_pending = false;
>>  		if (--grp->bc_ackers)
>>  			return;
>>  		list_del_init(&m->small_win);
>
>[Severity: Medium]
>Should the GRP_ACK_MSG handler also check m->bc_pending before it
>decrements grp->bc_ackers?
>
>The new test in tipc_group_delete_member() assumes bc_ackers equals the
>number of members with bc_pending set.
>
>This handler still decrements bc_ackers for any member that passes the acked
>== grp->bc_snd_nxt and m->bc_acked != acked checks.
>
>Two kinds of member can pass those checks without having been counted:
>
>- Members that were MBR_JOINING or MBR_LEAVING when
>  tipc_group_update_bc_members() ran keep a stale bc_acked and get
>  bc_pending = false.
>- Members created after the replicast have bc_pending = false and
>  bc_acked = bc_snd_nxt - 1 from tipc_group_create_member().
>
>For both, a GRP_ACK_MSG carrying acked == bc_snd_nxt passes every check
>here.
>
>For example, with bc_ackers == 2 for counted members A and B:
>
>  ack from uncounted member X     bc_ackers 2 -> 1
>  B acks, or B is deleted         bc_ackers 1 -> 0, *grp->open = true
>
>A still has bc_pending == true and has never acked.
>
>Isn't this the case the commit message gives as the reason for adding
>bc_pending, where a non-counted member "could silently void a still-
>outstanding ack from a real acker"?
>
>A cluster peer can trigger this. It sends GRP_JOIN_MSG, which makes
>tipc_group_proto_rcv() create an MBR_JOINING member for any port. It then
>sends GRP_ACK_MSG with acked == bc_snd_nxt. I could not find a benign
>protocol sequence that ends up here.
>
>Would something like this close the gap?
>
>	if (acked != grp->bc_snd_nxt || !m->bc_pending)
>		return;
>

Looks right. Please use this approach (Perhaps, bc_acked field of tipc_member needs to be removed).