RE: [PATCH net] tipc: fix bc_ackers underflow on duplicate GRP_ACK_MSG

[Tung Quang Nguyen]

>Subject: [PATCH net] tipc: fix bc_ackers underflow on duplicate GRP_ACK_MSG
>
>The GRP_ACK_MSG handler in tipc_group_proto_rcv() unconditionally
>decrements grp->bc_ackers on every inbound group ACK, even when the
>sending member has already acknowledged the current broadcast round.
>
>Because bc_ackers is a u16, a single duplicate ACK received after the legitimate
>set has drained the counter to zero wraps it to 65535.
>Once wrapped, tipc_group_bc_cong() permanently reports congestion,
>blocking all subsequent group broadcasts on the affected socket until the group
>is recreated.
>
>The member-removal path (tipc_group_delete_member) already handles this
>correctly: it only decrements bc_ackers when the counter is non-zero and the
>member still owes an ACK for the current broadcast round.
>
>Apply the same forward-progress guard to the GRP_ACK_MSG handler: only
>update m->bc_acked and decrement bc_ackers when the inbound ack value is
>strictly ahead of what has already been recorded for that member, and only
>decrement when bc_ackers is non-zero.
>
>Fixes: 75da2163dbb6 ("tipc: introduce communication groups")
>Cc: stable@xxxxxxxxxxxxxxx
>Signed-off-by: Oleh Konko <security@xxxxxxxxx>
>---
> net/tipc/group.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
>diff --git a/net/tipc/group.c b/net/tipc/group.c index
>e0e6227b433..41fa7bb3091 100644
>--- a/net/tipc/group.c
>+++ b/net/tipc/group.c
>@@ -745,7 +745,7 @@ void tipc_group_proto_rcv(struct tipc_group *grp, bool
>*usr_wakeup,
> 	u32 node = msg_orignode(hdr);
> 	u32 port = msg_origport(hdr);
> 	struct tipc_member *m, *pm;
>-	u16 remitted, in_flight;
>+	u16 remitted, in_flight, acked;
Variable declaration should be "Reverse X'mas tree" style.
>
> 	if (!grp)
> 		return;
>@@ -798,8 +798,13 @@ void tipc_group_proto_rcv(struct tipc_group *grp,
>bool *usr_wakeup,
> 	case GRP_ACK_MSG:
> 		if (!m)
> 			return;
>-		m->bc_acked = msg_grp_bc_acked(hdr);
>-		if (--grp->bc_ackers)
>+		acked = msg_grp_bc_acked(hdr);
>+		if (less(m->bc_acked, acked)) {
>+			m->bc_acked = acked;
>+			if (grp->bc_ackers)
>+				grp->bc_ackers--;
>+		}
This sanity check is not correct because subsequent statements are still executed in the case of receiving duplicate ACK. This patch should fix the issue:
diff --git a/net/tipc/group.c b/net/tipc/group.c
index e0e6227b433b..ed81bfe1830e 100644
--- a/net/tipc/group.c
+++ b/net/tipc/group.c
@@ -746,6 +746,7 @@ void tipc_group_proto_rcv(struct tipc_group *grp, bool *usr_wakeup,
        u32 port = msg_origport(hdr);
        struct tipc_member *m, *pm;
        u16 remitted, in_flight;
+       u16 acked;
 
        if (!grp)
                return;
@@ -798,7 +799,12 @@ void tipc_group_proto_rcv(struct tipc_group *grp, bool *usr_wakeup,
        case GRP_ACK_MSG:
                if (!m)
                        return;
-               m->bc_acked = msg_grp_bc_acked(hdr);
+
+               acked = msg_grp_bc_acked(hdr);
+               if (less_eq(acked, m->bc_acked))
+                       return;
+
+               m->bc_acked = acked;
                if (--grp->bc_ackers)
                        return;
                list_del_init(&m->small_win);
>+		if (grp->bc_ackers)
> 			return;
> 		list_del_init(&m->small_win);
> 		*m->group->open = true;
>--
>2.50.0
>