netdev - RE: [PATCH v3 net-next] tipc: adjust tipc_nodeid2string() to check the length of the result

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <GV1P189MB1988AF3D7C3BC2F0F8DE2491C61EA@GV1P189MB1988.EURP189.PROD.OUTLOOK.COM>
Date: Fri, 26 Sep 2025 03:34:04 +0000
From: Tung Quang Nguyen <tung.quang.nguyen@....tech>
To: Dmitry Antipov <dmantipov@...dex.ru>
CC: Jon Maloy <jmaloy@...hat.com>, "David S . Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, "tipc-discussion@...ts.sourceforge.net"
	<tipc-discussion@...ts.sourceforge.net>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, Simon Horman <horms@...nel.org>
Subject: RE: [PATCH v3 net-next] tipc: adjust tipc_nodeid2string() to check
 the length of the result

>Subject: [PATCH v3 net-next] tipc: adjust tipc_nodeid2string() to check the
>length of the result
>
>Since the value returned by 'tipc_nodeid2string()' is not used, the function may
>be adjusted to check the length of the result against NODE_ID_LEN, which is
>helpful to drop a few calls to 'strlen()' and simplify 'tipc_link_create()' and
>'tipc_link_bc_create()'. Compile tested only.
>
>Signed-off-by: Dmitry Antipov <dmantipov@...dex.ru>
>---
>v3: convert to check against NODE_ID_LEN (Simon Horman)
>v2: adjusted to target net-next (Tung Quang Nguyen)
>---
> net/tipc/addr.c | 6 +++---
> net/tipc/addr.h | 2 +-
> net/tipc/link.c | 9 +++------
> 3 files changed, 7 insertions(+), 10 deletions(-)
>
>diff --git a/net/tipc/addr.c b/net/tipc/addr.c index
>fd0796269eed..90e47add376e 100644
>--- a/net/tipc/addr.c
>+++ b/net/tipc/addr.c
>@@ -79,7 +79,7 @@ void tipc_set_node_addr(struct net *net, u32 addr)
> 	pr_info("Node number set to %u\n", addr);  }
>
>-char *tipc_nodeid2string(char *str, u8 *id)
>+bool tipc_nodeid2string(char *str, u8 *id)
> {
> 	int i;
> 	u8 c;
>@@ -109,7 +109,7 @@ char *tipc_nodeid2string(char *str, u8 *id)
> 	if (i == NODE_ID_LEN) {
> 		memcpy(str, id, NODE_ID_LEN);
> 		str[NODE_ID_LEN] = 0;
>-		return str;
>+		return false;
> 	}
>
> 	/* Translate to hex string */
>@@ -120,5 +120,5 @@ char *tipc_nodeid2string(char *str, u8 *id)
> 	for (i = NODE_ID_STR_LEN - 2; str[i] == '0'; i--)
> 		str[i] = 0;
>
>-	return str;
>+	return i + 1 > NODE_ID_LEN;
No, you should not do this.
Firstly, this makes the function look vague. tipc_nodeid2string() converts node id to string and its return value if needed should be converted string length as you did in V2 patch.
Secondly, this adds unnecessary overhead in case we use node id that is less than 16 characters in length (For example, tipc_node_create() calls tipc_nodeid2string() without caring its return value).
So, just let callers of tipc_nodeid2string() decide what value they want to compare to.
I think you can improve (in V4) by replacing 16 with NODE_ID_LEN to make it more descriptive (in tipc_link_create() and tipc_link_bc_create()).

> }
>diff --git a/net/tipc/addr.h b/net/tipc/addr.h index 93f82398283d..5e4fc27fe329
>100644
>--- a/net/tipc/addr.h
>+++ b/net/tipc/addr.h
>@@ -130,6 +130,6 @@ static inline int in_own_node(struct net *net, u32 addr)
>bool tipc_in_scope(bool legacy_format, u32 domain, u32 addr);  void
>tipc_set_node_id(struct net *net, u8 *id);  void tipc_set_node_addr(struct net
>*net, u32 addr); -char *tipc_nodeid2string(char *str, u8 *id);
>+bool tipc_nodeid2string(char *str, u8 *id);
>
> #endif
>diff --git a/net/tipc/link.c b/net/tipc/link.c index 3ee44d731700..93181b1d8898
>100644
>--- a/net/tipc/link.c
>+++ b/net/tipc/link.c
>@@ -495,11 +495,9 @@ bool tipc_link_create(struct net *net, char *if_name,
>int bearer_id,
>
> 	/* Set link name for unicast links only */
> 	if (peer_id) {
>-		tipc_nodeid2string(self_str, tipc_own_id(net));
>-		if (strlen(self_str) > 16)
>+		if (tipc_nodeid2string(self_str, tipc_own_id(net)))
> 			sprintf(self_str, "%x", self);
>-		tipc_nodeid2string(peer_str, peer_id);
>-		if (strlen(peer_str) > 16)
>+		if (tipc_nodeid2string(peer_str, peer_id))
> 			sprintf(peer_str, "%x", peer);
> 	}
> 	/* Peer i/f name will be completed by reset/activate message */ @@ -
>570,8 +568,7 @@ bool tipc_link_bc_create(struct net *net, u32 ownnode, u32
>peer, u8 *peer_id,
> 	if (peer_id) {
> 		char peer_str[NODE_ID_STR_LEN] = {0,};
>
>-		tipc_nodeid2string(peer_str, peer_id);
>-		if (strlen(peer_str) > 16)
>+		if (tipc_nodeid2string(peer_str, peer_id))
> 			sprintf(peer_str, "%x", peer);
> 		/* Broadcast receiver link name: "broadcast-link:<peer>" */
> 		snprintf(l->name, sizeof(l->name), "%s:%s", tipc_bclink_name,
>--
>2.51.0