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

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.

quoted

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