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

[Tung Quang Nguyen]

>Subject: [PATCH net v3] tipc: fix broadcast sender hang on last acker departure
>
>TIPC group broadcast senders are blocked while a replicast still has outstanding
>member acks: tipc_group_bc_cong() returns true as long as
>grp->bc_ackers is non-zero, and clears *grp->open, which is what
>tipc_poll() uses to report EPOLLOUT (via tsk->group_is_open).
>
>When the last outstanding acker departs before acking,
>tipc_group_delete_member() decrements grp->bc_ackers to zero but never
>restores *grp->open.  As a result poll(EPOLLOUT) never fires again.  A writer
>blocked in sendmsg() is only released if some other wakeup happens after the
>deletion: with TIPC_GROUP_MEMBER_EVTS the leave event delivery calls
>sk_data_ready() and wakes it, but without member events nothing does and it
>sleeps indefinitely, since the default send timeout is
>MAX_SCHEDULE_TIMEOUT.
>
>Commit 99cc2a62e07a ("tipc: reject invalid and unexpected GRP_ACK_MSG to
>prevent bc_ackers underflow") already calls this out in its changelog as a pre-
>existing issue.
>
>Restore the open state exactly on the 1->0 transition in the delete path,
>mirroring the GRP_ACK_MSG completion path, and invoke
>sk->sk_write_space() from tipc_sk_filter_rcv() when the group just
>became writable again.
>
>Also make the ack accounting precise.  The delete path used to test less(m-
>>bc_acked, grp->bc_snd_nxt - 1), which also matches members that were
>never counted for the current replicast (JOINING/LEAVING members keep a
>stale bc_acked), and the GRP_ACK_MSG handler accepted an ack from any
>member whose bc_acked differed from the expected value.  Either could zero
>bc_ackers on behalf of a member that owes nothing and silently void an ack
>still owed by a real acker.  Track whether a member owes an ack for the current
>replicast in a new per-member bc_pending flag, set when the member is
>counted and cleared when its ack arrives, and use it both to accept an ack and
>to decide on deletion.  bc_acked has no remaining user and is removed.
>Suggested by Tung Quang Nguyen.
>
>This issue was discovered by Tencent CodeBuddy Security.
>
>Cc: stable@xxxxxxxxxxxxxxx
>Fixes: 2f487712b893 ("tipc: guarantee that group broadcast doesn't bypass
>group unicast")
>Signed-off-by: Henry Martin <bsdhenrymartin@xxxxxxxxx>
>---
>
>v3: Correct the changelog: tipc_group_bc_cong() only writes *grp->open,
>    and a blocked sendmsg() hangs only when nothing wakes it after the
>    deletion (e.g. member events disabled); also require bc_pending
>    in the GRP_ACK_MSG handler so an uncounted member cannot decrement
>    bc_ackers, and drop the now unused bc_acked field.
>
>v2: Subject no longer claims a leak; add the reproduction; the delete
>    path now discriminates members that were actually counted for the
>    current replicast via a new per-member bc_pending flag instead
>    of comparing a possibly stale bc_acked (Tung Quang Nguyen).
>
>Reproduction (two QEMU guests, TIPC over UDP bearer, groups joined without
>member events; the acking peer withheld GRP_ACK_MSG via a test-only knob
>to emulate the lossy peer, the sender's kernel ran unmodified TIPC):
>
>    UNPATCHED (#88/#90, fix absent):
>      sender:  NO POLLOUT for 30s after member departed -> STALL
>      sender2: R2 NEVER returned in 32s (blocked writer not woken)
>    PATCHED (#89, v1 of this patch):
>      sender:  POLLOUT at t=18.5s, when the leave event was delivered
>      sender2: R2 returned at t=18.6s, EHOSTUNREACH on now-empty group
>
> net/tipc/group.c  | 20 +++++++++++---------  net/tipc/socket.c | 12
>+++++++++++-
> 2 files changed, 22 insertions(+), 10 deletions(-)
>
>diff --git a/net/tipc/group.c b/net/tipc/group.c index
>74f6d3dac0784..b76edd5001ffa 100644
>--- a/net/tipc/group.c
>+++ b/net/tipc/group.c
>@@ -74,7 +74,7 @@ struct tipc_member {
> 	u16 window;
> 	u16 bc_rcv_nxt;
> 	u16 bc_syncpt;
>-	u16 bc_acked;
>+	bool bc_pending;
> };
>
> struct tipc_group {
>@@ -316,7 +316,6 @@ static struct tipc_member
>*tipc_group_create_member(struct tipc_group *grp,
> 	m->node = node;
> 	m->port = port;
> 	m->instance = instance;
>-	m->bc_acked = grp->bc_snd_nxt - 1;
> 	ret = tipc_group_add_to_tree(grp, m);
> 	if (ret < 0) {
> 		kfree(m);
>@@ -340,9 +339,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);
>@@ -394,7 +396,6 @@ void tipc_group_update_member(struct tipc_member
>*m, int len)
>
> void tipc_group_update_bc_members(struct tipc_group *grp, int len, bool ack)
>{
>-	u16 prev = grp->bc_snd_nxt - 1;
> 	struct tipc_member *m;
> 	struct rb_node *n;
> 	u16 ackers = 0;
>@@ -403,9 +404,10 @@ void tipc_group_update_bc_members(struct
>tipc_group *grp, int len, bool ack)
> 		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++;
> 		}
>+		if (ack)
>+			m->bc_pending = tipc_group_is_receiver(m);
> 	}

Not sure if you read my comment for your v2:
" This needs to be done in a simple way:

@ -405,6 +405,7 @@ void tipc_group_update_bc_members(struct tipc_group *grp, int len, bool ack)
                        tipc_group_update_member(m, len);
                        m->bc_acked = prev;
                        ackers++;
+                       m->bc_pending = ack;
                }
        }
>
"

--
pw-bot: cr