diff mbox series

[net-next,3/3] tipc: simplify handling of lookup scope during multicast message reception

Message ID 20210529151400.781539-4-jmaloy@redhat.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series tipc: some small cleanups | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 1 maintainers not CCed: kuba@kernel.org
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 99 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Jon Maloy May 29, 2021, 3:14 p.m. UTC 
From: Jon Maloy <jmaloy@redhat.com>

We introduce a new macro TIPC_ANY_SCOPE to make the handling of the
lookup scope value more comprehensible during multicast reception.

The (unchanged) rules go as follows:

1) Multicast messages sent from own node are delivered to all matching
   sockets on the own node, irrespective of their binding scope.

2) Multicast messages sent from other nodes arrive here because they
   have found TIPC_CLUSTER_SCOPE bindings emanating from this node.
   Those messages should be delivered to exactly those sockets, but not
   to local sockets bound with TIPC_NODE_SCOPE, since the latter
   obviously were not meant to be visible for those senders.

3) Group multicast/broadcast messages are delivered to the sockets with
   a binding scope matching exactly the lookup scope indicated in the
   message header, and nobody else.

Reviewed-by: Xin Long <lucien.xin@gmail.com>
Tested-by: Hoang Le <hoang.h.le@dektech.com.au>
Signed-off-by: Jon Maloy <jmaloy@redhat.com>
---
 net/tipc/name_table.c |  6 +++---
 net/tipc/name_table.h |  4 +++-
 net/tipc/socket.c     | 26 ++++++++++----------------
 3 files changed, 16 insertions(+), 20 deletions(-)

Comments

kernel test robot May 29, 2021, 5:48 p.m. UTC | #1 
Hi,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/jmaloy-redhat-com/tipc-some-small-cleanups/20210529-231533
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 015dbf5662fd689d581c0bc980711b073ca09a1a
config: x86_64-randconfig-a005-20210529 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project bc6799f2f79f0ae87e9f1ebf9d25ba799fbd25a9)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/5ba80be36645dc45d04e6fc15b27a538269f6b1e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review jmaloy-redhat-com/tipc-some-small-cleanups/20210529-231533
        git checkout 5ba80be36645dc45d04e6fc15b27a538269f6b1e
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/tipc/socket.c:1220:15: warning: implicit conversion from 'int' to 'signed char' changes value from 255 to -1 [-Wconstant-conversion]
                           ua.scope = TIPC_ANY_SCOPE;
                                    ~ ^~~~~~~~~~~~~~
   net/tipc/name_table.h:54:24: note: expanded from macro 'TIPC_ANY_SCOPE'
   #define TIPC_ANY_SCOPE 255
                          ^~~
   1 warning generated.


