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

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

[Eric Dumazet]

Commit 48a5fe38772b ("tipc: fix bc_ackers underflow on duplicate
GRP_ACK_MSG") rejected duplicate/stale ACKs in tipc_group_proto_rcv()
by returning early when less_eq(acked, m->bc_acked).

However, that check remains incomplete in two ways:

1. When grp->bc_ackers is zero (e.g. on a quiet group, when replicast
   ACKs were not requested, or after all expected members have already
   acknowledged), an unexpected GRP_ACK_MSG with acked > m->bc_acked
   passes less_eq() and unconditionally decrements grp->bc_ackers.
   Because bc_ackers is a u16, this wraps to 65535, causing
   tipc_group_bc_cong() to permanently report congestion and blocking
   all future group broadcasts on the socket.

2. During an active broadcast round (grp->bc_ackers > 0), the sender
   transmits packet S and advances grp->bc_snd_nxt to S + 1. Receivers
   increment their expected counter to S + 1 upon consuming packet S,
   so the only valid ACK value for the current round is strictly
   acked == grp->bc_snd_nxt.

   However, tipc_group_update_bc_members() initializes each member's
   m->bc_acked to prev = grp->bc_snd_nxt - 1 (S - 1 before increment).
   This leaves a 2-sequence gap (S - 1 to S + 1) in sequence space.
   An incoming ACK is therefore neither rejected as duplicate nor
   prevented from decrementing grp->bc_ackers if an unexpected or stale
   value (such as S) is received. A member sending acked = S followed
   by acked = S + 1 could decrement grp->bc_ackers twice in the same
   round, prematurely clearing bc_ackers or underflowing it.

Fix this by:
- Dropping GRP_ACK_MSG immediately if grp->bc_ackers is zero.
- Requiring acked == grp->bc_snd_nxt and rejecting duplicates where
  m->bc_acked == acked. Because replicast broadcast rounds are strictly
  sequential, only grp->bc_snd_nxt can be acknowledged, and each member
  can acknowledge at most once per round.

Note that a related pre-existing issue in tipc_group_delete_member()
(where grp->bc_ackers decrementing to zero upon member departure does
not restore *grp->open or trigger a socket wakeup) will be addressed
in a separate patch.

Fixes: 48a5fe38772b ("tipc: fix bc_ackers underflow on duplicate GRP_ACK_MSG")
Fixes: 2f487712b893 ("tipc: guarantee that group broadcast doesn't bypass group unicast")
Reported-by: James Burton <jamesburton@meta.com>
Cc: stable@vger.kernel.org
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
Cc: Oleh Konko <security@1seal.org>
Cc: Tung Nguyen <tung.quang.nguyen@est.tech>
Cc: Jon Maloy <jmaloy@redhat.com>
---
 net/tipc/group.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/tipc/group.c b/net/tipc/group.c
index 14e6732624e28edc20ff80163787bca3018cb2dd..74f6d3dac0784d5a54db3e2202ead3878d8df9e2 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)
-- 
2.55.0.1007.g17ff1f9808-goog

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

[netdev-bot+sashiko]

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.

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch - the
patch strictly improves the path - but the same permanent congestion is
still reachable from tipc_send_group_bcast() in net/tipc/socket.c, where the
packets are transmitted before the round state is published:

net/tipc/socket.c:tipc_send_group_bcast() {
	/* Send message */
	rc = tipc_mcast_xmit(net, &pkts, method, dsts, &tsk->cong_link_cnt);
	if (unlikely(rc))
		return rc;

	/* Update broadcast sequence number and send windows */
	tipc_group_update_bc_members(tsk->group, blks, ack);
}

The error return skips the publication entirely, and the failure can happen
after partial delivery:

net/tipc/bcast.c:tipc_rcast_xmit() {
	list_for_each_entry_safe(dst, tmp, &dests->list, list) {
		dnode = dst->node;
		if (!tipc_msg_pskb_copy(dnode, pkts, &_pkts))
			return -ENOMEM;

		if (tipc_node_xmit(net, &_pkts, dnode, selector) == -ELINKCONG)
			(*cong_link_cnt)++;
	}
}

Peers that already received seqno S advance m->bc_rcv_nxt to S + 1 while the
sender's grp->bc_snd_nxt stays at S.  The next broadcast reuses S, and those
peers drop it at the head of tipc_group_filter_msg():

	if (less(msg_grp_bc_seqno(hdr), m->bc_rcv_nxt))
		goto drop;

which is before ack = msg_grp_bc_ack_req(hdr) is evaluated, so they can never
emit the ACK the sender is waiting for and grp->bc_ackers never returns to
zero.  tipc_group_bc_cong() then keeps *grp->open false for the lifetime of
that socket.

Before this patch the same desynchronization additionally wrapped
grp->bc_ackers through the unguarded --grp->bc_ackers, so this change does
remove that part.

Should the round state be published atomically with the transmission, or
rolled back when tipc_mcast_xmit() fails after partial delivery?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913044233.193927-1-edumazet%40google.com