RE: [PATCH net] tipc: don't count unjoined publications as receivers

[Tung Quang Nguyen]

>Subject: Re: [PATCH net] tipc: don't count unjoined publications as receivers
>
>Thank you for your contribution! Sashiko AI review found 5 potential
>issue(s) to consider.
>
>Critical: 0 · High: 2 · Medium: 0 · Low: 3
>
>If you believe none of the issues are legitimate, please reply to each issue of
>Medium severity or higher and explain why the AI is wrong.
>If any of them are legitimate and you plan to respin, please let patchwork
>know by including "pw-bot: cr" as a separate line at the end of your reply (one
>such reply per series is enough).
>
>- [High] The patch takes MBR_PUBLISHED out of tipc_group_is_receiver(),
>  which removes the gate that held a new joiner's sends until each…
>- [High] Changing tipc_group_is_receiver() also changes what
>  tipc_group_find_dest() and tipc_group_cong() return, which affects…
>- [Low] The new branch in tipc_group_open() opens the group whenever the
>  member is MBR_JOINED, is not on small_win, and the group is closed.
>- [Low] The changelog reads like a cover letter or bug report and is not
>  a commit log.
>- [Low] tipc_group_create_member() still adds every member's node to
>  grp->dests regardless of state (tipc_nlist_add(&grp->dests, m->node),…
>
>--
>
>tipc: don't count unjoined publications as receivers
>
>This commit removes MBR_PUBLISHED members from
>tipc_group_is_receiver(), so publications from sockets that never join the group
>no longer hold group senders on the small window list. It also lets
>tipc_group_open() reopen the group when a member reaches MBR_JOINED,
>even if that member was not on small_win.
>
>> Hello,
>>
>> A local process with no capabilities can block sends from a TIPC
>
>[ ... ]
>
>> HEAD: commit c03c1a14507dfaf8b856c2608f9ac216021fc222 ("Linux 7.3")
>> git tree:       upstream Linux, local branch master
>> source version: Linux 7.3.0 (v7.3)
>> compiler:       gcc 11.4.0, GNU ld 2.38
>> QEMU:           6.2.0
>>
>> Raw reproduction files:
>
>[ ... ]
>
>[Severity: Low]
>This isn't a bug, but the changelog reads like a cover letter or a bug report. The
>greeting, gist URLs, toolchain and QEMU versions, local branch name, HEAD
>reference and build/run instructions would all end up in git history. Could they
>move below the --- line?
>
>The changelog also leaves out several behaviour changes in the patch:
>senders are no longer gated on the JOIN handshake, unicast to a member still
>in the handshake now returns -EHOSTUNREACH, and anycast behaves
>differently.
>
>> diff --git a/net/tipc/group.c b/net/tipc/group.c index
>> 0c6a762000b02..c55daebc6cb38 100644
>> --- a/net/tipc/group.c
>> +++ b/net/tipc/group.c
>> @@ -105,9 +105,12 @@ static void tipc_group_proto_xmit(struct
>> tipc_group *grp, struct tipc_member *m,  static void
>> tipc_group_open(struct tipc_member *m, bool *wakeup)  {
>>  	*wakeup = false;
>> -	if (list_empty(&m->small_win))
>> -		return;
>> -	list_del_init(&m->small_win);
>> +	if (list_empty(&m->small_win)) {
>> +		if (m->state != MBR_JOINED || *m->group->open)
>> +			return;
>> +	} else {
>> +		list_del_init(&m->small_win);
>> +	}
>
>[Severity: Low]
>The new branch is aimed at the JOIN handshake transitions. However,
>tipc_group_open() is also called from the GRP_ADV_MSG, GRP_RECLAIM_MSG
>and GRP_LEAVE_MSG cases in tipc_group_proto_rcv(), for example:
>
>	case GRP_ADV_MSG:
>		if (!m)
>			return;
>		m->window += msg_adv_win(hdr);
>		tipc_group_open(m, usr_wakeup);
>		return;
>
>The group may be closed for an unrelated reason, such as pending replicast
>acks (grp->bc_ackers != 0) or congestion on another member.
>In that case, won't an ADV or RECLAIM from any JOINED member that is not on
>small_win now set *grp->open = true and *usr_wakeup = true?
>
>tipc_poll() reports EPOLLOUT based only on tsk->group_is_open. That would
>give a spurious POLLOUT followed by -EAGAIN on a nonblocking send, and
>spurious wakeups of blocked senders. The existing small_win branch already
>reopens without checking those conditions, but this change extends that to
>members that were never congested.
>
>The two departure paths also behave differently now. GRP_LEAVE_MSG calls
>tipc_group_open() while the member is still MBR_JOINED:
>
>		tipc_group_open(m, usr_wakeup);
>		tipc_group_decr_active(grp, m);
>		m->state = MBR_LEAVING;
>
>so the new branch can fire there. The TIPC_WITHDRAWN path in
>tipc_group_member_evt() sets MBR_LEAVING before calling
>tipc_group_open(), so it cannot.
>
>Should the new open be limited to the actual transition into MBR_JOINED?
>
>>  	*m->group->open = true;
>>  	*wakeup = true;
>>  }
>> @@ -145,7 +148,8 @@ u16 tipc_group_bc_snd_nxt(struct tipc_group *grp)
>>
>>  static bool tipc_group_is_receiver(struct tipc_member *m)  {
>> -	return m && m->state != MBR_JOINING && m->state != MBR_LEAVING;
>> +	return m && m->state != MBR_JOINING && m->state !=
>MBR_PUBLISHED &&
>> +	       m->state != MBR_LEAVING;
>
>[Severity: High]
>Does excluding MBR_PUBLISHED here also remove the gate that stopped a
>new joiner from sending before each existing member had processed its JOIN?
>
>Before this patch, tipc_group_join() did:
>
>	rbtree_postorder_for_each_entry_safe(m, tmp, tree, tree_node) {
>		tipc_group_proto_xmit(grp, m, GRP_JOIN_MSG, &xmitq);
>		tipc_group_update_member(m, 0);
>	}
>
>This put every PUBLISHED member (window 0) on grp->small_win, so
>tipc_group_bc_cong() blocked the sender until the peer's JOIN arrived.
>
>With this change, tipc_group_update_member() returns early for those
>members and small_win stays empty. tipc_group_bc_cong() now returns false
>right after setsockopt(TIPC_GROUP_JOIN).
>
>Take socket A joining while member B already exists. A's GRP_JOIN_MSG
>reaches B synchronously on the local node. tipc_group_proto_rcv() then
>creates A as MBR_JOINING with bc_rcv_nxt set to the syncpoint. A's
>TIPC_PUBLISHED topology event reaches B later, through the topsrv
>workqueue:
>
>tipc_topsrv_queue_evt()->queue_work()->tipc_topsrv_kern_evt()
>
>If A broadcasts before that worker runs, B sees this in
>tipc_group_filter_msg():
>
>	m = tipc_group_find_member(grp, node, port);
>	if (!tipc_group_is_sender(m))
>		goto drop;
>
>B drops the message without advancing bc_rcv_nxt. When the PUBLISHED
>event arrives, tipc_group_member_evt() moves A to MBR_JOINED but leaves
>bc_rcv_nxt unchanged.
>
>Every later message from A then hits this check. That includes unicasts,
>because tipc_send_group_msg() stamps bc_snd_nxt via
>msg_set_grp_bc_seqno():
>
>		if (more(msg_grp_bc_seqno(hdr), m->bc_rcv_nxt))
>			break;
>
>The message stays in m->deferredq. B never reads these messages, so it never
>sends GRP_ADV_MSG, and A's window for B eventually runs out.
>
>Can this leave A's broadcasts and unicasts to B blocked permanently?
>That is the symptom this patch sets out to fix, but here it is reachable by an
>ordinary join followed by an immediate send.
>
>There is also a side effect: tipc_group_update_bc_members() now skips
>members that A still sees as PUBLISHED. No window is charged and no ack is
>expected for them, even though the broadcast reaches their node through grp-
>>dests.
>
>[Severity: High]
>tipc_group_find_dest() and tipc_group_cong() both use
>tipc_group_is_receiver(). Doesn't this change unicast and anycast behaviour as
>well?
>
>For an MBR_PUBLISHED member, tipc_group_cong() now takes this path:
>
>	m = tipc_group_find_dest(grp, dnode, dport);
>	if (!tipc_group_is_receiver(m)) {
>		*mbr = NULL;
>		return false;
>	}
>
>Previously it returned true with *mbr set.
>
>For unicast, tipc_send_group_unicast() to a member still in the JOIN
>handshake now fails immediately instead of waiting for the JOIN:
>
>	if (unlikely(!mb))
>		return -EHOSTUNREACH;
>
>For anycast, tipc_send_group_anycast() was written for the old behaviour. The
>inner loop breaks on the first cong == false, and then:
>
>		/* Start over if destination was not in member list */
>		if (unlikely(!mbr))
>			continue;
>
>This uses up one of only three outer lookups (while (++lookups < 4)), and after
>the third one the send fails:
>
>	if (unlikely(lookups >= 4))
>		return -EHOSTUNREACH;
>
>tipc_nametbl_lookup_group() hands out candidates round-robin from
>sr->all_publ. It filters only by scope and the excluded port, not by
>group membership.
>
>Could an unprivileged process bind three or more ordinary (non-group)
>sockets to the group's type/instance and make group anycast fail with -
>EHOSTUNREACH, even when a joined, uncongested member is reachable?
>
>Before the patch, these phantom entries reported cong == true with mbr set,
>and the inner loop moved past them to a real member. So the problem
>described in the changelog seems to remain reachable through anycast, with
>the symptom changing from blocking to -EHOSTUNREACH.
>tipc_send_group_mcast() also still counts phantom publications in dstcnt.
>
>Would non-member publications need to be filtered out, rather than just
>classified differently?
>
>[Severity: Low]
>tipc_group_create_member() still adds every member's node to grp->dests,
>whatever its state:
>
>	tipc_nlist_add(&grp->dests, m->node);
>
>MBR_PUBLISHED members, though, are no longer receivers. Suppose the only
>peers matching the group are unjoined publications.
>tipc_send_group_bcast() then passes this check:
>
>	if (!dsts->local && !dsts->remote)
>		return -EHOSTUNREACH;
>
>tipc_group_bc_cong() returns false because small_win is empty. The broadcast
>goes out with zero accounted receivers, and the non-group socket drops it in
>tipc_sk_filter_rcv().
>
>Meanwhile tsk->group_is_open starts out false and only becomes true through
>a JOIN or a GRP_ACK, so tipc_poll() never reports EPOLLOUT:
>
>		if (tsk->group_is_open && !tsk->cong_link_cnt)
>			revents |= EPOLLOUT;
>
>Is it intended that sendmsg() succeeds while poll() says the socket is not
>writable? Also, should a send to a group with no joined members return
>success rather than -EHOSTUNREACH?
>
>>  }
>
>--
>Sashiko AI review · https://netdev-
>ai.bots.linux.dev/sashiko/#/patchset/20261107082422.900347-1-
>shawnq1%40163.com

---
pw-bot: cr