vim +1220 net/tipc/socket.c

  1183	
  1184	/**
  1185	 * tipc_sk_mcast_rcv - Deliver multicast messages to all destination sockets
  1186	 * @net: the associated network namespace
  1187	 * @arrvq: queue with arriving messages, to be cloned after destination lookup
  1188	 * @inputq: queue with cloned messages, delivered to socket after dest lookup
  1189	 *
  1190	 * Multi-threaded: parallel calls with reference to same queues may occur
  1191	 */
  1192	void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
  1193			       struct sk_buff_head *inputq)
  1194	{
  1195		u32 self = tipc_own_addr(net);
  1196		struct sk_buff *skb, *_skb;
  1197		u32 portid, onode;
  1198		struct sk_buff_head tmpq;
  1199		struct list_head dports;
  1200		struct tipc_msg *hdr;
  1201		struct tipc_uaddr ua;
  1202		int user, mtyp, hlen;
  1203	
  1204		__skb_queue_head_init(&tmpq);
  1205		INIT_LIST_HEAD(&dports);
  1206		ua.addrtype = TIPC_SERVICE_RANGE;
  1207	
  1208		/* tipc_skb_peek() increments the head skb's reference counter */
  1209		skb = tipc_skb_peek(arrvq, &inputq->lock);
  1210		for (; skb; skb = tipc_skb_peek(arrvq, &inputq->lock)) {
  1211			hdr = buf_msg(skb);
  1212			user = msg_user(hdr);
  1213			mtyp = msg_type(hdr);
  1214			hlen = skb_headroom(skb) + msg_hdr_sz(hdr);
  1215			onode = msg_orignode(hdr);
  1216			ua.sr.type = msg_nametype(hdr);
  1217			ua.sr.lower = msg_namelower(hdr);
  1218			ua.sr.upper = msg_nameupper(hdr);
  1219			if (onode == self)
> 1220				ua.scope = TIPC_ANY_SCOPE;
  1221			else
  1222				ua.scope = TIPC_CLUSTER_SCOPE;
  1223	
  1224			if (mtyp == TIPC_GRP_UCAST_MSG || user == GROUP_PROTOCOL) {
  1225				spin_lock_bh(&inputq->lock);
  1226				if (skb_peek(arrvq) == skb) {
  1227					__skb_dequeue(arrvq);
  1228					__skb_queue_tail(inputq, skb);
  1229				}
  1230				kfree_skb(skb);
  1231				spin_unlock_bh(&inputq->lock);
  1232				continue;
  1233			}
  1234	
  1235			/* Group messages require exact scope match */
  1236			if (msg_in_group(hdr)) {
  1237				ua.sr.lower = 0;
  1238				ua.sr.upper = ~0;
  1239				ua.scope = msg_lookup_scope(hdr);
  1240			}
  1241	
  1242			/* Create destination port list: */
  1243			tipc_nametbl_lookup_mcast_sockets(net, &ua, &dports);
  1244	
  1245			/* Clone message per destination */
  1246			while (tipc_dest_pop(&dports, NULL, &portid)) {
  1247				_skb = __pskb_copy(skb, hlen, GFP_ATOMIC);
  1248				if (_skb) {
  1249					msg_set_destport(buf_msg(_skb), portid);
  1250					__skb_queue_tail(&tmpq, _skb);
  1251					continue;
  1252				}
  1253				pr_warn("Failed to clone mcast rcv buffer\n");
  1254			}
  1255			/* Append clones to inputq only if skb is still head of arrvq */
  1256			spin_lock_bh(&inputq->lock);
  1257			if (skb_peek(arrvq) == skb) {
  1258				skb_queue_splice_tail_init(&tmpq, inputq);
  1259				/* Decrement the skb's refcnt */
  1260				kfree_skb(__skb_dequeue(arrvq));
  1261			}
  1262			spin_unlock_bh(&inputq->lock);
  1263			__skb_queue_purge(&tmpq);
  1264			kfree_skb(skb);
  1265		}
  1266		tipc_sk_rcv(net, inputq);
  1267	}
  1268	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
