tipc-discussion — User questions & problem reports, project news, developer discussion

[tipc-discussion] [PATCH net-next] tipc: replace open-code bearer rcu_dereference access in bearer.c

[Xin L. <luc...@gm...>]

Replace these open-code bearer rcu_dereference access with bearer_get(),
like other places in bearer.c. While at it, also use tipc_net() instead
of net_generic(net, tipc_net_id) to get "tn" in bearer.c.

Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/bearer.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 114140c49108..1d5d3677bdaf 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -176,7 +176,7 @@ static int bearer_name_validate(const char *name,
  */
 struct tipc_bearer *tipc_bearer_find(struct net *net, const char *name)
 {
-	struct tipc_net *tn = net_generic(net, tipc_net_id);
+	struct tipc_net *tn = tipc_net(net);
 	struct tipc_bearer *b;
 	u32 i;
 
@@ -211,11 +211,10 @@ int tipc_bearer_get_name(struct net *net, char *name, u32 bearer_id)
 
 void tipc_bearer_add_dest(struct net *net, u32 bearer_id, u32 dest)
 {
-	struct tipc_net *tn = net_generic(net, tipc_net_id);
 	struct tipc_bearer *b;
 
 	rcu_read_lock();
-	b = rcu_dereference(tn->bearer_list[bearer_id]);
+	b = bearer_get(net, bearer_id);
 	if (b)
 		tipc_disc_add_dest(b->disc);
 	rcu_read_unlock();
@@ -223,11 +222,10 @@ void tipc_bearer_add_dest(struct net *net, u32 bearer_id, u32 dest)
 
 void tipc_bearer_remove_dest(struct net *net, u32 bearer_id, u32 dest)
 {
-	struct tipc_net *tn = net_generic(net, tipc_net_id);
 	struct tipc_bearer *b;
 
 	rcu_read_lock();
-	b = rcu_dereference(tn->bearer_list[bearer_id]);
+	b = bearer_get(net, bearer_id);
 	if (b)
 		tipc_disc_remove_dest(b->disc);
 	rcu_read_unlock();
@@ -534,7 +532,7 @@ int tipc_bearer_mtu(struct net *net, u32 bearer_id)
 	struct tipc_bearer *b;
 
 	rcu_read_lock();
-	b = rcu_dereference(tipc_net(net)->bearer_list[bearer_id]);
+	b = bearer_get(net, bearer_id);
 	if (b)
 		mtu = b->mtu;
 	rcu_read_unlock();
@@ -745,7 +743,7 @@ void tipc_bearer_cleanup(void)
 
 void tipc_bearer_stop(struct net *net)
 {
-	struct tipc_net *tn = net_generic(net, tipc_net_id);
+	struct tipc_net *tn = tipc_net(net);
 	struct tipc_bearer *b;
 	u32 i;
 
@@ -881,7 +879,7 @@ int tipc_nl_bearer_dump(struct sk_buff *skb, struct netlink_callback *cb)
 	struct tipc_bearer *bearer;
 	struct tipc_nl_msg msg;
 	struct net *net = sock_net(skb->sk);
-	struct tipc_net *tn = net_generic(net, tipc_net_id);
+	struct tipc_net *tn = tipc_net(net);
 
 	if (i == MAX_BEARERS)
 		return 0;
-- 
2.39.1

[tipc-discussion] Passing SCM_RIGHTS/SCM_CREDENTIALS

[Duzan, G. D <Gar...@fi...>]

   Just out of curiosity, has anyone looked into supporting passing descriptors and credentials over node-scoped TIPC sockets? I have a use case where they would be useful, but the simple experiments that I've tried indicate that at least descriptor passing isn't working as it would for AF_UNIX.

   Thanks.

Gary Duzan
IT Architect Senior
GT.M Core Team

T:  +1.484.302.3226
E:  gar...@fi...
FIS | Advancing the way the world pays, banks and invests™

The information contained in this message is proprietary and/or confidential. If you are not the intended recipient, please: (i) delete the message and all copies; (ii) do not disclose, distribute or use the message in any manner; and (iii) notify the sender immediately. In addition, please be aware that any message addressed to our domain is subject to archiving and review by persons other than the intended recipient. Thank you.

Re: [tipc-discussion] [PATCH net-next] tipc: delete tipc_mtu_bad from tipc_udp_enable

[Tung Q. N. <tun...@de...>]

>Subject: [PATCH net-next] tipc: delete tipc_mtu_bad from tipc_udp_enable
>
>Since commit a4dfa72d0acd ("tipc: set default MTU for UDP media"), it's
>been no longer using dev->mtu for b->mtu, and the issue described in
>commit 3de81b758853 ("tipc: check minimum bearer MTU") doesn't exist
>in UDP bearer any more.
>
>Besides, dev->mtu can still be changed to a too small mtu after the UDP
>bearer is created even with tipc_mtu_bad() check in tipc_udp_enable().
>Note that NETDEV_CHANGEMTU event processing in tipc_l2_device_event()
>doesn't really work for UDP bearer.
>
>So this patch deletes the unnecessary tipc_mtu_bad from tipc_udp_enable.
>
>Signed-off-by: Xin Long <luc...@gm...>
>---
> net/tipc/bearer.c    | 4 ++--
> net/tipc/bearer.h    | 4 ++--
> net/tipc/udp_media.c | 4 ----
> 3 files changed, 4 insertions(+), 8 deletions(-)
>
>diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
>index 53881406e200..114140c49108 100644
>--- a/net/tipc/bearer.c
>+++ b/net/tipc/bearer.c
>@@ -431,7 +431,7 @@ int tipc_enable_l2_media(struct net *net, struct tipc_bearer *b,
> 	dev = dev_get_by_name(net, dev_name);
> 	if (!dev)
> 		return -ENODEV;
>-	if (tipc_mtu_bad(dev, 0)) {
>+	if (tipc_mtu_bad(dev)) {
> 		dev_put(dev);
> 		return -EINVAL;
> 	}
>@@ -708,7 +708,7 @@ static int tipc_l2_device_event(struct notifier_block *nb, unsigned long evt,
> 		test_and_set_bit_lock(0, &b->up);
> 		break;
> 	case NETDEV_CHANGEMTU:
>-		if (tipc_mtu_bad(dev, 0)) {
>+		if (tipc_mtu_bad(dev)) {
> 			bearer_disable(net, b);
> 			break;
> 		}
>diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
>index bd0cc5c287ef..1ee60649bd17 100644
>--- a/net/tipc/bearer.h
>+++ b/net/tipc/bearer.h
>@@ -257,9 +257,9 @@ static inline void tipc_loopback_trace(struct net *net,
> }
>
> /* check if device MTU is too low for tipc headers */
>-static inline bool tipc_mtu_bad(struct net_device *dev, unsigned int reserve)
>+static inline bool tipc_mtu_bad(struct net_device *dev)
> {
>-	if (dev->mtu >= TIPC_MIN_BEARER_MTU + reserve)
>+	if (dev->mtu >= TIPC_MIN_BEARER_MTU)
> 		return false;
> 	netdev_warn(dev, "MTU too low for tipc bearer\n");
> 	return true;
>diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
>index 0a85244fd618..926232557e77 100644
>--- a/net/tipc/udp_media.c
>+++ b/net/tipc/udp_media.c
>@@ -739,10 +739,6 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b,
> 		udp_conf.use_udp_checksums = false;
> 		ub->ifindex = dev->ifindex;
> 		b->encap_hlen = sizeof(struct iphdr) + sizeof(struct udphdr);
>-		if (tipc_mtu_bad(dev, b->encap_hlen)) {
>-			err = -EINVAL;
>-			goto err;
>-		}
> 		b->mtu = b->media->mtu;
> #if IS_ENABLED(CONFIG_IPV6)
> 	} else if (local.proto == htons(ETH_P_IPV6)) {
>--
>2.39.1
Reviewed-by: Tung Nguyen <tun...@de...>

[tipc-discussion] [PATCH net-next] tipc: delete tipc_mtu_bad from tipc_udp_enable

[Xin L. <luc...@gm...>]

Since commit a4dfa72d0acd ("tipc: set default MTU for UDP media"), it's
been no longer using dev->mtu for b->mtu, and the issue described in
commit 3de81b758853 ("tipc: check minimum bearer MTU") doesn't exist
in UDP bearer any more.

Besides, dev->mtu can still be changed to a too small mtu after the UDP
bearer is created even with tipc_mtu_bad() check in tipc_udp_enable().
Note that NETDEV_CHANGEMTU event processing in tipc_l2_device_event()
doesn't really work for UDP bearer.

So this patch deletes the unnecessary tipc_mtu_bad from tipc_udp_enable.

Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/bearer.c    | 4 ++--
 net/tipc/bearer.h    | 4 ++--
 net/tipc/udp_media.c | 4 ----
 3 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 53881406e200..114140c49108 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -431,7 +431,7 @@ int tipc_enable_l2_media(struct net *net, struct tipc_bearer *b,
 	dev = dev_get_by_name(net, dev_name);
 	if (!dev)
 		return -ENODEV;
-	if (tipc_mtu_bad(dev, 0)) {
+	if (tipc_mtu_bad(dev)) {
 		dev_put(dev);
 		return -EINVAL;
 	}
@@ -708,7 +708,7 @@ static int tipc_l2_device_event(struct notifier_block *nb, unsigned long evt,
 		test_and_set_bit_lock(0, &b->up);
 		break;
 	case NETDEV_CHANGEMTU:
-		if (tipc_mtu_bad(dev, 0)) {
+		if (tipc_mtu_bad(dev)) {
 			bearer_disable(net, b);
 			break;
 		}
diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
index bd0cc5c287ef..1ee60649bd17 100644
--- a/net/tipc/bearer.h
+++ b/net/tipc/bearer.h
@@ -257,9 +257,9 @@ static inline void tipc_loopback_trace(struct net *net,
 }
 
 /* check if device MTU is too low for tipc headers */
-static inline bool tipc_mtu_bad(struct net_device *dev, unsigned int reserve)
+static inline bool tipc_mtu_bad(struct net_device *dev)
 {
-	if (dev->mtu >= TIPC_MIN_BEARER_MTU + reserve)
+	if (dev->mtu >= TIPC_MIN_BEARER_MTU)
 		return false;
 	netdev_warn(dev, "MTU too low for tipc bearer\n");
 	return true;
diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index 0a85244fd618..926232557e77 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c
@@ -739,10 +739,6 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b,
 		udp_conf.use_udp_checksums = false;
 		ub->ifindex = dev->ifindex;
 		b->encap_hlen = sizeof(struct iphdr) + sizeof(struct udphdr);
-		if (tipc_mtu_bad(dev, b->encap_hlen)) {
-			err = -EINVAL;
-			goto err;
-		}
 		b->mtu = b->media->mtu;
 #if IS_ENABLED(CONFIG_IPV6)
 	} else if (local.proto == htons(ETH_P_IPV6)) {
-- 
2.39.1

Re: [tipc-discussion] Gacks on, callbacks suppressed messages

[Duzan, G. D <Gar...@fi...>]

   Thanks for your reply. Assuming that "handled well" implies that there are no unreported message drops, then I can focus elsewhere in my debugging.

Gary Duzan
IT Architect Senior
GT.M Core Team

T:  +1.484.302.3226
E:  gar...@fi...
FIS | Advancing the way the world pays, banks and invests™

________________________________
From: Tung Quang Nguyen <tun...@de...>
Sent: Thursday, May 18, 2023 9:10 PM
To: Duzan, Gary D <Gar...@fi...>; tip...@li... <tip...@li...>
Subject: RE: Gacks on, callbacks suppressed messages

>   When pushing TIPC a bit hard on some of our servers, I've recently had three of them issue clusters of "Gacks on" messages, with two
>of them also issuing "callbacks suppressed" messages. These had kernels from 5.10 to 6.2, so it doesn't seem like an issue with a
>particular kernel. Here is a sample dmesg fragment:
>
>[Wed May 17 17:36:15 2023] __tipc_build_gap_ack_blks: 5 callbacks suppressed
>[Wed May 17 17:36:15 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 291!
>[Wed May 17 17:36:15 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 307!
>[Wed May 17 17:36:15 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 227!
>[Wed May 17 17:36:15 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 243!
>[Wed May 17 17:36:15 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 259!
>[Wed May 17 17:36:16 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 355!
>[Wed May 17 17:36:16 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 259!
>[Wed May 17 17:36:18 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 323!
>[Wed May 17 17:36:18 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 515!
>[Wed May 17 17:36:18 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 531!
>[Wed May 17 17:36:21 2023] __tipc_build_gap_ack_blks: 121 callbacks suppressed
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 147!
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 163!
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 179!
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 355!
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 195!
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 195!
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 211!
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 403!
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 227!
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 243!
>
>Does this point to a need for tuning, or some bug?
No need for tuning, It is not a bug.
It indicates that your servers were under high load (a lot of message disorders or message losses).
The servers have used the maximum number of Selective ACK blocks (64) to deal with this situation and
everything was handled well.
The information contained in this message is proprietary and/or confidential. If you are not the intended recipient, please: (i) delete the message and all copies; (ii) do not disclose, distribute or use the message in any manner; and (iii) notify the sender immediately. In addition, please be aware that any message addressed to our domain is subject to archiving and review by persons other than the intended recipient. Thank you.

Re: [tipc-discussion] Gacks on, callbacks suppressed messages

[Tung Q. N. <tun...@de...>]

>   When pushing TIPC a bit hard on some of our servers, I've recently had three of them issue clusters of "Gacks on" messages, with two
>of them also issuing "callbacks suppressed" messages. These had kernels from 5.10 to 6.2, so it doesn't seem like an issue with a
>particular kernel. Here is a sample dmesg fragment:
>
>[Wed May 17 17:36:15 2023] __tipc_build_gap_ack_blks: 5 callbacks suppressed
>[Wed May 17 17:36:15 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 291!
>[Wed May 17 17:36:15 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 307!
>[Wed May 17 17:36:15 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 227!
>[Wed May 17 17:36:15 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 243!
>[Wed May 17 17:36:15 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 259!
>[Wed May 17 17:36:16 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 355!
>[Wed May 17 17:36:16 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 259!
>[Wed May 17 17:36:18 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 323!
>[Wed May 17 17:36:18 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 515!
>[Wed May 17 17:36:18 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 531!
>[Wed May 17 17:36:21 2023] __tipc_build_gap_ack_blks: 121 callbacks suppressed
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 147!
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 163!
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 179!
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 355!
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 195!
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 195!
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 211!
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 403!
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 227!
>[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 243!
>
>Does this point to a need for tuning, or some bug?
No need for tuning, It is not a bug.
It indicates that your servers were under high load (a lot of message disorders or message losses).
The servers have used the maximum number of Selective ACK blocks (64) to deal with this situation and
everything was handled well.

[tipc-discussion] Gacks on, callbacks suppressed messages

[Duzan, G. D <Gar...@fi...>]

   When pushing TIPC a bit hard on some of our servers, I've recently had three of them issue clusters of "Gacks on" messages, with two of them also issuing "callbacks suppressed" messages. These had kernels from 5.10 to 6.2, so it doesn't seem like an issue with a particular kernel. Here is a sample dmesg fragment:

[Wed May 17 17:36:15 2023] __tipc_build_gap_ack_blks: 5 callbacks suppressed
[Wed May 17 17:36:15 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 291!
[Wed May 17 17:36:15 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 307!
[Wed May 17 17:36:15 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 227!
[Wed May 17 17:36:15 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 243!
[Wed May 17 17:36:15 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 259!
[Wed May 17 17:36:16 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 355!
[Wed May 17 17:36:16 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 259!
[Wed May 17 17:36:18 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 323!
[Wed May 17 17:36:18 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 515!
[Wed May 17 17:36:18 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 531!
[Wed May 17 17:36:21 2023] __tipc_build_gap_ack_blks: 121 callbacks suppressed
[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 147!
[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 163!
[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 179!
[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 355!
[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 195!
[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 195!
[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 211!
[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4d75f5:eno1: 64, ql: 403!
[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 227!
[Wed May 17 17:36:21 2023] tipc: Gacks on 1866da4d3dd9:eth0-1866da4db43e:eno1: 64, ql: 243!

Does this point to a need for tuning, or some bug?

   Thanks.

Gary Duzan
IT Architect Senior
GT.M Core Team

T:  +1.484.302.3226
E:  gar...@fi...
FIS | Advancing the way the world pays, banks and invests™


The information contained in this message is proprietary and/or confidential. If you are not the intended recipient, please: (i) delete the message and all copies; (ii) do not disclose, distribute or use the message in any manner; and (iii) notify the sender immediately. In addition, please be aware that any message addressed to our domain is subject to archiving and review by persons other than the intended recipient. Thank you.

Re: [tipc-discussion] TIPC socket ( SOCK_SEQPACKET) cleanup issue

[Duzan, G. D <Gar...@fi...>]

   If the program is forking off processes, perhaps the child processes aren't closing the socket file descriptor. Using fcntl() to set FD_CLOEXEC on the descriptor in the parent may help.


Gary Duzan

IT Architect Senior

GT.M Core Team



T:  +1.484.302.3226

E:  gar...@fi...

FIS | Advancing the way the world pays, banks and invests™


________________________________
From: Tung Quang Nguyen via tipc-discussion <tip...@li...>
Sent: Monday, May 15, 2023 6:43 AM
To: prakash bisht <ps1...@gm...>
Cc: Xin Long <lx...@re...>; tip...@li... <tip...@li...>
Subject: Re: [tipc-discussion] TIPC socket ( SOCK_SEQPACKET) cleanup issue

Hi Tung,
Can you please share your thoughts.
I have responded to your queries in my previous email. Please let me know if you need any additional information from my end.

Thanks and regards,
Prakash
[Prakash ]  We are using iproute2 version-4.20.0-2+deb10u1 on amd64 platform. Our use case is very simple. We are creating/destroying a server socket based on some event using below code.
      // server socket creation
           int sd = socket(AF_TIPC, SOCK_SEQPACKET, 0);
      // Closing server socket
          close(sd);

      After closing the socket the file descriptor is freed but the tipc socket is still present in "tipc socket list" output.
      We have multiple applications in our system which are using tipc sockets. But we see this behaviour(stale tipc socket) only in one application.
      The only difference which I can notice is that this particular application  is spawning "strace" to monitor some other application. I am not sure but it looks like somehow running strace is affecting tipc socket
       cleanup.
>>> Perhaps you need to provide detailed reproduction steps like these:
1/ Output of “tipc socket list” before cleanup (to determine how many sockets are existing)
2/ strace command with detailed input parameters
3/ Do the cleanup
4/ Output of “tipc socket list” after cleanup

_______________________________________________
tipc-discussion mailing list
tip...@li...
https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.sourceforge.net%2Flists%2Flistinfo%2Ftipc-discussion&data=05%7C01%7Cgary.duzan%40fisglobal.com%7Ce548993f9552411d1b0608db55314e07%7Ce3ff91d834c84b15a0b418910a6ac575%7C0%7C0%7C638197442480641340%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=13RgGXQpTsC%2BmNp28zM6Xf442mA8Xyhr0%2BqWiiIJoXM%3D&reserved=0<https://lists.sourceforge.net/lists/listinfo/tipc-discussion>
The information contained in this message is proprietary and/or confidential. If you are not the intended recipient, please: (i) delete the message and all copies; (ii) do not disclose, distribute or use the message in any manner; and (iii) notify the sender immediately. In addition, please be aware that any message addressed to our domain is subject to archiving and review by persons other than the intended recipient. Thank you.

Re: [tipc-discussion] TIPC socket ( SOCK_SEQPACKET) cleanup issue

[Tung Q. N. <tun...@de...>]

Hi Tung,
Can you please share your thoughts.
I have responded to your queries in my previous email. Please let me know if you need any additional information from my end.

Thanks and regards,
Prakash
[Prakash ]  We are using iproute2 version-4.20.0-2+deb10u1 on amd64 platform. Our use case is very simple. We are creating/destroying a server socket based on some event using below code.
      // server socket creation
           int sd = socket(AF_TIPC, SOCK_SEQPACKET, 0);
      // Closing server socket
          close(sd);

      After closing the socket the file descriptor is freed but the tipc socket is still present in "tipc socket list" output.
      We have multiple applications in our system which are using tipc sockets. But we see this behaviour(stale tipc socket) only in one application.
      The only difference which I can notice is that this particular application  is spawning "strace" to monitor some other application. I am not sure but it looks like somehow running strace is affecting tipc socket
       cleanup.
>>> Perhaps you need to provide detailed reproduction steps like these:
1/ Output of “tipc socket list” before cleanup (to determine how many sockets are existing)
2/ strace command with detailed input parameters
3/ Do the cleanup
4/ Output of “tipc socket list” after cleanup

Re: [tipc-discussion] TIPC socket ( SOCK_SEQPACKET) cleanup issue

[prakash b. <ps1...@gm...>]

Hi Tung,
Can you please share your thoughts.
I have responded to your queries in my previous email. Please let me know
if you need any additional information from my end.

Thanks and regards,
Prakash

On Tue, Apr 25, 2023 at 2:43 PM prakash bisht <ps1...@gm...> wrote:

> Thanks Tung.
> Please find my response inline.
>
>
> Thanks and regards,
> Prakash
>
> On Tue, Apr 18, 2023 at 7:04 AM Tung Quang Nguyen <
> tun...@de...> wrote:
>
>>
>>
>>
>>
>> *From:* prakash bisht <ps1...@gm...>
>> *Sent:* Monday, April 17, 2023 11:20 AM
>> *To:* tip...@li...; Xin Long <lx...@re...>;
>> Tung Quang Nguyen <tun...@de...>; jm...@re...
>> *Subject:* Fwd: TIPC socket ( SOCK_SEQPACKET) cleanup issue
>>
>>
>>
>> Hi John/Xin,Tung,
>>
>>
>>
>> Any thoughts on my previous email? We are using tipc for our product for
>> quite a while and started facing this issue recently in a specific scenario
>> we launch strace to monitor another process.
>>
>> Also is there any way to deny creation of another tipc socket with the
>> same tipc address ?  In our case applications need a unique tipc server
>> socket.
>>
>> >>> There is no limitation for creating many sockets binding to the same
>> tipc address. Why you need “a unique tipc server socket” ? Can you provide
>> your code to demonstrate your use case ?
>>
>
>
>> *[Prakash]* We have services which are unique in our system and not
>> meant for load balancing. Each service is uniquely represented by a tipc
>> service address(type,instance).  What we are looking for is that if someone
>> by mistake creates a second Service using the same address(type,instance)
>> then the service creation fail and throw an error. This is just to avoid a
>> silent failure.
>>
>
>
>
>>
>>
>> Thanks and Regards,
>>
>> Prakash
>>
>> ---------- Forwarded message ---------
>> From: *prakash bisht* <ps1...@gm...>
>> Date: Mon, Apr 3, 2023 at 4:33 PM
>> Subject: TIPC socket ( SOCK_SEQPACKET) cleanup issue
>> To: <tip...@li...>
>>
>>
>>
>> Hi all,
>>
>>
>>
>> I am facing an issue while closing the TIPC server socket. In certain
>> scenarios, even after closing the server socket fd the ‘tipc socket list’
>> is still showing it as alive.
>>
>> >>> What is you iproute2 version ? Can you provide your code to
>> demonstrate your use case ?
>>
>
>
>> *[Prakash ]*  We are using iproute2 version-4.20.0-2+deb10u1 on amd64
>> platform. Our use case is very simple. We are creating/destroying a server
>> socket based on some event using below code.
>>
>       // server socket creation
>            int sd = socket(AF_TIPC, SOCK_SEQPACKET, 0);
>       // Closing server socket
>           close(sd);
>
>       After closing the socket the file descriptor is freed but the tipc
> socket is still present in "tipc socket list" output.
>       We have multiple applications in our system which are using tipc
> sockets. But we see this behaviour(stale tipc socket) only in one
> application.
>       The only difference which I can notice is that this particular
> application  is spawning "strace" to monitor some other application. I am
> not sure but it looks like somehow running strace is affecting tipc socket
>
>        cleanup.
>
>
>
>> I am sure that the fd has been closed as the next socket creation request
>> gets the same fd from linux. Even when the process exits, the stale socket
>> entry is still present in the ‘tipc socket list’ and it vanishes only after
>> rebooting the system.
>>
>>  Kernel version : 4.19.81
>>
>> Socket type :      SOCK_SEQPACKET
>>
>>
>>
>> Also, is there any way of finding out whether a tipc socket belongs to
>> which linux process ?
>>
>> >>> There is no command to know which tipc socket belongs to which Linux
>> process. But you can use function getsockname() to print out the port id
>> after creating a socket. Then you know the created socket belongs to the
>> calling process.
>>
> [
>
>>  *[Prakash]* Thanks.  getsockname() should server the purpose here.
>>
>
>
>> Would appreciate any help.
>>
>>
>>
>> Thanks,
>>
>> Prakash
>>
>

Re: [tipc-discussion] [PATCHv3 net 0/3] tipc: fix the mtu update in link mtu negotiation

[Tung Q. N. <tun...@de...>]

>Subject: [PATCHv3 net 0/3] tipc: fix the mtu update in link mtu negotiation
>
>This patchset fixes a crash caused by a too small MTU carried in the
>activate msg. Note that as such malicious packet does not exist in
>the normal env, the fix won't break any application
>
>The 1st patch introduces a function to calculate the minimum MTU for
>the bearer, and the 2nd patch fixes the crash with this helper. While
>at it, the 3rd patch fixes the udp bearer mtu update by netlink with
>this helper.
>
>Xin Long (3):
>  tipc: add tipc_bearer_min_mtu to calculate min mtu
>  tipc: do not update mtu if msg_max is too small in mtu negotiation
>  tipc: check the bearer min mtu properly when setting it by netlink
>
> net/tipc/bearer.c    | 17 +++++++++++++++--
> net/tipc/bearer.h    |  3 +++
> net/tipc/link.c      |  9 ++++++---
> net/tipc/udp_media.c |  5 +++--
> 4 files changed, 27 insertions(+), 7 deletions(-)
>
>--
>2.39.1
For the whole series:
Reviewed-by: Tung Nguyen <tun...@de...>

[tipc-discussion] [PATCHv3 net 3/3] tipc: check the bearer min mtu properly when setting it by netlink

[Xin L. <luc...@gm...>]

Checking the bearer min mtu with tipc_udp_mtu_bad() only works for
IPv4 UDP bearer, and IPv6 UDP bearer has a different value for the
min mtu. This patch checks with encap_hlen + TIPC_MIN_BEARER_MTU
for min mtu, which works for both IPv4 and IPv6 UDP bearer.

Note that tipc_udp_mtu_bad() is still used to check media min mtu
in __tipc_nl_media_set(), as m->mtu currently is only used by the
IPv4 UDP bearer as its default mtu value.

Fixes: 682cd3cf946b ("tipc: confgiure and apply UDP bearer MTU on running links")
Signed-off-by: Xin Long <luc...@gm...>
Acked-by: Jon Maloy <jm...@re...>
---
 net/tipc/bearer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 0e9a29e1536b..53881406e200 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -1151,8 +1151,8 @@ int __tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
 				return -EINVAL;
 			}
 #ifdef CONFIG_TIPC_MEDIA_UDP
-			if (tipc_udp_mtu_bad(nla_get_u32
-					     (props[TIPC_NLA_PROP_MTU]))) {
+			if (nla_get_u32(props[TIPC_NLA_PROP_MTU]) <
+			    b->encap_hlen + TIPC_MIN_BEARER_MTU) {
 				NL_SET_ERR_MSG(info->extack,
 					       "MTU value is out-of-range");
 				return -EINVAL;
-- 
2.39.1

[tipc-discussion] [PATCHv3 net 1/3] tipc: add tipc_bearer_min_mtu to calculate min mtu

[Xin L. <luc...@gm...>]

As different media may requires different min mtu, and even the
same media with different net family requires different min mtu,
add tipc_bearer_min_mtu() to calculate min mtu accordingly.

This API will be used to check the new mtu when doing the link
mtu negotiation in the next patch.

Signed-off-by: Xin Long <luc...@gm...>
Acked-by: Jon Maloy <jm...@re...>
---
v2:
- use bearer_get() to avoid the open code.
v3:
- move the change history under ---, as Tung suggested.
---
 net/tipc/bearer.c    | 13 +++++++++++++
 net/tipc/bearer.h    |  3 +++
 net/tipc/udp_media.c |  5 +++--
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 35cac7733fd3..0e9a29e1536b 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -541,6 +541,19 @@ int tipc_bearer_mtu(struct net *net, u32 bearer_id)
 	return mtu;
 }
 
+int tipc_bearer_min_mtu(struct net *net, u32 bearer_id)
+{
+	int mtu = TIPC_MIN_BEARER_MTU;
+	struct tipc_bearer *b;
+
+	rcu_read_lock();
+	b = bearer_get(net, bearer_id);
+	if (b)
+		mtu += b->encap_hlen;
+	rcu_read_unlock();
+	return mtu;
+}
+
 /* tipc_bearer_xmit_skb - sends buffer to destination over bearer
  */
 void tipc_bearer_xmit_skb(struct net *net, u32 bearer_id,
diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
index 490ad6e5f7a3..bd0cc5c287ef 100644
--- a/net/tipc/bearer.h
+++ b/net/tipc/bearer.h
@@ -146,6 +146,7 @@ struct tipc_media {
  * @identity: array index of this bearer within TIPC bearer array
  * @disc: ptr to link setup request
  * @net_plane: network plane ('A' through 'H') currently associated with bearer
+ * @encap_hlen: encap headers length
  * @up: bearer up flag (bit 0)
  * @refcnt: tipc_bearer reference counter
  *
@@ -170,6 +171,7 @@ struct tipc_bearer {
 	u32 identity;
 	struct tipc_discoverer *disc;
 	char net_plane;
+	u16 encap_hlen;
 	unsigned long up;
 	refcount_t refcnt;
 };
@@ -232,6 +234,7 @@ int tipc_bearer_setup(void);
 void tipc_bearer_cleanup(void);
 void tipc_bearer_stop(struct net *net);
 int tipc_bearer_mtu(struct net *net, u32 bearer_id);
+int tipc_bearer_min_mtu(struct net *net, u32 bearer_id);
 bool tipc_bearer_bcast_support(struct net *net, u32 bearer_id);
 void tipc_bearer_xmit_skb(struct net *net, u32 bearer_id,
 			  struct sk_buff *skb,
diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index c2bb818704c8..0a85244fd618 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c
@@ -738,8 +738,8 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b,
 			udp_conf.local_ip.s_addr = local.ipv4.s_addr;
 		udp_conf.use_udp_checksums = false;
 		ub->ifindex = dev->ifindex;
-		if (tipc_mtu_bad(dev, sizeof(struct iphdr) +
-				      sizeof(struct udphdr))) {
+		b->encap_hlen = sizeof(struct iphdr) + sizeof(struct udphdr);
+		if (tipc_mtu_bad(dev, b->encap_hlen)) {
 			err = -EINVAL;
 			goto err;
 		}
@@ -760,6 +760,7 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b,
 		else
 			udp_conf.local_ip6 = local.ipv6;
 		ub->ifindex = dev->ifindex;
+		b->encap_hlen = sizeof(struct ipv6hdr) + sizeof(struct udphdr);
 		b->mtu = 1280;
 #endif
 	} else {
-- 
2.39.1

[tipc-discussion] [PATCHv3 net 0/3] tipc: fix the mtu update in link mtu negotiation

[Xin L. <luc...@gm...>]

This patchset fixes a crash caused by a too small MTU carried in the
activate msg. Note that as such malicious packet does not exist in
the normal env, the fix won't break any application

The 1st patch introduces a function to calculate the minimum MTU for
the bearer, and the 2nd patch fixes the crash with this helper. While
at it, the 3rd patch fixes the udp bearer mtu update by netlink with
this helper.

Xin Long (3):
  tipc: add tipc_bearer_min_mtu to calculate min mtu
  tipc: do not update mtu if msg_max is too small in mtu negotiation
  tipc: check the bearer min mtu properly when setting it by netlink

 net/tipc/bearer.c    | 17 +++++++++++++++--
 net/tipc/bearer.h    |  3 +++
 net/tipc/link.c      |  9 ++++++---
 net/tipc/udp_media.c |  5 +++--
 4 files changed, 27 insertions(+), 7 deletions(-)

-- 
2.39.1

[tipc-discussion] [PATCHv3 net 2/3] tipc: do not update mtu if msg_max is too small in mtu negotiation

[Xin L. <luc...@gm...>]

When doing link mtu negotiation, a malicious peer may send Activate msg
with a very small mtu, e.g. 4 in Shuang's testing, without checking for
the minimum mtu, l->mtu will be set to 4 in tipc_link_proto_rcv(), then
n->links[bearer_id].mtu is set to 4294967228, which is a overflow of
'4 - INT_H_SIZE - EMSG_OVERHEAD' in tipc_link_mss().

With tipc_link.mtu = 4, tipc_link_xmit() kept printing the warning:

 tipc: Too large msg, purging xmit list 1 5 0 40 4!
 tipc: Too large msg, purging xmit list 1 15 0 60 4!

And with tipc_link_entry.mtu 4294967228, a huge skb was allocated in
named_distribute(), and when purging it in tipc_link_xmit(), a crash
was even caused:

  general protection fault, probably for non-canonical address 0x2100001011000dd: 0000 [#1] PREEMPT SMP PTI
  CPU: 0 PID: 0 Comm: swapper/0 Kdump: loaded Not tainted 6.3.0.neta #19
  RIP: 0010:kfree_skb_list_reason+0x7e/0x1f0
  Call Trace:
   <IRQ>
   skb_release_data+0xf9/0x1d0
   kfree_skb_reason+0x40/0x100
   tipc_link_xmit+0x57a/0x740 [tipc]
   tipc_node_xmit+0x16c/0x5c0 [tipc]
   tipc_named_node_up+0x27f/0x2c0 [tipc]
   tipc_node_write_unlock+0x149/0x170 [tipc]
   tipc_rcv+0x608/0x740 [tipc]
   tipc_udp_recv+0xdc/0x1f0 [tipc]
   udp_queue_rcv_one_skb+0x33e/0x620
   udp_unicast_rcv_skb.isra.72+0x75/0x90
   __udp4_lib_rcv+0x56d/0xc20
   ip_protocol_deliver_rcu+0x100/0x2d0

This patch fixes it by checking the new mtu against tipc_bearer_min_mtu(),
and not updating mtu if it is too small.

Fixes: ed193ece2649 ("tipc: simplify link mtu negotiation")
Reported-by: Shuang Li <sh...@re...>
Signed-off-by: Xin Long <luc...@gm...>
Acked-by: Jon Maloy <jm...@re...>
---
v2:
- do the msg_max check against the min MTU early, as Tung suggested.
v3:
- move the change history under ---, as Tung suggested.
---
 net/tipc/link.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index b3ce24823f50..2eff1c7949cb 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -2200,7 +2200,7 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
 	struct tipc_msg *hdr = buf_msg(skb);
 	struct tipc_gap_ack_blks *ga = NULL;
 	bool reply = msg_probe(hdr), retransmitted = false;
-	u32 dlen = msg_data_sz(hdr), glen = 0;
+	u32 dlen = msg_data_sz(hdr), glen = 0, msg_max;
 	u16 peers_snd_nxt =  msg_next_sent(hdr);
 	u16 peers_tol = msg_link_tolerance(hdr);
 	u16 peers_prio = msg_linkprio(hdr);
@@ -2239,6 +2239,9 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
 	switch (mtyp) {
 	case RESET_MSG:
 	case ACTIVATE_MSG:
+		msg_max = msg_max_pkt(hdr);
+		if (msg_max < tipc_bearer_min_mtu(l->net, l->bearer_id))
+			break;
 		/* Complete own link name with peer's interface name */
 		if_name =  strrchr(l->name, ':') + 1;
 		if (sizeof(l->name) - (if_name - l->name) <= TIPC_MAX_IF_NAME)
@@ -2283,8 +2286,8 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
 		l->peer_session = msg_session(hdr);
 		l->in_session = true;
 		l->peer_bearer_id = msg_bearer_id(hdr);
-		if (l->mtu > msg_max_pkt(hdr))
-			l->mtu = msg_max_pkt(hdr);
+		if (l->mtu > msg_max)
+			l->mtu = msg_max;
 		break;
 
 	case STATE_MSG:
-- 
2.39.1

Re: [tipc-discussion] [PATCHv2 net 0/3] tipc: fix the mtu update in link mtu negotiation

[Jon M. <jm...@re...>]

On 2023-05-02 18:13, Xin Long wrote:
> This patchset fixes a crash caused by a too small MTU carried in the
> activate msg. Note that as such malicious packet does not exist in
> the normal env, the fix won't break any application
>
> The 1st patch introduces a function to calculate the minimum MTU for
> the bearer, and the 2nd patch fixes the crash with this helper. While
> at it, the 3rd patch fixes the udp bearer mtu update by netlink with
> this helper.
>
> Xin Long (3):
>    tipc: add tipc_bearer_min_mtu to calculate min mtu
>    tipc: do not update mtu if msg_max is too small in mtu negotiation
>    tipc: check the bearer min mtu properly when setting it by netlink
>
>   net/tipc/bearer.c    | 17 +++++++++++++++--
>   net/tipc/bearer.h    |  3 +++
>   net/tipc/link.c      |  9 ++++++---
>   net/tipc/udp_media.c |  5 +++--
>   4 files changed, 27 insertions(+), 7 deletions(-)
>
Series
Acked-by: Jon Maloy <jm...@re...>

Re: [tipc-discussion] [PATCHv2 net 2/3] tipc: do not update mtu if msg_max is too small in mtu negotiation

[Jon M. <jm...@re...>]

On 2023-05-03 09:35, Xin Long wrote:
> On Tue, May 2, 2023 at 11:31 PM Tung Quang Nguyen
> <tun...@de...> wrote:
>>> When doing link mtu negotiation, a malicious peer may send Activate msg
>>> with a very small mtu, e.g. 4 in Shuang's testing, without checking for
>>> the minimum mtu, l->mtu will be set to 4 in tipc_link_proto_rcv(), then
>>> n->links[bearer_id].mtu is set to 4294967228, which is a overflow of
>>> '4 - INT_H_SIZE - EMSG_OVERHEAD' in tipc_link_mss().
>>>
>>> With tipc_link.mtu = 4, tipc_link_xmit() kept printing the warning:
>>>
>>> tipc: Too large msg, purging xmit list 1 5 0 40 4!
>>> tipc: Too large msg, purging xmit list 1 15 0 60 4!
>>>
>>> And with tipc_link_entry.mtu 4294967228, a huge skb was allocated in
>>> named_distribute(), and when purging it in tipc_link_xmit(), a crash
>>> was even caused:
>>>
>>>   general protection fault, probably for non-canonical address 0x2100001011000dd: 0000 [#1] PREEMPT SMP PTI
>>>   CPU: 0 PID: 0 Comm: swapper/0 Kdump: loaded Not tainted 6.3.0.neta #19
>>>   RIP: 0010:kfree_skb_list_reason+0x7e/0x1f0
>>>   Call Trace:
>>>    <IRQ>
>>>    skb_release_data+0xf9/0x1d0
>>>    kfree_skb_reason+0x40/0x100
>>>    tipc_link_xmit+0x57a/0x740 [tipc]
>>>    tipc_node_xmit+0x16c/0x5c0 [tipc]
>>>    tipc_named_node_up+0x27f/0x2c0 [tipc]
>>>    tipc_node_write_unlock+0x149/0x170 [tipc]
>>>    tipc_rcv+0x608/0x740 [tipc]
>>>    tipc_udp_recv+0xdc/0x1f0 [tipc]
>>>    udp_queue_rcv_one_skb+0x33e/0x620
>>>    udp_unicast_rcv_skb.isra.72+0x75/0x90
>>>    __udp4_lib_rcv+0x56d/0xc20
>>>    ip_protocol_deliver_rcu+0x100/0x2d0
>>>
>>> This patch fixes it by checking the new mtu against tipc_bearer_min_mtu(),
>>> and not updating mtu if it is too small.
>>>
>>> v1->v2:
>>>   - do the msg_max check against the min MTU early, as Tung suggested.
>> Please move above version change comment to after "---".
> I think it's correct to NOT use ''---' for version changes, see the
> comment from davem:
>
>    https://lore.kernel.org/netdev/201...@da.../
>
> unless there are some new rules I missed.
I have not seen this one before, and I disagree with David here. Many of 
the changes
between versions are trivial, and some comments even incomprehensible 
once the patch has
been applied.
I have always put them after the "---" comment, and I will continue to 
do so until David starts
rejecting such patches.

But ok, do as you find right.

///jon

>
> Thanks.
>
>>> Fixes: ed193ece2649 ("tipc: simplify link mtu negotiation")
>>> Reported-by: Shuang Li <sh...@re...>
>>> Signed-off-by: Xin Long <luc...@gm...>
>>> ---
>>> net/tipc/link.c | 9 ++++++---
>>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/tipc/link.c b/net/tipc/link.c
>>> index b3ce24823f50..2eff1c7949cb 100644
>>> --- a/net/tipc/link.c
>>> +++ b/net/tipc/link.c
>>> @@ -2200,7 +2200,7 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
>>>        struct tipc_msg *hdr = buf_msg(skb);
>>>        struct tipc_gap_ack_blks *ga = NULL;
>>>        bool reply = msg_probe(hdr), retransmitted = false;
>>> -      u32 dlen = msg_data_sz(hdr), glen = 0;
>>> +      u32 dlen = msg_data_sz(hdr), glen = 0, msg_max;
>>>        u16 peers_snd_nxt =  msg_next_sent(hdr);
>>>        u16 peers_tol = msg_link_tolerance(hdr);
>>>        u16 peers_prio = msg_linkprio(hdr);
>>> @@ -2239,6 +2239,9 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
>>>        switch (mtyp) {
>>>        case RESET_MSG:
>>>        case ACTIVATE_MSG:
>>> +              msg_max = msg_max_pkt(hdr);
>>> +              if (msg_max < tipc_bearer_min_mtu(l->net, l->bearer_id))
>>> +                      break;
>>>                /* Complete own link name with peer's interface name */
>>>                if_name =  strrchr(l->name, ':') + 1;
>>>                if (sizeof(l->name) - (if_name - l->name) <= TIPC_MAX_IF_NAME)
>>> @@ -2283,8 +2286,8 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
>>>                l->peer_session = msg_session(hdr);
>>>                l->in_session = true;
>>>                l->peer_bearer_id = msg_bearer_id(hdr);
>>> -              if (l->mtu > msg_max_pkt(hdr))
>>> -                      l->mtu = msg_max_pkt(hdr);
>>> +              if (l->mtu > msg_max)
>>> +                      l->mtu = msg_max;
>>>                break;
>>>
>>>        case STATE_MSG:
>>> --
>>> 2.39.1

Re: [tipc-discussion] [PATCHv2 net 1/3] tipc: add tipc_bearer_min_mtu to calculate min mtu

[Xin L. <luc...@gm...>]

On Tue, May 2, 2023 at 11:37 PM Tung Quang Nguyen
<tun...@de...> wrote:
>
> >As different media may requires different min mtu, and even the
> >same media with different net family requires different min mtu,
> >add tipc_bearer_min_mtu() to calculate min mtu accordingly.
> >
> >This API will be used to check the new mtu when doing the link
> >mtu negotiation in the next patch.
> >
> >v1->v2:
> > - use bearer_get() to avoid the open code.
> This version change comment does not seem right. Please correct it to avoid confusion and put it after "---".
See the comment in Patch 2/3.

> >
> >Signed-off-by: Xin Long <luc...@gm...>
> >---
> > net/tipc/bearer.c    | 13 +++++++++++++
> > net/tipc/bearer.h    |  3 +++
> > net/tipc/udp_media.c |  5 +++--
> > 3 files changed, 19 insertions(+), 2 deletions(-)
> >
> >diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> >index 35cac7733fd3..0e9a29e1536b 100644
> >--- a/net/tipc/bearer.c
> >+++ b/net/tipc/bearer.c
> >@@ -541,6 +541,19 @@ int tipc_bearer_mtu(struct net *net, u32 bearer_id)
> >       return mtu;
> > }
> >
> >+int tipc_bearer_min_mtu(struct net *net, u32 bearer_id)
> >+{
> >+      int mtu = TIPC_MIN_BEARER_MTU;
> >+      struct tipc_bearer *b;
> >+
> >+      rcu_read_lock();
> >+      b = bearer_get(net, bearer_id);
> >+      if (b)
> >+              mtu += b->encap_hlen;
> >+      rcu_read_unlock();
> >+      return mtu;
> >+}
> >+
> > /* tipc_bearer_xmit_skb - sends buffer to destination over bearer
> >  */
> > void tipc_bearer_xmit_skb(struct net *net, u32 bearer_id,
> >diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
> >index 490ad6e5f7a3..bd0cc5c287ef 100644
> >--- a/net/tipc/bearer.h
> >+++ b/net/tipc/bearer.h
> >@@ -146,6 +146,7 @@ struct tipc_media {
> >  * @identity: array index of this bearer within TIPC bearer array
> >  * @disc: ptr to link setup request
> >  * @net_plane: network plane ('A' through 'H') currently associated with bearer
> >+ * @encap_hlen: encap headers length
> >  * @up: bearer up flag (bit 0)
> >  * @refcnt: tipc_bearer reference counter
> >  *
> >@@ -170,6 +171,7 @@ struct tipc_bearer {
> >       u32 identity;
> >       struct tipc_discoverer *disc;
> >       char net_plane;
> >+      u16 encap_hlen;
> >       unsigned long up;
> >       refcount_t refcnt;
> > };
> >@@ -232,6 +234,7 @@ int tipc_bearer_setup(void);
> > void tipc_bearer_cleanup(void);
> > void tipc_bearer_stop(struct net *net);
> > int tipc_bearer_mtu(struct net *net, u32 bearer_id);
> >+int tipc_bearer_min_mtu(struct net *net, u32 bearer_id);
> > bool tipc_bearer_bcast_support(struct net *net, u32 bearer_id);
> > void tipc_bearer_xmit_skb(struct net *net, u32 bearer_id,
> >                         struct sk_buff *skb,
> >diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
> >index c2bb818704c8..0a85244fd618 100644
> >--- a/net/tipc/udp_media.c
> >+++ b/net/tipc/udp_media.c
> >@@ -738,8 +738,8 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b,
> >                       udp_conf.local_ip.s_addr = local.ipv4.s_addr;
> >               udp_conf.use_udp_checksums = false;
> >               ub->ifindex = dev->ifindex;
> >-              if (tipc_mtu_bad(dev, sizeof(struct iphdr) +
> >-                                    sizeof(struct udphdr))) {
> >+              b->encap_hlen = sizeof(struct iphdr) + sizeof(struct udphdr);
> >+              if (tipc_mtu_bad(dev, b->encap_hlen)) {
> I agree that calling tipc_mtu_bad() is not necessary for UDP bearer enabling. You can remove it in this patch.
To be honest, it's NOT appropriate to do code cleanup in this patch,
nor in net.git.

Thanks.

> >                       err = -EINVAL;
> >                       goto err;
> >               }
> >@@ -760,6 +760,7 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b,
> >               else
> >                       udp_conf.local_ip6 = local.ipv6;
> >               ub->ifindex = dev->ifindex;
> >+              b->encap_hlen = sizeof(struct ipv6hdr) + sizeof(struct udphdr);
> >               b->mtu = 1280;
> > #endif
> >       } else {
> >--
> >2.39.1
>

Re: [tipc-discussion] [PATCHv2 net 2/3] tipc: do not update mtu if msg_max is too small in mtu negotiation

[Xin L. <luc...@gm...>]

On Tue, May 2, 2023 at 11:31 PM Tung Quang Nguyen
<tun...@de...> wrote:
>
> >When doing link mtu negotiation, a malicious peer may send Activate msg
> >with a very small mtu, e.g. 4 in Shuang's testing, without checking for
> >the minimum mtu, l->mtu will be set to 4 in tipc_link_proto_rcv(), then
> >n->links[bearer_id].mtu is set to 4294967228, which is a overflow of
> >'4 - INT_H_SIZE - EMSG_OVERHEAD' in tipc_link_mss().
> >
> >With tipc_link.mtu = 4, tipc_link_xmit() kept printing the warning:
> >
> > tipc: Too large msg, purging xmit list 1 5 0 40 4!
> > tipc: Too large msg, purging xmit list 1 15 0 60 4!
> >
> >And with tipc_link_entry.mtu 4294967228, a huge skb was allocated in
> >named_distribute(), and when purging it in tipc_link_xmit(), a crash
> >was even caused:
> >
> >  general protection fault, probably for non-canonical address 0x2100001011000dd: 0000 [#1] PREEMPT SMP PTI
> >  CPU: 0 PID: 0 Comm: swapper/0 Kdump: loaded Not tainted 6.3.0.neta #19
> >  RIP: 0010:kfree_skb_list_reason+0x7e/0x1f0
> >  Call Trace:
> >   <IRQ>
> >   skb_release_data+0xf9/0x1d0
> >   kfree_skb_reason+0x40/0x100
> >   tipc_link_xmit+0x57a/0x740 [tipc]
> >   tipc_node_xmit+0x16c/0x5c0 [tipc]
> >   tipc_named_node_up+0x27f/0x2c0 [tipc]
> >   tipc_node_write_unlock+0x149/0x170 [tipc]
> >   tipc_rcv+0x608/0x740 [tipc]
> >   tipc_udp_recv+0xdc/0x1f0 [tipc]
> >   udp_queue_rcv_one_skb+0x33e/0x620
> >   udp_unicast_rcv_skb.isra.72+0x75/0x90
> >   __udp4_lib_rcv+0x56d/0xc20
> >   ip_protocol_deliver_rcu+0x100/0x2d0
> >
> >This patch fixes it by checking the new mtu against tipc_bearer_min_mtu(),
> >and not updating mtu if it is too small.
> >
> >v1->v2:
> >  - do the msg_max check against the min MTU early, as Tung suggested.
> Please move above version change comment to after "---".
I think it's correct to NOT use ''---' for version changes, see the
comment from davem:

  https://lore.kernel.org/netdev/201...@da.../

unless there are some new rules I missed.

Thanks.

> >
> >Fixes: ed193ece2649 ("tipc: simplify link mtu negotiation")
> >Reported-by: Shuang Li <sh...@re...>
> >Signed-off-by: Xin Long <luc...@gm...>
> >---
> > net/tipc/link.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> >diff --git a/net/tipc/link.c b/net/tipc/link.c
> >index b3ce24823f50..2eff1c7949cb 100644
> >--- a/net/tipc/link.c
> >+++ b/net/tipc/link.c
> >@@ -2200,7 +2200,7 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
> >       struct tipc_msg *hdr = buf_msg(skb);
> >       struct tipc_gap_ack_blks *ga = NULL;
> >       bool reply = msg_probe(hdr), retransmitted = false;
> >-      u32 dlen = msg_data_sz(hdr), glen = 0;
> >+      u32 dlen = msg_data_sz(hdr), glen = 0, msg_max;
> >       u16 peers_snd_nxt =  msg_next_sent(hdr);
> >       u16 peers_tol = msg_link_tolerance(hdr);
> >       u16 peers_prio = msg_linkprio(hdr);
> >@@ -2239,6 +2239,9 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
> >       switch (mtyp) {
> >       case RESET_MSG:
> >       case ACTIVATE_MSG:
> >+              msg_max = msg_max_pkt(hdr);
> >+              if (msg_max < tipc_bearer_min_mtu(l->net, l->bearer_id))
> >+                      break;
> >               /* Complete own link name with peer's interface name */
> >               if_name =  strrchr(l->name, ':') + 1;
> >               if (sizeof(l->name) - (if_name - l->name) <= TIPC_MAX_IF_NAME)
> >@@ -2283,8 +2286,8 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
> >               l->peer_session = msg_session(hdr);
> >               l->in_session = true;
> >               l->peer_bearer_id = msg_bearer_id(hdr);
> >-              if (l->mtu > msg_max_pkt(hdr))
> >-                      l->mtu = msg_max_pkt(hdr);
> >+              if (l->mtu > msg_max)
> >+                      l->mtu = msg_max;
> >               break;
> >
> >       case STATE_MSG:
> >--
> >2.39.1
>

Re: [tipc-discussion] [PATCHv2 net 1/3] tipc: add tipc_bearer_min_mtu to calculate min mtu

[Tung Q. N. <tun...@de...>]

>As different media may requires different min mtu, and even the
>same media with different net family requires different min mtu,
>add tipc_bearer_min_mtu() to calculate min mtu accordingly.
>
>This API will be used to check the new mtu when doing the link
>mtu negotiation in the next patch.
>
>v1->v2:
> - use bearer_get() to avoid the open code.
This version change comment does not seem right. Please correct it to avoid confusion and put it after "---".
>
>Signed-off-by: Xin Long <luc...@gm...>
>---
> net/tipc/bearer.c    | 13 +++++++++++++
> net/tipc/bearer.h    |  3 +++
> net/tipc/udp_media.c |  5 +++--
> 3 files changed, 19 insertions(+), 2 deletions(-)
>
>diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
>index 35cac7733fd3..0e9a29e1536b 100644
>--- a/net/tipc/bearer.c
>+++ b/net/tipc/bearer.c
>@@ -541,6 +541,19 @@ int tipc_bearer_mtu(struct net *net, u32 bearer_id)
> 	return mtu;
> }
>
>+int tipc_bearer_min_mtu(struct net *net, u32 bearer_id)
>+{
>+	int mtu = TIPC_MIN_BEARER_MTU;
>+	struct tipc_bearer *b;
>+
>+	rcu_read_lock();
>+	b = bearer_get(net, bearer_id);
>+	if (b)
>+		mtu += b->encap_hlen;
>+	rcu_read_unlock();
>+	return mtu;
>+}
>+
> /* tipc_bearer_xmit_skb - sends buffer to destination over bearer
>  */
> void tipc_bearer_xmit_skb(struct net *net, u32 bearer_id,
>diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
>index 490ad6e5f7a3..bd0cc5c287ef 100644
>--- a/net/tipc/bearer.h
>+++ b/net/tipc/bearer.h
>@@ -146,6 +146,7 @@ struct tipc_media {
>  * @identity: array index of this bearer within TIPC bearer array
>  * @disc: ptr to link setup request
>  * @net_plane: network plane ('A' through 'H') currently associated with bearer
>+ * @encap_hlen: encap headers length
>  * @up: bearer up flag (bit 0)
>  * @refcnt: tipc_bearer reference counter
>  *
>@@ -170,6 +171,7 @@ struct tipc_bearer {
> 	u32 identity;
> 	struct tipc_discoverer *disc;
> 	char net_plane;
>+	u16 encap_hlen;
> 	unsigned long up;
> 	refcount_t refcnt;
> };
>@@ -232,6 +234,7 @@ int tipc_bearer_setup(void);
> void tipc_bearer_cleanup(void);
> void tipc_bearer_stop(struct net *net);
> int tipc_bearer_mtu(struct net *net, u32 bearer_id);
>+int tipc_bearer_min_mtu(struct net *net, u32 bearer_id);
> bool tipc_bearer_bcast_support(struct net *net, u32 bearer_id);
> void tipc_bearer_xmit_skb(struct net *net, u32 bearer_id,
> 			  struct sk_buff *skb,
>diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
>index c2bb818704c8..0a85244fd618 100644
>--- a/net/tipc/udp_media.c
>+++ b/net/tipc/udp_media.c
>@@ -738,8 +738,8 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b,
> 			udp_conf.local_ip.s_addr = local.ipv4.s_addr;
> 		udp_conf.use_udp_checksums = false;
> 		ub->ifindex = dev->ifindex;
>-		if (tipc_mtu_bad(dev, sizeof(struct iphdr) +
>-				      sizeof(struct udphdr))) {
>+		b->encap_hlen = sizeof(struct iphdr) + sizeof(struct udphdr);
>+		if (tipc_mtu_bad(dev, b->encap_hlen)) {
I agree that calling tipc_mtu_bad() is not necessary for UDP bearer enabling. You can remove it in this patch.
> 			err = -EINVAL;
> 			goto err;
> 		}
>@@ -760,6 +760,7 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b,
> 		else
> 			udp_conf.local_ip6 = local.ipv6;
> 		ub->ifindex = dev->ifindex;
>+		b->encap_hlen = sizeof(struct ipv6hdr) + sizeof(struct udphdr);
> 		b->mtu = 1280;
> #endif
> 	} else {
>--
>2.39.1

Re: [tipc-discussion] [PATCHv2 net 2/3] tipc: do not update mtu if msg_max is too small in mtu negotiation

[Tung Q. N. <tun...@de...>]

>When doing link mtu negotiation, a malicious peer may send Activate msg
>with a very small mtu, e.g. 4 in Shuang's testing, without checking for
>the minimum mtu, l->mtu will be set to 4 in tipc_link_proto_rcv(), then
>n->links[bearer_id].mtu is set to 4294967228, which is a overflow of
>'4 - INT_H_SIZE - EMSG_OVERHEAD' in tipc_link_mss().
>
>With tipc_link.mtu = 4, tipc_link_xmit() kept printing the warning:
>
> tipc: Too large msg, purging xmit list 1 5 0 40 4!
> tipc: Too large msg, purging xmit list 1 15 0 60 4!
>
>And with tipc_link_entry.mtu 4294967228, a huge skb was allocated in
>named_distribute(), and when purging it in tipc_link_xmit(), a crash
>was even caused:
>
>  general protection fault, probably for non-canonical address 0x2100001011000dd: 0000 [#1] PREEMPT SMP PTI
>  CPU: 0 PID: 0 Comm: swapper/0 Kdump: loaded Not tainted 6.3.0.neta #19
>  RIP: 0010:kfree_skb_list_reason+0x7e/0x1f0
>  Call Trace:
>   <IRQ>
>   skb_release_data+0xf9/0x1d0
>   kfree_skb_reason+0x40/0x100
>   tipc_link_xmit+0x57a/0x740 [tipc]
>   tipc_node_xmit+0x16c/0x5c0 [tipc]
>   tipc_named_node_up+0x27f/0x2c0 [tipc]
>   tipc_node_write_unlock+0x149/0x170 [tipc]
>   tipc_rcv+0x608/0x740 [tipc]
>   tipc_udp_recv+0xdc/0x1f0 [tipc]
>   udp_queue_rcv_one_skb+0x33e/0x620
>   udp_unicast_rcv_skb.isra.72+0x75/0x90
>   __udp4_lib_rcv+0x56d/0xc20
>   ip_protocol_deliver_rcu+0x100/0x2d0
>
>This patch fixes it by checking the new mtu against tipc_bearer_min_mtu(),
>and not updating mtu if it is too small.
>
>v1->v2:
>  - do the msg_max check against the min MTU early, as Tung suggested.
Please move above version change comment to after "---".
>
>Fixes: ed193ece2649 ("tipc: simplify link mtu negotiation")
>Reported-by: Shuang Li <sh...@re...>
>Signed-off-by: Xin Long <luc...@gm...>
>---
> net/tipc/link.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
>diff --git a/net/tipc/link.c b/net/tipc/link.c
>index b3ce24823f50..2eff1c7949cb 100644
>--- a/net/tipc/link.c
>+++ b/net/tipc/link.c
>@@ -2200,7 +2200,7 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
> 	struct tipc_msg *hdr = buf_msg(skb);
> 	struct tipc_gap_ack_blks *ga = NULL;
> 	bool reply = msg_probe(hdr), retransmitted = false;
>-	u32 dlen = msg_data_sz(hdr), glen = 0;
>+	u32 dlen = msg_data_sz(hdr), glen = 0, msg_max;
> 	u16 peers_snd_nxt =  msg_next_sent(hdr);
> 	u16 peers_tol = msg_link_tolerance(hdr);
> 	u16 peers_prio = msg_linkprio(hdr);
>@@ -2239,6 +2239,9 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
> 	switch (mtyp) {
> 	case RESET_MSG:
> 	case ACTIVATE_MSG:
>+		msg_max = msg_max_pkt(hdr);
>+		if (msg_max < tipc_bearer_min_mtu(l->net, l->bearer_id))
>+			break;
> 		/* Complete own link name with peer's interface name */
> 		if_name =  strrchr(l->name, ':') + 1;
> 		if (sizeof(l->name) - (if_name - l->name) <= TIPC_MAX_IF_NAME)
>@@ -2283,8 +2286,8 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
> 		l->peer_session = msg_session(hdr);
> 		l->in_session = true;
> 		l->peer_bearer_id = msg_bearer_id(hdr);
>-		if (l->mtu > msg_max_pkt(hdr))
>-			l->mtu = msg_max_pkt(hdr);
>+		if (l->mtu > msg_max)
>+			l->mtu = msg_max;
> 		break;
>
> 	case STATE_MSG:
>--
>2.39.1

[tipc-discussion] [PATCHv2 net 1/3] tipc: add tipc_bearer_min_mtu to calculate min mtu

[Xin L. <luc...@gm...>]

As different media may requires different min mtu, and even the
same media with different net family requires different min mtu,
add tipc_bearer_min_mtu() to calculate min mtu accordingly.

This API will be used to check the new mtu when doing the link
mtu negotiation in the next patch.

v1->v2:
 - use bearer_get() to avoid the open code.

Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/bearer.c    | 13 +++++++++++++
 net/tipc/bearer.h    |  3 +++
 net/tipc/udp_media.c |  5 +++--
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 35cac7733fd3..0e9a29e1536b 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -541,6 +541,19 @@ int tipc_bearer_mtu(struct net *net, u32 bearer_id)
 	return mtu;
 }
 
+int tipc_bearer_min_mtu(struct net *net, u32 bearer_id)
+{
+	int mtu = TIPC_MIN_BEARER_MTU;
+	struct tipc_bearer *b;
+
+	rcu_read_lock();
+	b = bearer_get(net, bearer_id);
+	if (b)
+		mtu += b->encap_hlen;
+	rcu_read_unlock();
+	return mtu;
+}
+
 /* tipc_bearer_xmit_skb - sends buffer to destination over bearer
  */
 void tipc_bearer_xmit_skb(struct net *net, u32 bearer_id,
diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
index 490ad6e5f7a3..bd0cc5c287ef 100644
--- a/net/tipc/bearer.h
+++ b/net/tipc/bearer.h
@@ -146,6 +146,7 @@ struct tipc_media {
  * @identity: array index of this bearer within TIPC bearer array
  * @disc: ptr to link setup request
  * @net_plane: network plane ('A' through 'H') currently associated with bearer
+ * @encap_hlen: encap headers length
  * @up: bearer up flag (bit 0)
  * @refcnt: tipc_bearer reference counter
  *
@@ -170,6 +171,7 @@ struct tipc_bearer {
 	u32 identity;
 	struct tipc_discoverer *disc;
 	char net_plane;
+	u16 encap_hlen;
 	unsigned long up;
 	refcount_t refcnt;
 };
@@ -232,6 +234,7 @@ int tipc_bearer_setup(void);
 void tipc_bearer_cleanup(void);
 void tipc_bearer_stop(struct net *net);
 int tipc_bearer_mtu(struct net *net, u32 bearer_id);
+int tipc_bearer_min_mtu(struct net *net, u32 bearer_id);
 bool tipc_bearer_bcast_support(struct net *net, u32 bearer_id);
 void tipc_bearer_xmit_skb(struct net *net, u32 bearer_id,
 			  struct sk_buff *skb,
diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index c2bb818704c8..0a85244fd618 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c
@@ -738,8 +738,8 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b,
 			udp_conf.local_ip.s_addr = local.ipv4.s_addr;
 		udp_conf.use_udp_checksums = false;
 		ub->ifindex = dev->ifindex;
-		if (tipc_mtu_bad(dev, sizeof(struct iphdr) +
-				      sizeof(struct udphdr))) {
+		b->encap_hlen = sizeof(struct iphdr) + sizeof(struct udphdr);
+		if (tipc_mtu_bad(dev, b->encap_hlen)) {
 			err = -EINVAL;
 			goto err;
 		}
@@ -760,6 +760,7 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b,
 		else
 			udp_conf.local_ip6 = local.ipv6;
 		ub->ifindex = dev->ifindex;
+		b->encap_hlen = sizeof(struct ipv6hdr) + sizeof(struct udphdr);
 		b->mtu = 1280;
 #endif
 	} else {
-- 
2.39.1

[tipc-discussion] [PATCHv2 net 3/3] tipc: check the bearer min mtu properly when setting it by netlink

[Xin L. <luc...@gm...>]

Checking the bearer min mtu with tipc_udp_mtu_bad() only works for
IPv4 UDP bearer, and IPv6 UDP bearer has a different value for the
min mtu. This patch checks with encap_hlen + TIPC_MIN_BEARER_MTU
for min mtu, which works for both IPv4 and IPv6 UDP bearer.

Note that tipc_udp_mtu_bad() is still used to check media min mtu
in __tipc_nl_media_set(), as m->mtu currently is only used by the
IPv4 UDP bearer as its default mtu value.

Fixes: 682cd3cf946b ("tipc: confgiure and apply UDP bearer MTU on running links")
Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/bearer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 0e9a29e1536b..53881406e200 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -1151,8 +1151,8 @@ int __tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
 				return -EINVAL;
 			}
 #ifdef CONFIG_TIPC_MEDIA_UDP
-			if (tipc_udp_mtu_bad(nla_get_u32
-					     (props[TIPC_NLA_PROP_MTU]))) {
+			if (nla_get_u32(props[TIPC_NLA_PROP_MTU]) <
+			    b->encap_hlen + TIPC_MIN_BEARER_MTU) {
 				NL_SET_ERR_MSG(info->extack,
 					       "MTU value is out-of-range");
 				return -EINVAL;
-- 
2.39.1

[tipc-discussion] [PATCHv2 net 2/3] tipc: do not update mtu if msg_max is too small in mtu negotiation

[Xin L. <luc...@gm...>]

When doing link mtu negotiation, a malicious peer may send Activate msg
with a very small mtu, e.g. 4 in Shuang's testing, without checking for
the minimum mtu, l->mtu will be set to 4 in tipc_link_proto_rcv(), then
n->links[bearer_id].mtu is set to 4294967228, which is a overflow of
'4 - INT_H_SIZE - EMSG_OVERHEAD' in tipc_link_mss().

With tipc_link.mtu = 4, tipc_link_xmit() kept printing the warning:

 tipc: Too large msg, purging xmit list 1 5 0 40 4!
 tipc: Too large msg, purging xmit list 1 15 0 60 4!

And with tipc_link_entry.mtu 4294967228, a huge skb was allocated in
named_distribute(), and when purging it in tipc_link_xmit(), a crash
was even caused:

  general protection fault, probably for non-canonical address 0x2100001011000dd: 0000 [#1] PREEMPT SMP PTI
  CPU: 0 PID: 0 Comm: swapper/0 Kdump: loaded Not tainted 6.3.0.neta #19
  RIP: 0010:kfree_skb_list_reason+0x7e/0x1f0
  Call Trace:
   <IRQ>
   skb_release_data+0xf9/0x1d0
   kfree_skb_reason+0x40/0x100
   tipc_link_xmit+0x57a/0x740 [tipc]
   tipc_node_xmit+0x16c/0x5c0 [tipc]
   tipc_named_node_up+0x27f/0x2c0 [tipc]
   tipc_node_write_unlock+0x149/0x170 [tipc]
   tipc_rcv+0x608/0x740 [tipc]
   tipc_udp_recv+0xdc/0x1f0 [tipc]
   udp_queue_rcv_one_skb+0x33e/0x620
   udp_unicast_rcv_skb.isra.72+0x75/0x90
   __udp4_lib_rcv+0x56d/0xc20
   ip_protocol_deliver_rcu+0x100/0x2d0

This patch fixes it by checking the new mtu against tipc_bearer_min_mtu(),
and not updating mtu if it is too small.

v1->v2:
  - do the msg_max check against the min MTU early, as Tung suggested.

Fixes: ed193ece2649 ("tipc: simplify link mtu negotiation")
Reported-by: Shuang Li <sh...@re...>
Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/link.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index b3ce24823f50..2eff1c7949cb 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -2200,7 +2200,7 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
 	struct tipc_msg *hdr = buf_msg(skb);
 	struct tipc_gap_ack_blks *ga = NULL;
 	bool reply = msg_probe(hdr), retransmitted = false;
-	u32 dlen = msg_data_sz(hdr), glen = 0;
+	u32 dlen = msg_data_sz(hdr), glen = 0, msg_max;
 	u16 peers_snd_nxt =  msg_next_sent(hdr);
 	u16 peers_tol = msg_link_tolerance(hdr);
 	u16 peers_prio = msg_linkprio(hdr);
@@ -2239,6 +2239,9 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
 	switch (mtyp) {
 	case RESET_MSG:
 	case ACTIVATE_MSG:
+		msg_max = msg_max_pkt(hdr);
+		if (msg_max < tipc_bearer_min_mtu(l->net, l->bearer_id))
+			break;
 		/* Complete own link name with peer's interface name */
 		if_name =  strrchr(l->name, ':') + 1;
 		if (sizeof(l->name) - (if_name - l->name) <= TIPC_MAX_IF_NAME)
@@ -2283,8 +2286,8 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
 		l->peer_session = msg_session(hdr);
 		l->in_session = true;
 		l->peer_bearer_id = msg_bearer_id(hdr);
-		if (l->mtu > msg_max_pkt(hdr))
-			l->mtu = msg_max_pkt(hdr);
+		if (l->mtu > msg_max)
+			l->mtu = msg_max;
 		break;
 
 	case STATE_MSG:
-- 
2.39.1

[tipc-discussion] [PATCHv2 net 0/3] tipc: fix the mtu update in link mtu negotiation

[Xin L. <luc...@gm...>]

This patchset fixes a crash caused by a too small MTU carried in the
activate msg. Note that as such malicious packet does not exist in
the normal env, the fix won't break any application

The 1st patch introduces a function to calculate the minimum MTU for
the bearer, and the 2nd patch fixes the crash with this helper. While
at it, the 3rd patch fixes the udp bearer mtu update by netlink with
this helper.

Xin Long (3):
  tipc: add tipc_bearer_min_mtu to calculate min mtu
  tipc: do not update mtu if msg_max is too small in mtu negotiation
  tipc: check the bearer min mtu properly when setting it by netlink

 net/tipc/bearer.c    | 17 +++++++++++++++--
 net/tipc/bearer.h    |  3 +++
 net/tipc/link.c      |  9 ++++++---
 net/tipc/udp_media.c |  5 +++--
 4 files changed, 27 insertions(+), 7 deletions(-)

-- 
2.39.1