Re: [PATCH net] tipc: reject invalid and unexpected GRP_ACK_MSG to prevent bc_ackers underflow

[Paolo Abeni <pabeni@redhat.com>]

On 9/14/26 06:52, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0
> 
> - [Medium] Incomplete ACK accounting in tipc_group_proto_rcv()
>    (net/tipc/group.c:799-808): the new validation gates the decrement of…
> 
> Pre-existing issues:
> - [Medium] Pre-existing (not introduced or claimed by this patch): in
>    tipc_send_group_bcast() (net/tipc/socket.c:1131-1137) the packets…
> 
> --
> 
> tipc: reject invalid and unexpected GRP_ACK_MSG to prevent bc_ackers underflow
> 
> This tightens the GRP_ACK_MSG path in tipc_group_proto_rcv() so that the
> message is dropped when grp->bc_ackers is zero, and so that only an ACK
> whose value equals grp->bc_snd_nxt (and which is not a repeat of the
> member's own last ACK) is allowed to decrement grp->bc_ackers.
> 
>> diff --git a/net/tipc/group.c b/net/tipc/group.c
>> index 14e6732624e28..74f6d3dac0784 100644
>> --- a/net/tipc/group.c
>> +++ b/net/tipc/group.c
>> @@ -797,10 +797,10 @@ void tipc_group_proto_rcv(struct tipc_group *grp, bool *usr_wakeup,
>>   		tipc_group_open(m, usr_wakeup);
>>   		return;
>>   	case GRP_ACK_MSG:
>> -		if (!m)
>> +		if (!m || !grp->bc_ackers)
>>   			return;
>>   		acked = msg_grp_bc_acked(hdr);
>> -		if (less_eq(acked, m->bc_acked))
>> +		if (acked != grp->bc_snd_nxt || m->bc_acked == acked)
>>   			return;
>>   		m->bc_acked = acked;
>>   		if (--grp->bc_ackers)
> 
> [Severity: Medium]
> The new test gates the decrement on the sequence value and on per-member
> duplication.  Does it also need to check that the acking member was one of
> the members actually counted into grp->bc_ackers for the current round?
> 
> tipc_group_update_bc_members() counts and re-arms receivers only:
> 
> net/tipc/group.c:tipc_group_update_bc_members() {
> 	u16 prev = grp->bc_snd_nxt - 1;
> 	...
> 	for (n = rb_first(&grp->members); n; n = rb_next(n)) {
> 		m = container_of(n, struct tipc_member, tree_node);
> 		if (tipc_group_is_receiver(m)) {
> 			tipc_group_update_member(m, len);
> 			m->bc_acked = prev;
> 			ackers++;
> 		}
> 	}
> 
> 	/* Mark number of acknowledges to expect, if any */
> 	if (ack)
> 		grp->bc_ackers = ackers;
> 	grp->bc_snd_nxt++;
> }
> 
> A member in MBR_JOINING or MBR_LEAVING is therefore neither summed into
> ackers nor re-armed, so it keeps a stale m->bc_acked.  The same holds for a
> member created after the round was published, since
> tipc_group_create_member() sets:
> 
> 	m->bc_acked = grp->bc_snd_nxt - 1;
> 
> In both cases m->bc_acked differs from grp->bc_snd_nxt, so a GRP_ACK_MSG
> from that member passes both new conditions and still executes
> --grp->bc_ackers although it contributed no increment.  The lookup on this
> path, tipc_group_find_member(), applies no state filter.
> 
> Can that let grp->bc_ackers reach zero while a counted receiver still owes
> an ACK?  The number of accepted decrements per round looks bounded by
> grp->member_cnt rather than by the counted acker set.
> 
> If bc_ackers is cleared early, tipc_group_bc_cong() stops reporting
> congestion:
> 
> net/tipc/group.c:tipc_group_bc_cong() {
> 	/* If prev bcast was replicast, reject until all receivers have acked */
> 	if (grp->bc_ackers) {
> 		*grp->open = false;
> 		return true;
> 	}
> 
> so the sender releases the next group broadcast while the previous replicast
> round is still outstanding, which is the ordering guarantee from
> 2f487712b893, one of the Fixes tags on this patch.  The zero-transition
> branch also runs list_del_init(&m->small_win) and *m->group->open = true for
> the acking member, which need not be a receiver, and the genuine acker's
> later ACK is then swallowed by the new !grp->bc_ackers guard, so its
> small_win / tipc_group_update_member() bookkeeping never runs.
> 
> The commit message states that "each member can acknowledge at most once per
> round" - that part holds per member, but does it cover members that were
> never counted for the round?
> 
> Would a per-round marker set in tipc_group_update_bc_members(), or accepting
> only members whose m->bc_acked == grp->bc_snd_nxt - 2, close the remaining
> gap?
> 
> The u16 wrap to 65535 itself does look closed by the new !grp->bc_ackers
> guard, so what is left here is the premature clearing of the counter that
> item 2 of the commit message also mentions.

My reading of the above is that it's a slightly different problem, that 
could/should deserve a separate patch. WDYT?

/P