index fecab516bf41..01396dd1c899 100644
--- a/net/tipc/name_table.c
+++ b/net/tipc/name_table.c
@@ -673,12 +673,12 @@  bool tipc_nametbl_lookup_group(struct net *net, struct tipc_uaddr *ua,
  * Returns a list of local sockets
  */
 void tipc_nametbl_lookup_mcast_sockets(struct net *net, struct tipc_uaddr *ua,
-				       bool exact, struct list_head *dports)
+				       struct list_head *dports)
 {
 	struct service_range *sr;
 	struct tipc_service *sc;
 	struct publication *p;
-	u32 scope = ua->scope;
+	u8 scope = ua->scope;
 
 	rcu_read_lock();
 	sc = tipc_service_find(net, ua);
@@ -688,7 +688,7 @@  void tipc_nametbl_lookup_mcast_sockets(struct net *net, struct tipc_uaddr *ua,
 	spin_lock_bh(&sc->lock);
 	service_range_foreach_match(sr, sc, ua->sr.lower, ua->sr.upper) {
 		list_for_each_entry(p, &sr->local_publ, local_publ) {
-			if (p->scope == scope || (!exact && p->scope < scope))
+			if (scope == p->scope || scope == TIPC_ANY_SCOPE)
 				tipc_dest_push(dports, 0, p->sk.ref);
 		}
 	}
diff --git a/net/tipc/name_table.h b/net/tipc/name_table.h
index c7c9a3ddd420..148b0f640959 100644
--- a/net/tipc/name_table.h
+++ b/net/tipc/name_table.h
@@ -51,6 +51,8 @@  struct tipc_uaddr;
 #define TIPC_PUBL_SCOPE_NUM	(TIPC_NODE_SCOPE + 1)
 #define TIPC_NAMETBL_SIZE	1024	/* must be a power of 2 */
 
+#define TIPC_ANY_SCOPE 255
+
 /**
  * struct publication - info about a published service address or range
  * @sr: service range represented by this publication
@@ -113,7 +115,7 @@  int tipc_nl_name_table_dump(struct sk_buff *skb, struct netlink_callback *cb);
 bool tipc_nametbl_lookup_anycast(struct net *net, struct tipc_uaddr *ua,
 				 struct tipc_socket_addr *sk);
 void tipc_nametbl_lookup_mcast_sockets(struct net *net, struct tipc_uaddr *ua,
-				       bool exact, struct list_head *dports);
+				       struct list_head *dports);
 void tipc_nametbl_lookup_mcast_nodes(struct net *net, struct tipc_uaddr *ua,
 				     struct tipc_nlist *nodes);
 bool tipc_nametbl_lookup_group(struct net *net, struct tipc_uaddr *ua,
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index c635fd27fb38..575a0238deb2 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1200,12 +1200,12 @@  void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
 	struct tipc_msg *hdr;
 	struct tipc_uaddr ua;
 	int user, mtyp, hlen;
-	bool exact;
 
 	__skb_queue_head_init(&tmpq);
 	INIT_LIST_HEAD(&dports);
 	ua.addrtype = TIPC_SERVICE_RANGE;
 
+	/* tipc_skb_peek() increments the head skb's reference counter */
 	skb = tipc_skb_peek(arrvq, &inputq->lock);
 	for (; skb; skb = tipc_skb_peek(arrvq, &inputq->lock)) {
 		hdr = buf_msg(skb);
@@ -1214,6 +1214,12 @@  void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
 		hlen = skb_headroom(skb) + msg_hdr_sz(hdr);
 		onode = msg_orignode(hdr);
 		ua.sr.type = msg_nametype(hdr);
+		ua.sr.lower = msg_namelower(hdr);
+		ua.sr.upper = msg_nameupper(hdr);
+		if (onode == self)
+			ua.scope = TIPC_ANY_SCOPE;
+		else
+			ua.scope = TIPC_CLUSTER_SCOPE;
 
 		if (mtyp == TIPC_GRP_UCAST_MSG || user == GROUP_PROTOCOL) {
 			spin_lock_bh(&inputq->lock);
@@ -1231,20 +1237,10 @@  void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
 			ua.sr.lower = 0;
 			ua.sr.upper = ~0;
 			ua.scope = msg_lookup_scope(hdr);
-			exact = true;
-		} else {
-			/* TIPC_NODE_SCOPE means "any scope" in this context */
-			if (onode == self)
-				ua.scope = TIPC_NODE_SCOPE;
-			else
-				ua.scope = TIPC_CLUSTER_SCOPE;
-			exact = false;
-			ua.sr.lower = msg_namelower(hdr);
-			ua.sr.upper = msg_nameupper(hdr);
 		}
 
 		/* Create destination port list: */
-		tipc_nametbl_lookup_mcast_sockets(net, &ua, exact, &dports);
+		tipc_nametbl_lookup_mcast_sockets(net, &ua, &dports);
 
 		/* Clone message per destination */
 		while (tipc_dest_pop(&dports, NULL, &portid)) {
@@ -1256,13 +1252,11 @@  void tipc_sk_mcast_rcv(struct net *net, struct sk_buff_head *arrvq,
 			}
 			pr_warn("Failed to clone mcast rcv buffer\n");
 		}
-		/* Append to inputq if not already done by other thread */
+		/* Append clones to inputq only if skb is still head of arrvq */
 		spin_lock_bh(&inputq->lock);
 		if (skb_peek(arrvq) == skb) {
 			skb_queue_splice_tail_init(&tmpq, inputq);
-			/* Decrease the skb's refcnt as increasing in the
-			 * function tipc_skb_peek
-			 */
+			/* Decrement the skb's refcnt */
 			kfree_skb(__skb_dequeue(arrvq));
 		}
 		spin_unlock_bh(&inputq->lock);