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

[tipc-discussion] [PATCH net] tipc: only accept encrypted MSG_CRYPTO msgs

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

The MSG_CRYPTO msgs are always encrypted and sent to other nodes
for keys' deployment. But when receiving in peers, if those nodes
do not validate it and make sure it's encrypted, one could craft
a malicious MSG_CRYPTO msg to deploy its key with no need to know
other nodes' keys.

This patch is to do that by checking TIPC_SKB_CB(skb)->decrypted
and discard it if this packet never got decrypted.

Note that this is also a supplementary fix to CVE-2021-43267 that
can be triggered by an unencrypted malicious MSG_CRYPTO msg.

Fixes: 1ef6f7c9390f ("tipc: add automatic session key exchange")
Acked-by: Ying Xue <yin...@wi...>
Acked-by: Jon Maloy <jm...@re...>
Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/link.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 1b7a487c8841..09ae8448f394 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1298,8 +1298,11 @@ static bool tipc_data_input(struct tipc_link *l, struct sk_buff *skb,
 		return false;
 #ifdef CONFIG_TIPC_CRYPTO
 	case MSG_CRYPTO:
-		tipc_crypto_msg_rcv(l->net, skb);
-		return true;
+		if (TIPC_SKB_CB(skb)->decrypted) {
+			tipc_crypto_msg_rcv(l->net, skb);
+			return true;
+		}
+		fallthrough;
 #endif
 	default:
 		pr_warn("Dropping received illegal msg type\n");
-- 
2.27.0

Re: [tipc-discussion] [PATCH net] tipc: only accept encrypted MSG_CRYPTO msgs

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

You're right, will do

Thanks.

On Sun, Nov 14, 2021 at 2:00 PM Jon Maloy <jm...@re...> wrote:
>
> You should mention that is a supplementary fix to CVE-2021-43267,
> improving the original fix in commit
> fa40d9734a57bcbfa79a280189799f76c88f7bb0 ("tipc: fix size validations
> for the MSG_CRYPTO type")
>
> ///jon
>
>
>
>
> On 11/14/21 08:09, Xue, Ying wrote:
> > Thanks Xin! The patch looks good to me.
> >
> > Acked-by: Ying Xue <yin...@wi...>
> >
> > -----Original Message-----
> > From: Xin Long <luc...@gm...>
> > Sent: Saturday, November 13, 2021 3:23 AM
> > To: tip...@li...
> > Subject: [tipc-discussion] [PATCH net] tipc: only accept encrypted MSG_CRYPTO msgs
> >
> > The MSG_CRYPTO msgs are always encrypted and sent to other nodes for keys' deployment. But when receiving in peers, if those nodes do not validate it and make sure it's encrypted, one could craft a malicious MSG_CRYPTO msg to deploy its key with no need to know other nodes' keys.
> >
> > This patch is to do that by checking TIPC_SKB_CB(skb)->decrypted and discard it if this packet never got decrypted.
> >
> > Fixes: 1ef6f7c9390f ("tipc: add automatic session key exchange")
> > Signed-off-by: Xin Long <luc...@gm...>
> > ---
> >   net/tipc/link.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/tipc/link.c b/net/tipc/link.c index 1b7a487c8841..09ae8448f394 100644
> > --- a/net/tipc/link.c
> > +++ b/net/tipc/link.c
> > @@ -1298,8 +1298,11 @@ static bool tipc_data_input(struct tipc_link *l, struct sk_buff *skb,
> >               return false;
> >   #ifdef CONFIG_TIPC_CRYPTO
> >       case MSG_CRYPTO:
> > -             tipc_crypto_msg_rcv(l->net, skb);
> > -             return true;
> > +             if (TIPC_SKB_CB(skb)->decrypted) {
> > +                     tipc_crypto_msg_rcv(l->net, skb);
> > +                     return true;
> > +             }
> > +             fallthrough;
> >   #endif
> >       default:
> >               pr_warn("Dropping received illegal msg type\n");
> > --
> > 2.27.0
> >
> >
> >
> > _______________________________________________
> > tipc-discussion mailing list
> > tip...@li...
> > https://lists.sourceforge.net/lists/listinfo/tipc-discussion
> >
> >
> > _______________________________________________
> > tipc-discussion mailing list
> > tip...@li...
> > https://lists.sourceforge.net/lists/listinfo/tipc-discussion
> >
>

Re: [tipc-discussion] [PATCH net] tipc: only accept encrypted MSG_CRYPTO msgs

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

You should mention that is a supplementary fix to CVE-2021-43267, 
improving the original fix in commit 
fa40d9734a57bcbfa79a280189799f76c88f7bb0 ("tipc: fix size validations 
for the MSG_CRYPTO type")

///jon




On 11/14/21 08:09, Xue, Ying wrote:
> Thanks Xin! The patch looks good to me.
>
> Acked-by: Ying Xue <yin...@wi...>
>
> -----Original Message-----
> From: Xin Long <luc...@gm...>
> Sent: Saturday, November 13, 2021 3:23 AM
> To: tip...@li...
> Subject: [tipc-discussion] [PATCH net] tipc: only accept encrypted MSG_CRYPTO msgs
>
> The MSG_CRYPTO msgs are always encrypted and sent to other nodes for keys' deployment. But when receiving in peers, if those nodes do not validate it and make sure it's encrypted, one could craft a malicious MSG_CRYPTO msg to deploy its key with no need to know other nodes' keys.
>
> This patch is to do that by checking TIPC_SKB_CB(skb)->decrypted and discard it if this packet never got decrypted.
>
> Fixes: 1ef6f7c9390f ("tipc: add automatic session key exchange")
> Signed-off-by: Xin Long <luc...@gm...>
> ---
>   net/tipc/link.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/tipc/link.c b/net/tipc/link.c index 1b7a487c8841..09ae8448f394 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -1298,8 +1298,11 @@ static bool tipc_data_input(struct tipc_link *l, struct sk_buff *skb,
>   		return false;
>   #ifdef CONFIG_TIPC_CRYPTO
>   	case MSG_CRYPTO:
> -		tipc_crypto_msg_rcv(l->net, skb);
> -		return true;
> +		if (TIPC_SKB_CB(skb)->decrypted) {
> +			tipc_crypto_msg_rcv(l->net, skb);
> +			return true;
> +		}
> +		fallthrough;
>   #endif
>   	default:
>   		pr_warn("Dropping received illegal msg type\n");
> --
> 2.27.0
>
>
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>

Re: [tipc-discussion] [PATCH net] tipc: only accept encrypted MSG_CRYPTO msgs

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

Acked-by: Jon Maloy <jm...@re...>

On 11/14/21 08:09, Xue, Ying wrote:
> Thanks Xin! The patch looks good to me.
>
> Acked-by: Ying Xue <yin...@wi...>
>
> -----Original Message-----
> From: Xin Long <luc...@gm...>
> Sent: Saturday, November 13, 2021 3:23 AM
> To: tip...@li...
> Subject: [tipc-discussion] [PATCH net] tipc: only accept encrypted MSG_CRYPTO msgs
>
> The MSG_CRYPTO msgs are always encrypted and sent to other nodes for keys' deployment. But when receiving in peers, if those nodes do not validate it and make sure it's encrypted, one could craft a malicious MSG_CRYPTO msg to deploy its key with no need to know other nodes' keys.
>
> This patch is to do that by checking TIPC_SKB_CB(skb)->decrypted and discard it if this packet never got decrypted.
>
> Fixes: 1ef6f7c9390f ("tipc: add automatic session key exchange")
> Signed-off-by: Xin Long <luc...@gm...>
> ---
>   net/tipc/link.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/tipc/link.c b/net/tipc/link.c index 1b7a487c8841..09ae8448f394 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -1298,8 +1298,11 @@ static bool tipc_data_input(struct tipc_link *l, struct sk_buff *skb,
>   		return false;
>   #ifdef CONFIG_TIPC_CRYPTO
>   	case MSG_CRYPTO:
> -		tipc_crypto_msg_rcv(l->net, skb);
> -		return true;
> +		if (TIPC_SKB_CB(skb)->decrypted) {
> +			tipc_crypto_msg_rcv(l->net, skb);
> +			return true;
> +		}
> +		fallthrough;
>   #endif
>   	default:
>   		pr_warn("Dropping received illegal msg type\n");
> --
> 2.27.0
>
>
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>

Re: [tipc-discussion] [PATCH net] tipc: only accept encrypted MSG_CRYPTO msgs

[Xue, Y. <Yin...@wi...>]

Thanks Xin! The patch looks good to me.

Acked-by: Ying Xue <yin...@wi...>

-----Original Message-----
From: Xin Long <luc...@gm...> 
Sent: Saturday, November 13, 2021 3:23 AM
To: tip...@li...
Subject: [tipc-discussion] [PATCH net] tipc: only accept encrypted MSG_CRYPTO msgs

The MSG_CRYPTO msgs are always encrypted and sent to other nodes for keys' deployment. But when receiving in peers, if those nodes do not validate it and make sure it's encrypted, one could craft a malicious MSG_CRYPTO msg to deploy its key with no need to know other nodes' keys.

This patch is to do that by checking TIPC_SKB_CB(skb)->decrypted and discard it if this packet never got decrypted.

Fixes: 1ef6f7c9390f ("tipc: add automatic session key exchange")
Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/link.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c index 1b7a487c8841..09ae8448f394 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1298,8 +1298,11 @@ static bool tipc_data_input(struct tipc_link *l, struct sk_buff *skb,
 		return false;
 #ifdef CONFIG_TIPC_CRYPTO
 	case MSG_CRYPTO:
-		tipc_crypto_msg_rcv(l->net, skb);
-		return true;
+		if (TIPC_SKB_CB(skb)->decrypted) {
+			tipc_crypto_msg_rcv(l->net, skb);
+			return true;
+		}
+		fallthrough;
 #endif
 	default:
 		pr_warn("Dropping received illegal msg type\n");
--
2.27.0



_______________________________________________
tipc-discussion mailing list
tip...@li...
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] [PATCH net] tipc: only accept encrypted MSG_CRYPTO msgs

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

The MSG_CRYPTO msgs are always encrypted and sent to other nodes
for keys' deployment. But when receiving in peers, if those nodes
do not validate it and make sure it's encrypted, one could craft
a malicious MSG_CRYPTO msg to deploy its key with no need to know
other nodes' keys.

This patch is to do that by checking TIPC_SKB_CB(skb)->decrypted
and discard it if this packet never got decrypted.

Fixes: 1ef6f7c9390f ("tipc: add automatic session key exchange")
Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/link.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 1b7a487c8841..09ae8448f394 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1298,8 +1298,11 @@ static bool tipc_data_input(struct tipc_link *l, struct sk_buff *skb,
 		return false;
 #ifdef CONFIG_TIPC_CRYPTO
 	case MSG_CRYPTO:
-		tipc_crypto_msg_rcv(l->net, skb);
-		return true;
+		if (TIPC_SKB_CB(skb)->decrypted) {
+			tipc_crypto_msg_rcv(l->net, skb);
+			return true;
+		}
+		fallthrough;
 #endif
 	default:
 		pr_warn("Dropping received illegal msg type\n");
-- 
2.27.0

Re: [tipc-discussion] [PATCH] tipc: check for null after calling kmemdup

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

On 11/11/21 15:59, Tadeusz Struk wrote:
> kmemdup can return a null pointer so need to check for it, otherwise
> the null key will be dereferenced later in tipc_crypto_key_xmit as
> can be seen in the trace [1].
>
> Cc: Jon Maloy <jm...@re...>
> Cc: Ying Xue <yin...@wi...>
> Cc: "David S. Miller" <da...@da...>
> Cc: Jakub Kicinski <ku...@ke...>
> Cc: ne...@vg...
> Cc: tip...@li...
> Cc: lin...@vg...
> Cc: st...@vg... # 5.15, 5.14, 5.10
>
> [1] https://syzkaller.appspot.com/bug?id=bca180abb29567b189efdbdb34cbf7ba851c2a58
>
> Reported-by: Dmitry Vyukov <dv...@go...>
> Signed-off-by: Tadeusz Struk <tad...@li...>
> ---
>   net/tipc/crypto.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
> index dc60c32bb70d..988a343f9fd5 100644
> --- a/net/tipc/crypto.c
> +++ b/net/tipc/crypto.c
> @@ -597,6 +597,11 @@ static int tipc_aead_init(struct tipc_aead **aead, struct tipc_aead_key *ukey,
>   	tmp->cloned = NULL;
>   	tmp->authsize = TIPC_AES_GCM_TAG_SIZE;
>   	tmp->key = kmemdup(ukey, tipc_aead_key_size(ukey), GFP_KERNEL);
> +	if (!tmp->key) {
> +		free_percpu(tmp->tfm_entry);
> +		kfree_sensitive(tmp);
> +		return -ENOMEM;
> +	}
>   	memcpy(&tmp->salt, ukey->key + keylen, TIPC_AES_GCM_SALT_SIZE);
>   	atomic_set(&tmp->users, 0);
>   	atomic64_set(&tmp->seqno, 0);
Acked-by: Jon Maloy <jm...@re...>

[tipc-discussion] shouldn't unencrypted packets be discarded if any key is set on local node?

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

Hi Everyone,

Currently in tcp_rcv(), it seems that both unencrypted and encrypted packets
can be processed even when key/master_key is set.

After the key is set, which means all packets going out will be encrypted, to
respond to the unencrypted packets with encrypted packets doesn't seem
normal, from my point of view.

Besides, it may cause some potential security issues if the local node can
still receive unencrypted packets after the key is set, such as the CVE
one fixed by:

fa40d9734a57 ("tipc: fix size validations for the MSG_CRYPTO type")

So I'm thinking of only accepting the encrypted packets if any key is
set on the local node. But I'm not sure if we have any other cases
needing it to accept both kinds of packets, anyone know? Tuong?

Thanks.

Re: [tipc-discussion] Setting Node Address

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

On 9/23/21 11:28, Andy Stec via tipc-discussion wrote:
> The online documentation says that 'tipc node set address' command can be omitted starting with kernel 4.17.  But it doesn't say that it should be replaced with 'tipc node set identity'.  Based on that and based on my testing, it doesn't seem like those 2 commands produce the same results.
>
> From: Hoang Huu Le [mailto:hoa...@de...]
> Sent: Wednesday, September 22, 2021 11:37 PM
> To: Andy Stec <And...@in...>; tip...@li...
> Subject: RE: Setting Node Address
>
>
> From: Andy Stec <And...@in...<mailto:And...@in...>>
> Sent: Thursday, September 23, 2021 10:14 AM
> To: Hoang Huu Le <hoa...@de...<mailto:hoa...@de...>>; tip...@li...<mailto:tip...@li...>
> Subject: Re: Setting Node Address
>
> Which command is still supported, is it 'tipc node set address'?  I'm getting operation not permitted error when I use 'tipc node set address'.
> [Hoang] I don't know exactly in redhat 8, but in the upstream kernel (i.e v5.14.x) I'm able to use that one to set a node address.
>
> Does 'tipc node set identity' set the node address?
> [Hoang] Yuh, I think so. Please give a try on your node.
>
> Sorry about all these questions.  Needless to say I'm not a tipc expert.
> ________________________________
> From: Hoang Huu Le <hoa...@de...<mailto:hoa...@de...>>
> Sent: Wednesday, September 22, 2021 9:05 PM
> To: tip...@li...<mailto:tip...@li...> <tip...@li...<mailto:tip...@li...>>; tip...@li...<mailto:tip...@li...> <tip...@li...<mailto:tip...@li...>>
> Subject: Re: [tipc-discussion] Setting Node Address
>
> Let's use 'tipc node set identity' instead.
> However, that command is still support as legacy, this means you can continue using it in your application.
>
>> -----Original Message-----
>> From: Andy Stec via tipc-discussion <tip...@li...<mailto:tip...@li...>>
>> Sent: Thursday, September 23, 2021 8:16 AM
>> To: tip...@li...<mailto:tip...@li...>
>> Subject: [tipc-discussion] Setting Node Address
>>
>> We are porting an application from redhat 7 to redhat 8, that is from kernel 3.10 to kernel 4.18.  It appears that "tipc node set address"
>> command has been removed in kernel 4.17.  Is there another way of setting node address?  We are trying to limit the changes in the
>> application.
tipc node set address is still supported in all versions; it has only 
been removed from the help text.
I just made a quick test doing "tipc node set address 1.1.1" on 
RHEL-8.1.0, and it works like a charm.
If you get a failure with the above syntax you should file a bug report.
Which RHEL-8 version are you running?

///Jon
>>
>> _______________________________________________
>> tipc-discussion mailing list
>> tip...@li...<mailto:tip...@li...>
>> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...<mailto:tip...@li...>
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>

Re: [tipc-discussion] Setting Node Address

[Andy S. <And...@in...>]

The online documentation says that 'tipc node set address' command can be omitted starting with kernel 4.17.  But it doesn't say that it should be replaced with 'tipc node set identity'.  Based on that and based on my testing, it doesn't seem like those 2 commands produce the same results.

From: Hoang Huu Le [mailto:hoa...@de...]
Sent: Wednesday, September 22, 2021 11:37 PM
To: Andy Stec <And...@in...>; tip...@li...
Subject: RE: Setting Node Address


From: Andy Stec <And...@in...<mailto:And...@in...>>
Sent: Thursday, September 23, 2021 10:14 AM
To: Hoang Huu Le <hoa...@de...<mailto:hoa...@de...>>; tip...@li...<mailto:tip...@li...>
Subject: Re: Setting Node Address

Which command is still supported, is it 'tipc node set address'?  I'm getting operation not permitted error when I use 'tipc node set address'.
[Hoang] I don't know exactly in redhat 8, but in the upstream kernel (i.e v5.14.x) I'm able to use that one to set a node address.

Does 'tipc node set identity' set the node address?
[Hoang] Yuh, I think so. Please give a try on your node.

Sorry about all these questions.  Needless to say I'm not a tipc expert.
________________________________
From: Hoang Huu Le <hoa...@de...<mailto:hoa...@de...>>
Sent: Wednesday, September 22, 2021 9:05 PM
To: tip...@li...<mailto:tip...@li...> <tip...@li...<mailto:tip...@li...>>; tip...@li...<mailto:tip...@li...> <tip...@li...<mailto:tip...@li...>>
Subject: Re: [tipc-discussion] Setting Node Address

Let's use 'tipc node set identity' instead.
However, that command is still support as legacy, this means you can continue using it in your application.

> -----Original Message-----
> From: Andy Stec via tipc-discussion <tip...@li...<mailto:tip...@li...>>
> Sent: Thursday, September 23, 2021 8:16 AM
> To: tip...@li...<mailto:tip...@li...>
> Subject: [tipc-discussion] Setting Node Address
>
> We are porting an application from redhat 7 to redhat 8, that is from kernel 3.10 to kernel 4.18.  It appears that "tipc node set address"
> command has been removed in kernel 4.17.  Is there another way of setting node address?  We are trying to limit the changes in the
> application.
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...<mailto:tip...@li...>
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion


_______________________________________________
tipc-discussion mailing list
tip...@li...<mailto:tip...@li...>
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] Setting Node Address

[Hoang H. Le <hoa...@de...>]

From: Andy Stec <And...@in...>
Sent: Thursday, September 23, 2021 10:14 AM
To: Hoang Huu Le <hoa...@de...>; tip...@li...
Subject: Re: Setting Node Address

Which command is still supported, is it 'tipc node set address'?  I'm getting operation not permitted error when I use 'tipc node set address'.
[Hoang] I don't know exactly in redhat 8, but in the upstream kernel (i.e v5.14.x) I'm able to use that one to set a node address.

Does 'tipc node set identity' set the node address?
[Hoang] Yuh, I think so. Please give a try on your node.

Sorry about all these questions.  Needless to say I'm not a tipc expert.
________________________________
From: Hoang Huu Le <hoa...@de...<mailto:hoa...@de...>>
Sent: Wednesday, September 22, 2021 9:05 PM
To: tip...@li...<mailto:tip...@li...> <tip...@li...<mailto:tip...@li...>>; tip...@li...<mailto:tip...@li...> <tip...@li...<mailto:tip...@li...>>
Subject: Re: [tipc-discussion] Setting Node Address

Let's use 'tipc node set identity' instead.
However, that command is still support as legacy, this means you can continue using it in your application.

> -----Original Message-----
> From: Andy Stec via tipc-discussion <tip...@li...<mailto:tip...@li...>>
> Sent: Thursday, September 23, 2021 8:16 AM
> To: tip...@li...<mailto:tip...@li...>
> Subject: [tipc-discussion] Setting Node Address
>
> We are porting an application from redhat 7 to redhat 8, that is from kernel 3.10 to kernel 4.18.  It appears that "tipc node set address"
> command has been removed in kernel 4.17.  Is there another way of setting node address?  We are trying to limit the changes in the
> application.
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...<mailto:tip...@li...>
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion


_______________________________________________
tipc-discussion mailing list
tip...@li...<mailto:tip...@li...>
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] Setting Node Address

[Andy S. <And...@in...>]

Which command is still supported, is it 'tipc node set address'?  I'm getting operation not permitted error when I use 'tipc node set address'.

Does 'tipc node set identity' set the node address?

Sorry about all these questions.  Needless to say I'm not a tipc expert.
________________________________
From: Hoang Huu Le <hoa...@de...>
Sent: Wednesday, September 22, 2021 9:05 PM
To: tip...@li... <tip...@li...>; tip...@li... <tip...@li...>
Subject: Re: [tipc-discussion] Setting Node Address

Let's use 'tipc node set identity' instead.
However, that command is still support as legacy, this means you can continue using it in your application.

> -----Original Message-----
> From: Andy Stec via tipc-discussion <tip...@li...>
> Sent: Thursday, September 23, 2021 8:16 AM
> To: tip...@li...
> Subject: [tipc-discussion] Setting Node Address
>
> We are porting an application from redhat 7 to redhat 8, that is from kernel 3.10 to kernel 4.18.  It appears that "tipc node set address"
> command has been removed in kernel 4.17.  Is there another way of setting node address?  We are trying to limit the changes in the
> application.
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion


_______________________________________________
tipc-discussion mailing list
tip...@li...
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] Setting Node Address

[Hoang H. Le <hoa...@de...>]

Let's use 'tipc node set identity' instead.
However, that command is still support as legacy, this means you can continue using it in your application.

> -----Original Message-----
> From: Andy Stec via tipc-discussion <tip...@li...>
> Sent: Thursday, September 23, 2021 8:16 AM
> To: tip...@li...
> Subject: [tipc-discussion] Setting Node Address
> 
> We are porting an application from redhat 7 to redhat 8, that is from kernel 3.10 to kernel 4.18.  It appears that "tipc node set address"
> command has been removed in kernel 4.17.  Is there another way of setting node address?  We are trying to limit the changes in the
> application.
> 
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion

[tipc-discussion] Setting Node Address

[Andy S. <And...@in...>]

We are porting an application from redhat 7 to redhat 8, that is from kernel 3.10 to kernel 4.18.  It appears that "tipc node set address" command has been removed in kernel 4.17.  Is there another way of setting node address?  We are trying to limit the changes in the application.

Re: [tipc-discussion] [PATCH net-next 8/8] tipc: add hardware gso

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

On Fri, Sep 10, 2021 at 8:08 AM Jon Maloy <jm...@re...> wrote:
>
>
>
> On 06/07/2021 14:22, Xin Long wrote:
> > Since there's no enough bit in netdev_features_t for
> > NETIF_F_GSO_TIPC_BIT, and tipc is using the simliar
> > code as sctp, this patch will reuse SKB_GSO_SCTP and
> > NETIF_F_GSO_SCTP_BIT for tipc.
> >
> > Signed-off-by: Xin Long <luc...@gm...>
> > ---
> >   include/linux/skbuff.h |  2 --
> >   net/tipc/node.c        | 15 ++++++++++++++-
> >   net/tipc/offload.c     |  4 ++--
> >   3 files changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 148bf0ed7336..b2db9cd9a73f 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -599,8 +599,6 @@ enum {
> >       SKB_GSO_UDP_L4 = 1 << 17,
> >
> >       SKB_GSO_FRAGLIST = 1 << 18,
> > -
> > -     SKB_GSO_TIPC = 1 << 19,
> >   };
> >
> >   #if BITS_PER_LONG > 32
> > diff --git a/net/tipc/node.c b/net/tipc/node.c
> > index 9947b7dfe1d2..17e59c8dac31 100644
> > --- a/net/tipc/node.c
> > +++ b/net/tipc/node.c
> > @@ -2068,7 +2068,7 @@ static bool tipc_node_check_state(struct tipc_node *n, struct sk_buff *skb,
> >    * Invoked with no locks held. Bearer pointer must point to a valid bearer
> >    * structure (i.e. cannot be NULL), but bearer can be inactive.
> >    */
> > -void tipc_rcv(struct net *net, struct sk_buff *skb, struct tipc_bearer *b)
> > +static void __tipc_rcv(struct net *net, struct sk_buff *skb, struct tipc_bearer *b)
> >   {
> >       struct sk_buff_head xmitq;
> >       struct tipc_link_entry *le;
> > @@ -2189,6 +2189,19 @@ void tipc_rcv(struct net *net, struct sk_buff *skb, struct tipc_bearer *b)
> >       kfree_skb(skb);
> >   }
> >
> > +void tipc_rcv(struct net *net, struct sk_buff *skb, struct tipc_bearer *b)
> > +{
> > +     struct sk_buff *seg, *next;
> > +
> > +     if (!skb_is_gso(skb) || !skb_is_gso_sctp(skb))
> > +             return __tipc_rcv(net, skb, b);
> > +
> > +     skb_list_walk_safe(skb_shinfo(skb)->frag_list, seg, next)
> > +             __tipc_rcv(net, seg, b);
> > +     skb_shinfo(skb)->frag_list = NULL;
> > +     consume_skb(skb);
> > +}
> > +
> >   void tipc_node_apply_property(struct net *net, struct tipc_bearer *b,
> >                             int prop)
> >   {
> > diff --git a/net/tipc/offload.c b/net/tipc/offload.c
> > index d137679f4db0..26e372178635 100644
> > --- a/net/tipc/offload.c
> > +++ b/net/tipc/offload.c
> > @@ -5,7 +5,7 @@
> >   static struct sk_buff *tipc_gso_segment(struct sk_buff *skb,
> >                                       netdev_features_t features)
> >   {
> > -     if (!(skb_shinfo(skb)->gso_type & SKB_GSO_TIPC))
> > +     if (!(skb_shinfo(skb)->gso_type & SKB_GSO_SCTP))
> >               return ERR_PTR(-EINVAL);
> >
> >       return skb_segment(skb, (features | NETIF_F_HW_CSUM) & ~NETIF_F_SG);
> > @@ -39,7 +39,7 @@ bool tipc_msg_gso_append(struct sk_buff **p, struct sk_buff *skb, u16 segs)
> >
> >               skb_shinfo(nskb)->frag_list = head;
> >               skb_shinfo(nskb)->gso_segs = 1;
> > -             skb_shinfo(nskb)->gso_type = SKB_GSO_TIPC;
> > +             skb_shinfo(nskb)->gso_type = SKB_GSO_SCTP;
> >               skb_shinfo(nskb)->gso_size = GSO_BY_FRAGS;
> >               skb_reset_network_header(head);
> >
> >
>
> I don´t have much more to add, -it looks good to me, and way simpler
> than what I was trying a couple of years ago.
>
> If you fix the checkpatch issues and, maybe if possible, split it into
> two series, you have my ack.
>
> PS. Did you test this with crypto?
Hi Jon,

Sorry for late.

Got an urgent problem from a customer recently, and spent quite a few
weeks getting things almost done.
I need to do more testing for its performance in different scenarios
before continuing.
I think I did, but I will confirm it will work well over crypto.

Thanks a lot for checking!

>
> ///jon
>

[tipc-discussion] [net] tipc: increase timeout in tipc_sk_enqueue()

[Hoang Le <hoa...@de...>]

In tipc_sk_enqueue() we use hardcoded 2 jiffies to extract
socket buffer from generic queue to particular socket.
The 2 jiffies is too short in case there are other high priority
tasks get CPU cycles for multiple jiffies update. As result, no
buffer could be enqueued to particular socket.

To solve this, we switch to use constant timeout 20msecs.
Then, the function will be expired between 2 jiffies (CONFIG_100HZ)
and 20 jiffies (CONFIG_1000HZ).

Fixes: c637c1035534 ("tipc: resolve race problem at unicast message reception")
Acked-by: Jon Maloy <jm...@re...>
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/socket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index a0a27d87f631..ad570c2450be 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2423,7 +2423,7 @@ static int tipc_sk_backlog_rcv(struct sock *sk, struct sk_buff *skb)
 static void tipc_sk_enqueue(struct sk_buff_head *inputq, struct sock *sk,
 			    u32 dport, struct sk_buff_head *xmitq)
 {
-	unsigned long time_limit = jiffies + 2;
+	unsigned long time_limit = jiffies + usecs_to_jiffies(20000);
 	struct sk_buff *skb;
 	unsigned int lim;
 	atomic_t *dcnt;
-- 
2.30.2

Re: [tipc-discussion] [net] tipc: increase timeout in tipc_sk_enqueue()

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

On 10/09/2021 01:38, Hoang Le wrote:
> In tipc_sk_enqueue() we use hardcoded 2 jiffies to extract
> socket buffer from generic queue to particular socket.
> The 2 jiffies is too short in case there are other high priority
> tasks get CPU cycles for multiple jiffies update. As result, no
> buffer could be enqueued to particular socket.
> 
> To solve this, we switch to use to constant timeout 20msecs.
> Then, the function will be expired between 2 jiffies (CONFIG_100HZ)
> and 20 jiffies (CONFIG_1000HZ).
> 
> Signed-off-by: Hoang Le <hoa...@de...>
> ---
>   net/tipc/socket.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index a0a27d87f631..ad570c2450be 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -2423,7 +2423,7 @@ static int tipc_sk_backlog_rcv(struct sock *sk, struct sk_buff *skb)
>   static void tipc_sk_enqueue(struct sk_buff_head *inputq, struct sock *sk,
>   			    u32 dport, struct sk_buff_head *xmitq)
>   {
> -	unsigned long time_limit = jiffies + 2;
> +	unsigned long time_limit = jiffies + usecs_to_jiffies(20000);
>   	struct sk_buff *skb;
>   	unsigned int lim;
>   	atomic_t *dcnt;
> 

Acked-by: Jon Maloy <jm...@re...>

[tipc-discussion] [net] tipc: increase timeout in tipc_sk_enqueue()

[Hoang Le <hoa...@de...>]

In tipc_sk_enqueue() we use hardcoded 2 jiffies to extract
socket buffer from generic queue to particular socket.
The 2 jiffies is too short in case there are other high priority
tasks get CPU cycles for multiple jiffies update. As result, no
buffer could be enqueued to particular socket.

To solve this, we switch to use to constant timeout 20msecs.
Then, the function will be expired between 2 jiffies (CONFIG_100HZ)
and 20 jiffies (CONFIG_1000HZ).

Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/socket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index a0a27d87f631..ad570c2450be 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2423,7 +2423,7 @@ static int tipc_sk_backlog_rcv(struct sock *sk, struct sk_buff *skb)
 static void tipc_sk_enqueue(struct sk_buff_head *inputq, struct sock *sk,
 			    u32 dport, struct sk_buff_head *xmitq)
 {
-	unsigned long time_limit = jiffies + 2;
+	unsigned long time_limit = jiffies + usecs_to_jiffies(20000);
 	struct sk_buff *skb;
 	unsigned int lim;
 	atomic_t *dcnt;
-- 
2.30.2

Re: [tipc-discussion] [PATCH net-next 8/8] tipc: add hardware gso

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

On 06/07/2021 14:22, Xin Long wrote:
> Since there's no enough bit in netdev_features_t for
> NETIF_F_GSO_TIPC_BIT, and tipc is using the simliar
> code as sctp, this patch will reuse SKB_GSO_SCTP and
> NETIF_F_GSO_SCTP_BIT for tipc.
> 
> Signed-off-by: Xin Long <luc...@gm...>
> ---
>   include/linux/skbuff.h |  2 --
>   net/tipc/node.c        | 15 ++++++++++++++-
>   net/tipc/offload.c     |  4 ++--
>   3 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 148bf0ed7336..b2db9cd9a73f 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -599,8 +599,6 @@ enum {
>   	SKB_GSO_UDP_L4 = 1 << 17,
>   
>   	SKB_GSO_FRAGLIST = 1 << 18,
> -
> -	SKB_GSO_TIPC = 1 << 19,
>   };
>   
>   #if BITS_PER_LONG > 32
> diff --git a/net/tipc/node.c b/net/tipc/node.c
> index 9947b7dfe1d2..17e59c8dac31 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -2068,7 +2068,7 @@ static bool tipc_node_check_state(struct tipc_node *n, struct sk_buff *skb,
>    * Invoked with no locks held. Bearer pointer must point to a valid bearer
>    * structure (i.e. cannot be NULL), but bearer can be inactive.
>    */
> -void tipc_rcv(struct net *net, struct sk_buff *skb, struct tipc_bearer *b)
> +static void __tipc_rcv(struct net *net, struct sk_buff *skb, struct tipc_bearer *b)
>   {
>   	struct sk_buff_head xmitq;
>   	struct tipc_link_entry *le;
> @@ -2189,6 +2189,19 @@ void tipc_rcv(struct net *net, struct sk_buff *skb, struct tipc_bearer *b)
>   	kfree_skb(skb);
>   }
>   
> +void tipc_rcv(struct net *net, struct sk_buff *skb, struct tipc_bearer *b)
> +{
> +	struct sk_buff *seg, *next;
> +
> +	if (!skb_is_gso(skb) || !skb_is_gso_sctp(skb))
> +		return __tipc_rcv(net, skb, b);
> +
> +	skb_list_walk_safe(skb_shinfo(skb)->frag_list, seg, next)
> +		__tipc_rcv(net, seg, b);
> +	skb_shinfo(skb)->frag_list = NULL;
> +	consume_skb(skb);
> +}
> +
>   void tipc_node_apply_property(struct net *net, struct tipc_bearer *b,
>   			      int prop)
>   {
> diff --git a/net/tipc/offload.c b/net/tipc/offload.c
> index d137679f4db0..26e372178635 100644
> --- a/net/tipc/offload.c
> +++ b/net/tipc/offload.c
> @@ -5,7 +5,7 @@
>   static struct sk_buff *tipc_gso_segment(struct sk_buff *skb,
>   					netdev_features_t features)
>   {
> -	if (!(skb_shinfo(skb)->gso_type & SKB_GSO_TIPC))
> +	if (!(skb_shinfo(skb)->gso_type & SKB_GSO_SCTP))
>   		return ERR_PTR(-EINVAL);
>   
>   	return skb_segment(skb, (features | NETIF_F_HW_CSUM) & ~NETIF_F_SG);
> @@ -39,7 +39,7 @@ bool tipc_msg_gso_append(struct sk_buff **p, struct sk_buff *skb, u16 segs)
>   
>   		skb_shinfo(nskb)->frag_list = head;
>   		skb_shinfo(nskb)->gso_segs = 1;
> -		skb_shinfo(nskb)->gso_type = SKB_GSO_TIPC;
> +		skb_shinfo(nskb)->gso_type = SKB_GSO_SCTP;
>   		skb_shinfo(nskb)->gso_size = GSO_BY_FRAGS;
>   		skb_reset_network_header(head);
>   
> 

I don´t have much more to add, -it looks good to me, and way simpler 
than what I was trying a couple of years ago.

If you fix the checkpatch issues and, maybe if possible, split it into 
two series, you have my ack.

PS. Did you test this with crypto?

///jon

Re: [tipc-discussion] [PATCH net-next 5/8] tipc: add probe recv and state transition

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

On 06/07/2021 14:22, Xin Long wrote:
> This patch is to receive and process the probe ack by checking
> msg_max_pkt() == l->pl.probe_size then does state transition
> in tipc_link_pl_recv().
> 
> For the details, see:
> 
>    https://lwn.net/Articles/860385/
> 
> Signed-off-by: Xin Long <luc...@gm...>
> ---
>   net/tipc/link.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 48 insertions(+)
> 
> diff --git a/net/tipc/link.c b/net/tipc/link.c
> index 3af6c04f82c2..241c9378e258 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -293,6 +293,7 @@ static int tipc_link_advance_transmq(struct tipc_link *l, struct tipc_link *r,
>   static void tipc_link_update_cwin(struct tipc_link *l, int released,
>   				  bool retransmitted);
>   static void tipc_link_pl_send(struct tipc_link *l);
> +static void tipc_link_pl_recv(struct tipc_link *l);
>   /*
>    *  Simple non-static link routines (i.e. referenced outside this file)
>    */
> @@ -2333,6 +2334,13 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
>   			break;
>   		}
>   
> +		if (!reply && msg_max_pkt(hdr) == l->pl.probe_size) {
> +			tipc_link_pl_recv(l);
> +			if (l->pl.state == TIPC_PL_COMPLETE)
> +				break;
> +			tipc_link_build_proto_msg(l, STATE_MSG, PROBE_PLPMTU, 0, 0, 0, 0, xmitq);
> +		}
> +
>   		/* Receive Gap ACK blocks from peer if any */
>   		glen = tipc_get_gap_ack_blks(&ga, l, hdr, true);
>   
> @@ -3061,3 +3069,43 @@ static void tipc_link_pl_send(struct tipc_link *l)
>   	}
>   	l->pl.count = TIPC_PROBE_INTERVAL;
>   }
> +
> +static void tipc_link_pl_recv(struct tipc_link *l)
> +{
> +	pr_debug("%s: PLPMTUD: link: %p, state: %d, pmtu: %d, size: %d, high: %d\n",
> +		 __func__, l, l->pl.state, l->pl.pmtu, l->pl.probe_size, l->pl.probe_high);

Many of these lines will not pass checkpatch.
///jon

> +
> +	l->pl.pmtu = l->pl.probe_size;
> +	l->pl.count = 0;
> +	if (l->pl.state == TIPC_PL_BASE) {
> +		l->pl.state = TIPC_PL_SEARCH; /* Base -> Search */
> +		l->pl.probe_size += TIPC_PL_BIG_STEP;
> +	} else if (l->pl.state == TIPC_PL_ERROR) {
> +		l->pl.state = TIPC_PL_SEARCH; /* Error -> Search */
> +
> +		l->pl.pmtu = l->pl.probe_size;
> +		l->mtu = l->pl.pmtu;
> +		l->pl.probe_size += TIPC_PL_BIG_STEP;
> +	} else if (l->pl.state == TIPC_PL_SEARCH) {
> +		if (!l->pl.probe_high) {
> +			l->pl.probe_size = min(l->pl.probe_size + TIPC_PL_BIG_STEP,
> +					       TIPC_MAX_PLPMTU);
> +			return;
> +		}
> +		l->pl.probe_size += TIPC_PL_MIN_STEP;
> +		if (l->pl.probe_size >= l->pl.probe_high) {
> +			l->pl.probe_high = 0;
> +			l->pl.raise = 0;
> +			l->pl.state = TIPC_PL_COMPLETE; /* Search -> Search Complete */
> +
> +			l->pl.probe_size = l->pl.pmtu;
> +			l->mtu = l->pl.pmtu;
> +		}
> +	} else if (l->pl.state == TIPC_PL_COMPLETE) {
> +		l->pl.raise++;
> +		if (l->pl.raise == 30) {
> +			l->pl.state = TIPC_PL_SEARCH; /* Search Complete -> Search */
> +			l->pl.probe_size += TIPC_PL_MIN_STEP;
> +		}
> +	}
> +}
> 

Re: [tipc-discussion] [PATCH net-next 0/8] tipc: add PLPMTUD probe and GSO offload

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

On 06/07/2021 14:22, Xin Long wrote:
> This patchset is to implement PLPMTUD and GSO for TIPC,
> Patch 1-5 are for PLPMTUD while 6-8 are for GSO.

I think this should be posted as two separate series, as they really 
implement two different features.

The problem I see with this is that you reduce MTU in patch #1, so
performance will suffer until the second series is adapted.

Unless there is a way around this the two series mut at least be
applied within the same release.

Also, if I understand this correctly, PLTMUD will work also for
the Ethernet bearer, so that jumbo frame capability can be detected?
I am uncertain about the value of this, since jumbo frame capability is 
already detected by the endpoint bearers, and I doubt that such frames
ever do more than one intra-subnet hop. But maybe I am wrong here? 
Anyway, this feature cannot do any harm even on Ethernet.

///jon

> 
> It gets some ideas from SCTP as their similarities like
> both are reliable datagram packets and possible to run
> over IP(v6)/UDP. But also it does some adjustments for
> TIPC.
> 
> Xin Long (8):
>    tipc: set the mtu for bearer properly for udp media
>    tipc: add the constants and variables for plpmtud
>    tipc: build probe and its reply in tipc_link_build_proto_msg
>    tipc: add probe send and state transition
>    tipc: add probe recv and state transition
>    tipc: add offload base
>    tipc: add software gso
>    tipc: add hardware gso
> 
>   include/uapi/linux/tipc_config.h |   6 --
>   net/tipc/Makefile                |   2 +-
>   net/tipc/bearer.c                |  23 ++++-
>   net/tipc/core.c                  |   3 +
>   net/tipc/link.c                  | 147 +++++++++++++++++++++++++++----
>   net/tipc/link.h                  |  29 ++++++
>   net/tipc/msg.c                   |   1 +
>   net/tipc/msg.h                   |   3 +
>   net/tipc/node.c                  |  15 +++-
>   net/tipc/offload.c               |  70 +++++++++++++++
>   net/tipc/udp_media.c             |  18 ++--
>   11 files changed, 287 insertions(+), 30 deletions(-)
>   create mode 100644 net/tipc/offload.c
> 

Re: [tipc-discussion] Strange behavior in socket.c::tipc_sk_enqueue()

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

On 08/09/2021 19:10, Hoang Huu Le wrote:
> 
> 
>> -----Original Message-----
>> From: Jon Maloy <jm...@re...>
>> Sent: Thursday, September 9, 2021 5:42 AM
>> To: Hoang Huu Le <hoa...@de...>; tip...@li...; Tung Quang Nguyen
>> <tun...@de...>; Xin Long <luc...@gm...>; Ying Xue <yin...@wi...>
>> Cc: Huy Xuan Nhat Hoang <huy...@de...>
>> Subject: Re: Strange behavior in socket.c::tipc_sk_enqueue()
>>
>>
>>
>> On 06/09/2021 05:02, Hoang Huu Le wrote:
>>> Hi Jon,  all,
>>>
>>> I did a test by setting two variables condition in range:
>>> - time limit:  2 msecs ... unlimited
>>> - search depth limit (sock's skbs): 2 skbs ... unlimited
>>>
>>> With above range settings, a maximum sock's skbs can be enqueued around 12 skbs regardless of time and search depth limit.
>>> I also combine the test with iperf TCP traffic generated and the result looks the same.
>>>
>>> So, I don't think we need to apply the search depth limit condition and/or longer timer in this function, just 2msecs is enough.
>>> I guess this result depends on kernel schedule. What are your views?
>>
>> I assume your test was done with many, e.g. 100 connections?
> 
> Yes, I did the test from 1 to 150 connections and combine with/out other traffic generate (i.e TCP).

Ok. The simpler the better. So, I suggest you post it so we can have a look.

///jon

> 
>>
>> ///jon
>>
>>>
>>> Regards,
>>> Hoang
>>>> -----Original Message-----
>>>> From: Jon Maloy <jm...@re...>
>>>> Sent: Wednesday, September 1, 2021 7:39 AM
>>>> To: Hoang Huu Le <hoa...@de...>; tip...@li...; Tung Quang Nguyen
>>>> <tun...@de...>; Xin Long <luc...@gm...>; Ying Xue <yin...@wi...>
>>>> Cc: Huy Xuan Nhat Hoang <huy...@de...>
>>>> Subject: Re: Strange behavior in socket.c::tipc_sk_enqueue()
>>>>
>>>> Guys,
>>>> After our discussion this morning regarding this problem I gave it some
>>>> more thought.
>>>>
>>>> What if we simply limit the search depth in the receive queue to some
>>>> fix number, 10, 20, 50 or something and return NULL if nothing is found
>>>> within this range. This would be a simple stack counter inside
>>>> tipc_skb_dequeue(), and would cost almost nothing.
>>>>
>>>> If you experiment with this, of course in combination with a max limit
>>>> of some milliseconds as we also discussed, we might obtain acceptable
>>>> results.
>>>>
>>>> What do you think?
>>>>
>>>> ///jon
>>>>
>>>>
>>>> On 28/07/2021 04:04, Hoang Huu Le wrote:
>>>>> Hi Jon,
>>>>>
>>>>> Let's enjoy your vacation.
>>>>> Our new team member (CCed) will take a look at it.
>>>>>
>>>>> Regards,
>>>>> Hoang
>>>>>> -----Original Message-----
>>>>>> From: Jon Maloy <jm...@re...>
>>>>>> Sent: Wednesday, July 28, 2021 6:20 AM
>>>>>> To: tip...@li...; Tung Quang Nguyen <tun...@de...>; Hoang Huu Le
>>>>>> <hoa...@de...>; Xin Long <luc...@gm...>; Ying Xue <yin...@wi...>
>>>>>> Subject: Strange behavior in socket.c::tipc_sk_enqueue()
>>>>>>
>>>>>> I did by accident discover a strange behavior in the function
>>>>>> tipc_sk_enqueue:
>>>>>>
>>>>>>
>>>>>> static void tipc_sk_enqueue(struct sk_buff_head *inputq,
>>>>>>                                 struct sock *sk, u32 dport,
>>>>>>                                 struct sk_buff_head *xmitq)
>>>>>> {
>>>>>>             struct tipc_sock *tsk = tipc_sk(sk);
>>>>>>             unsigned long time_limit = jiffies + 2;
>>>>>>             struct sk_buff *skb;
>>>>>>             unsigned int lim;
>>>>>>             atomic_t *dcnt;
>>>>>>             u32 onode;
>>>>>>
>>>>>>             while (skb_queue_len(inputq)) {
>>>>>>             if (unlikely(time_after_eq(jiffies, time_limit)))
>>>>>>                   return;
>>>>>>             [...]
>>>>>>             }
>>>>>> }
>>>>>>
>>>>>> At the moment we call time_after_eq() the two jiffies often
>>>>>> have already passed, and the skb is not dequeued.
>>>>>> I noticed that tipc_sk_rcv() may call tipc_sk_enqueue()
>>>>>> with the same skb dozens of times before the buffer can
>>>>>> be delivered further upwards in the stack.
>>>>>>
>>>>>> Needless to say that this cannot be good for performance.
>>>>>>
>>>>>> I believe the value of 2 jiffies was hard coded at a time
>>>>>> when machines were slower, and a jiffie represented a much
>>>>>> longer time interval.
>>>>>>
>>>>>> Now it is clearly too short, and should be replaced with something
>>>>>> longer and more consisten, e.g. msec_to_jiffies(2).
>>>>>>
>>>>>> Can anybody look into this?
>>>>>>
>>>>>> Also, I will be on vacation the next two weeks, which means we
>>>>>> should cancel the bi-weekly meeting next Tuesday.
>>>>>>
>>>>>> ///jon
>>>>>>
>>>>>
>>>
> 

Re: [tipc-discussion] Strange behavior in socket.c::tipc_sk_enqueue()

[Hoang H. Le <hoa...@de...>]

> -----Original Message-----
> From: Jon Maloy <jm...@re...>
> Sent: Thursday, September 9, 2021 5:42 AM
> To: Hoang Huu Le <hoa...@de...>; tip...@li...; Tung Quang Nguyen
> <tun...@de...>; Xin Long <luc...@gm...>; Ying Xue <yin...@wi...>
> Cc: Huy Xuan Nhat Hoang <huy...@de...>
> Subject: Re: Strange behavior in socket.c::tipc_sk_enqueue()
> 
> 
> 
> On 06/09/2021 05:02, Hoang Huu Le wrote:
> > Hi Jon,  all,
> >
> > I did a test by setting two variables condition in range:
> > - time limit:  2 msecs ... unlimited
> > - search depth limit (sock's skbs): 2 skbs ... unlimited
> >
> > With above range settings, a maximum sock's skbs can be enqueued around 12 skbs regardless of time and search depth limit.
> > I also combine the test with iperf TCP traffic generated and the result looks the same.
> >
> > So, I don't think we need to apply the search depth limit condition and/or longer timer in this function, just 2msecs is enough.
> > I guess this result depends on kernel schedule. What are your views?
> 
> I assume your test was done with many, e.g. 100 connections?

Yes, I did the test from 1 to 150 connections and combine with/out other traffic generate (i.e TCP).

> 
> ///jon
> 
> >
> > Regards,
> > Hoang
> >> -----Original Message-----
> >> From: Jon Maloy <jm...@re...>
> >> Sent: Wednesday, September 1, 2021 7:39 AM
> >> To: Hoang Huu Le <hoa...@de...>; tip...@li...; Tung Quang Nguyen
> >> <tun...@de...>; Xin Long <luc...@gm...>; Ying Xue <yin...@wi...>
> >> Cc: Huy Xuan Nhat Hoang <huy...@de...>
> >> Subject: Re: Strange behavior in socket.c::tipc_sk_enqueue()
> >>
> >> Guys,
> >> After our discussion this morning regarding this problem I gave it some
> >> more thought.
> >>
> >> What if we simply limit the search depth in the receive queue to some
> >> fix number, 10, 20, 50 or something and return NULL if nothing is found
> >> within this range. This would be a simple stack counter inside
> >> tipc_skb_dequeue(), and would cost almost nothing.
> >>
> >> If you experiment with this, of course in combination with a max limit
> >> of some milliseconds as we also discussed, we might obtain acceptable
> >> results.
> >>
> >> What do you think?
> >>
> >> ///jon
> >>
> >>
> >> On 28/07/2021 04:04, Hoang Huu Le wrote:
> >>> Hi Jon,
> >>>
> >>> Let's enjoy your vacation.
> >>> Our new team member (CCed) will take a look at it.
> >>>
> >>> Regards,
> >>> Hoang
> >>>> -----Original Message-----
> >>>> From: Jon Maloy <jm...@re...>
> >>>> Sent: Wednesday, July 28, 2021 6:20 AM
> >>>> To: tip...@li...; Tung Quang Nguyen <tun...@de...>; Hoang Huu Le
> >>>> <hoa...@de...>; Xin Long <luc...@gm...>; Ying Xue <yin...@wi...>
> >>>> Subject: Strange behavior in socket.c::tipc_sk_enqueue()
> >>>>
> >>>> I did by accident discover a strange behavior in the function
> >>>> tipc_sk_enqueue:
> >>>>
> >>>>
> >>>> static void tipc_sk_enqueue(struct sk_buff_head *inputq,
> >>>>                                struct sock *sk, u32 dport,
> >>>>                                struct sk_buff_head *xmitq)
> >>>> {
> >>>>            struct tipc_sock *tsk = tipc_sk(sk);
> >>>>            unsigned long time_limit = jiffies + 2;
> >>>>            struct sk_buff *skb;
> >>>>            unsigned int lim;
> >>>>            atomic_t *dcnt;
> >>>>            u32 onode;
> >>>>
> >>>>            while (skb_queue_len(inputq)) {
> >>>>            if (unlikely(time_after_eq(jiffies, time_limit)))
> >>>>                  return;
> >>>>            [...]
> >>>>            }
> >>>> }
> >>>>
> >>>> At the moment we call time_after_eq() the two jiffies often
> >>>> have already passed, and the skb is not dequeued.
> >>>> I noticed that tipc_sk_rcv() may call tipc_sk_enqueue()
> >>>> with the same skb dozens of times before the buffer can
> >>>> be delivered further upwards in the stack.
> >>>>
> >>>> Needless to say that this cannot be good for performance.
> >>>>
> >>>> I believe the value of 2 jiffies was hard coded at a time
> >>>> when machines were slower, and a jiffie represented a much
> >>>> longer time interval.
> >>>>
> >>>> Now it is clearly too short, and should be replaced with something
> >>>> longer and more consisten, e.g. msec_to_jiffies(2).
> >>>>
> >>>> Can anybody look into this?
> >>>>
> >>>> Also, I will be on vacation the next two weeks, which means we
> >>>> should cancel the bi-weekly meeting next Tuesday.
> >>>>
> >>>> ///jon
> >>>>
> >>>
> >

Re: [tipc-discussion] Strange behavior in socket.c::tipc_sk_enqueue()

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

On 06/09/2021 05:02, Hoang Huu Le wrote:
> Hi Jon,  all,
> 
> I did a test by setting two variables condition in range:
> - time limit:  2 msecs ... unlimited
> - search depth limit (sock's skbs): 2 skbs ... unlimited
> 
> With above range settings, a maximum sock's skbs can be enqueued around 12 skbs regardless of time and search depth limit.
> I also combine the test with iperf TCP traffic generated and the result looks the same.
> 
> So, I don't think we need to apply the search depth limit condition and/or longer timer in this function, just 2msecs is enough.
> I guess this result depends on kernel schedule. What are your views?

I assume your test was done with many, e.g. 100 connections?

///jon

> 
> Regards,
> Hoang
>> -----Original Message-----
>> From: Jon Maloy <jm...@re...>
>> Sent: Wednesday, September 1, 2021 7:39 AM
>> To: Hoang Huu Le <hoa...@de...>; tip...@li...; Tung Quang Nguyen
>> <tun...@de...>; Xin Long <luc...@gm...>; Ying Xue <yin...@wi...>
>> Cc: Huy Xuan Nhat Hoang <huy...@de...>
>> Subject: Re: Strange behavior in socket.c::tipc_sk_enqueue()
>>
>> Guys,
>> After our discussion this morning regarding this problem I gave it some
>> more thought.
>>
>> What if we simply limit the search depth in the receive queue to some
>> fix number, 10, 20, 50 or something and return NULL if nothing is found
>> within this range. This would be a simple stack counter inside
>> tipc_skb_dequeue(), and would cost almost nothing.
>>
>> If you experiment with this, of course in combination with a max limit
>> of some milliseconds as we also discussed, we might obtain acceptable
>> results.
>>
>> What do you think?
>>
>> ///jon
>>
>>
>> On 28/07/2021 04:04, Hoang Huu Le wrote:
>>> Hi Jon,
>>>
>>> Let's enjoy your vacation.
>>> Our new team member (CCed) will take a look at it.
>>>
>>> Regards,
>>> Hoang
>>>> -----Original Message-----
>>>> From: Jon Maloy <jm...@re...>
>>>> Sent: Wednesday, July 28, 2021 6:20 AM
>>>> To: tip...@li...; Tung Quang Nguyen <tun...@de...>; Hoang Huu Le
>>>> <hoa...@de...>; Xin Long <luc...@gm...>; Ying Xue <yin...@wi...>
>>>> Subject: Strange behavior in socket.c::tipc_sk_enqueue()
>>>>
>>>> I did by accident discover a strange behavior in the function
>>>> tipc_sk_enqueue:
>>>>
>>>>
>>>> static void tipc_sk_enqueue(struct sk_buff_head *inputq,
>>>>                                struct sock *sk, u32 dport,
>>>>                                struct sk_buff_head *xmitq)
>>>> {
>>>>            struct tipc_sock *tsk = tipc_sk(sk);
>>>>            unsigned long time_limit = jiffies + 2;
>>>>            struct sk_buff *skb;
>>>>            unsigned int lim;
>>>>            atomic_t *dcnt;
>>>>            u32 onode;
>>>>
>>>>            while (skb_queue_len(inputq)) {
>>>>            if (unlikely(time_after_eq(jiffies, time_limit)))
>>>>                  return;
>>>>            [...]
>>>>            }
>>>> }
>>>>
>>>> At the moment we call time_after_eq() the two jiffies often
>>>> have already passed, and the skb is not dequeued.
>>>> I noticed that tipc_sk_rcv() may call tipc_sk_enqueue()
>>>> with the same skb dozens of times before the buffer can
>>>> be delivered further upwards in the stack.
>>>>
>>>> Needless to say that this cannot be good for performance.
>>>>
>>>> I believe the value of 2 jiffies was hard coded at a time
>>>> when machines were slower, and a jiffie represented a much
>>>> longer time interval.
>>>>
>>>> Now it is clearly too short, and should be replaced with something
>>>> longer and more consisten, e.g. msec_to_jiffies(2).
>>>>
>>>> Can anybody look into this?
>>>>
>>>> Also, I will be on vacation the next two weeks, which means we
>>>> should cancel the bi-weekly meeting next Tuesday.
>>>>
>>>> ///jon
>>>>
>>>
> 

Re: [tipc-discussion] Strange behavior in socket.c::tipc_sk_enqueue()

[Hoang H. Le <hoa...@de...>]

Hi Jon,  all,

I did a test by setting two variables condition in range:
- time limit:  2 msecs ... unlimited
- search depth limit (sock's skbs): 2 skbs ... unlimited

With above range settings, a maximum sock's skbs can be enqueued around 12 skbs regardless of time and search depth limit.
I also combine the test with iperf TCP traffic generated and the result looks the same.

So, I don't think we need to apply the search depth limit condition and/or longer timer in this function, just 2msecs is enough.
I guess this result depends on kernel schedule. What are your views?

Regards,
Hoang
> -----Original Message-----
> From: Jon Maloy <jm...@re...>
> Sent: Wednesday, September 1, 2021 7:39 AM
> To: Hoang Huu Le <hoa...@de...>; tip...@li...; Tung Quang Nguyen
> <tun...@de...>; Xin Long <luc...@gm...>; Ying Xue <yin...@wi...>
> Cc: Huy Xuan Nhat Hoang <huy...@de...>
> Subject: Re: Strange behavior in socket.c::tipc_sk_enqueue()
> 
> Guys,
> After our discussion this morning regarding this problem I gave it some
> more thought.
> 
> What if we simply limit the search depth in the receive queue to some
> fix number, 10, 20, 50 or something and return NULL if nothing is found
> within this range. This would be a simple stack counter inside
> tipc_skb_dequeue(), and would cost almost nothing.
> 
> If you experiment with this, of course in combination with a max limit
> of some milliseconds as we also discussed, we might obtain acceptable
> results.
> 
> What do you think?
> 
> ///jon
> 
> 
> On 28/07/2021 04:04, Hoang Huu Le wrote:
> > Hi Jon,
> >
> > Let's enjoy your vacation.
> > Our new team member (CCed) will take a look at it.
> >
> > Regards,
> > Hoang
> >> -----Original Message-----
> >> From: Jon Maloy <jm...@re...>
> >> Sent: Wednesday, July 28, 2021 6:20 AM
> >> To: tip...@li...; Tung Quang Nguyen <tun...@de...>; Hoang Huu Le
> >> <hoa...@de...>; Xin Long <luc...@gm...>; Ying Xue <yin...@wi...>
> >> Subject: Strange behavior in socket.c::tipc_sk_enqueue()
> >>
> >> I did by accident discover a strange behavior in the function
> >> tipc_sk_enqueue:
> >>
> >>
> >> static void tipc_sk_enqueue(struct sk_buff_head *inputq,
> >>                               struct sock *sk, u32 dport,
> >>                               struct sk_buff_head *xmitq)
> >> {
> >>           struct tipc_sock *tsk = tipc_sk(sk);
> >>           unsigned long time_limit = jiffies + 2;
> >>           struct sk_buff *skb;
> >>           unsigned int lim;
> >>           atomic_t *dcnt;
> >>           u32 onode;
> >>
> >>           while (skb_queue_len(inputq)) {
> >>           if (unlikely(time_after_eq(jiffies, time_limit)))
> >>                 return;
> >>           [...]
> >>           }
> >> }
> >>
> >> At the moment we call time_after_eq() the two jiffies often
> >> have already passed, and the skb is not dequeued.
> >> I noticed that tipc_sk_rcv() may call tipc_sk_enqueue()
> >> with the same skb dozens of times before the buffer can
> >> be delivered further upwards in the stack.
> >>
> >> Needless to say that this cannot be good for performance.
> >>
> >> I believe the value of 2 jiffies was hard coded at a time
> >> when machines were slower, and a jiffie represented a much
> >> longer time interval.
> >>
> >> Now it is clearly too short, and should be replaced with something
> >> longer and more consisten, e.g. msec_to_jiffies(2).
> >>
> >> Can anybody look into this?
> >>
> >> Also, I will be on vacation the next two weeks, which means we
> >> should cancel the bi-weekly meeting next Tuesday.
> >>
> >> ///jon
> >>
> >