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

Re: [tipc-discussion] [PATCH RFC 0/5] tipc: add more features to TIPC encryption

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

On 7/13/20 6:23 AM, Tuong Tong Lien wrote:
> Hi Jon,
>
> Please see my feedback inline.
>
>> -----Original Message-----
>> From: Jon Maloy <jm...@re...>
>> Sent: Saturday, July 11, 2020 1:38 AM
>> To: Tuong Tong Lien <tuo...@de...>; ma...@do...; yin...@wi...; tipc-
>> dis...@li...
>> Cc: tipc-dek <tip...@de...>
>> Subject: Re: [PATCH RFC 0/5] tipc: add more features to TIPC encryption
>>
>>
>>
>> On 7/10/20 6:11 AM, Tuong Lien wrote:
>>> Hi Jon/all,
>>>
>>> As mentioned, I'd like to share the series that I have added some new
>>> features in order to complete the TIPC encryption:
>>>
>>> - Patch 1 ("tipc: fix using smp_processor_id() in preemptible"):
>>> - Patch 2 ("tipc: optimize key switching time and logic"):
>>> These two patches just do a bug-fix and optimization for the code as
>>> a preparation for later commits.
>>>
>>> - Patch 3 ("tipc: introduce encryption master key"):
>>> This will introduce a 'master key' support which is set by user as a
>>> 'long-term' or static key (e.g. shared between nodes in the cluster in
>>> user control way). It will act like a key encryption key for the later
>>> key exchange, as well as allow a new node joins the cluster while it
>>> has no knowledge of current active session keys in the existing nodes.
>>>
>>> The master key setting will use the same 'tipc node set key' command
>>> but with a 'master' flag (see below).
>>>
>>> - Patch 4 ("tipc: add automatic session key exchange"):
>>> TX key of a node will now be able to be exchanged to peer nodes (
>>> encrypted/decrypted by the master key) and attached as the
>>> corresponding RX keys automatically. A node can also 'request' for a TX
>>> key from peer whenever needed.
>>>
>>> This will enable us to do the later rekeying, and also make a new node
>>> being able to obtain the session keys from existing nodes.
>>>
>>> - Patch 5 ("tipc: add automatic rekeying for encryption key"):
>>> Finally, this patch will add the automatic rekeying which will generate
>>> a session key on each node at a specific interval. The key will be
>>> also distributed automatically to peer nodes, so it will be switched to
>>> be active shortly and traffic will be finally encrypted/decrypted by
>>> that new key.
>>>
>>> The rekeying interval is configurable as well, also user can disable or
>>> trigger an immediate rekeying if he wants.
>>>
>>> Besides, there will be a patch in the 'iproute2/tipc' including the new
>>> 'tipc node set key' command options, basically it will look like this:
>>>
>>> ---------
>>> $tipc node set key --help
>>> Usage: tipc node set key KEY [algname ALGNAME] [PROPERTIES]
>>>          tipc node set key rekeying REKEYING
>>>
>>> KEY
>>>     Symmetric KEY & SALT as a composite ASCII or hex string (0x...) in form:
>>>     [KEY: 16, 24 or 32 octets][SALT: 4 octets]
>>>
>>> ALGNAME
>>>     Cipher algorithm [default: "gcm(aes)"]
>>>
>>> PROPERTIES
>>>     master                - Set KEY as a cluster master key
>>>     <empty>               - Set KEY as a cluster key
>>>     nodeid NODEID         - Set KEY as a per-node key for own or peer
>>>
>>> REKEYING
>>>     INTERVAL              - Set rekeying interval (in minutes) [0: disable]
>>>     now                   - Trigger one (first) rekeying immediately
>>>
>>> EXAMPLES
>>>     tipc node set key 0x746869735F69735F615F6B657931365F73616C74
>>>     tipc node set key this_is_a_key16_salt algname "gcm(aes)" nodeid 1001002
>>>     tipc node set key this_is_a_master_key master rekeying now
>>>     tipc node set key rekeying 600
>>> ---------
>>>
>>> So, please help check the patches and give your comments, thanks a lot!
>>>
>>> BR/Tuong
>> I haven't reviewed this yet, but still have a comment and a question.
>> 1) It would sound less scary if we call this a "cluster key" instead of
>> a "master key"
> Yes, in terms of encryption/decryption, a "master key" is actually a "cluster key" (because the same key will be used for both TX/RX in cluster...) but it is a bit different from our traditional cluster or per-node key concepts in terms of use or key management, especially by the following points:
> - The master key will not be used for data or traffic encryption like our traditional keys, but act like a "key-encryption key" to encrypt those subordinate data keys (i.e. in the key exchange).
> - The master key should be a long-term or static key, instead of the traditional ones which will be "ephemeral" (i.e. in the rekeying...).
> - The master key will allow a new node to join the cluster when it completely doesn't know what the current active (ephemeral) keys in the existing nodes. It will also act like an "authentication" key, so ensure that the new node has the right authenticity first.
> - The master key will be almost the only key that needs to be provided by (privileged) user, along with the other automatic key mechanisms we provide here, "ephemeral" keys will be generated/used for all the TIPC traffic encryption frequently, so getting a higher secure level.
> So, I think we can call it as a "cluster master key", just to distinguish it from the "cluster key" while still retain its nature 😊.
Or just "authentication key", even if that is not the whole truth. The 
regular encryption keys can be referred to as session keys.
>
>> 2) Do you have any thoughts about how we can replace this key if it ever
>> gets compromised?
>>       To me it sounds like will need a user space TLS based framework
>> after all to achieve this.
> Yes, I have thought about this. It will require a "framework" for setting or updating the same master key in the cluster, but it will be much simpler now since that is the only one we need (as said above)... In a secure system, I believe they had such a framework already, like a "security service" for the whole cluster system (used for SSH, CLI, etc... not only TIPC), for example: the key can be simply stored in a private/protected directory or database which is shared between nodes in the cluster. So once it is updated, the key will be sent to TIPC and set as the master key, that's it.
>
> In general, the achievement from these commits is to make things much convenient for user (he just needs to set the master key...) but help increase the security for TIPC encryption since the traffic encryption keys can be replaced very frequently now...
>
> BR/Tuong
Yes, I totally agree. I will review this later this week.

///jon

>> Regards
>> ///jon
>>
>>> Tuong Lien (5):
>>>     tipc: fix using smp_processor_id() in preemptible
>>>     tipc: optimize key switching time and logic
>>>     tipc: introduce encryption master key
>>>     tipc: add automatic session key exchange
>>>     tipc: add automatic rekeying for encryption key
>>>
>>>    include/uapi/linux/tipc.h         |   2 +
>>>    include/uapi/linux/tipc_netlink.h |   2 +
>>>    net/tipc/crypto.c                 | 986 ++++++++++++++++++++++++++++----------
>>>    net/tipc/crypto.h                 |  41 +-
>>>    net/tipc/link.c                   |   5 +
>>>    net/tipc/msg.h                    |  10 +-
>>>    net/tipc/netlink.c                |   2 +
>>>    net/tipc/node.c                   |  89 ++--
>>>    net/tipc/node.h                   |   2 +
>>>    net/tipc/sysctl.c                 |   9 +
>>>    10 files changed, 862 insertions(+), 286 deletions(-)
>>>

Re: [tipc-discussion] [PATCH RFC 0/5] tipc: add more features to TIPC encryption

[Tuong T. L. <tuo...@de...>]

Hi Jon,

Please see my feedback inline.

> -----Original Message-----
> From: Jon Maloy <jm...@re...>
> Sent: Saturday, July 11, 2020 1:38 AM
> To: Tuong Tong Lien <tuo...@de...>; ma...@do...; yin...@wi...; tipc-
> dis...@li...
> Cc: tipc-dek <tip...@de...>
> Subject: Re: [PATCH RFC 0/5] tipc: add more features to TIPC encryption
> 
> 
> 
> On 7/10/20 6:11 AM, Tuong Lien wrote:
> > Hi Jon/all,
> >
> > As mentioned, I'd like to share the series that I have added some new
> > features in order to complete the TIPC encryption:
> >
> > - Patch 1 ("tipc: fix using smp_processor_id() in preemptible"):
> > - Patch 2 ("tipc: optimize key switching time and logic"):
> > These two patches just do a bug-fix and optimization for the code as
> > a preparation for later commits.
> >
> > - Patch 3 ("tipc: introduce encryption master key"):
> > This will introduce a 'master key' support which is set by user as a
> > 'long-term' or static key (e.g. shared between nodes in the cluster in
> > user control way). It will act like a key encryption key for the later
> > key exchange, as well as allow a new node joins the cluster while it
> > has no knowledge of current active session keys in the existing nodes.
> >
> > The master key setting will use the same 'tipc node set key' command
> > but with a 'master' flag (see below).
> >
> > - Patch 4 ("tipc: add automatic session key exchange"):
> > TX key of a node will now be able to be exchanged to peer nodes (
> > encrypted/decrypted by the master key) and attached as the
> > corresponding RX keys automatically. A node can also 'request' for a TX
> > key from peer whenever needed.
> >
> > This will enable us to do the later rekeying, and also make a new node
> > being able to obtain the session keys from existing nodes.
> >
> > - Patch 5 ("tipc: add automatic rekeying for encryption key"):
> > Finally, this patch will add the automatic rekeying which will generate
> > a session key on each node at a specific interval. The key will be
> > also distributed automatically to peer nodes, so it will be switched to
> > be active shortly and traffic will be finally encrypted/decrypted by
> > that new key.
> >
> > The rekeying interval is configurable as well, also user can disable or
> > trigger an immediate rekeying if he wants.
> >
> > Besides, there will be a patch in the 'iproute2/tipc' including the new
> > 'tipc node set key' command options, basically it will look like this:
> >
> > ---------
> > $tipc node set key --help
> > Usage: tipc node set key KEY [algname ALGNAME] [PROPERTIES]
> >         tipc node set key rekeying REKEYING
> >
> > KEY
> >    Symmetric KEY & SALT as a composite ASCII or hex string (0x...) in form:
> >    [KEY: 16, 24 or 32 octets][SALT: 4 octets]
> >
> > ALGNAME
> >    Cipher algorithm [default: "gcm(aes)"]
> >
> > PROPERTIES
> >    master                - Set KEY as a cluster master key
> >    <empty>               - Set KEY as a cluster key
> >    nodeid NODEID         - Set KEY as a per-node key for own or peer
> >
> > REKEYING
> >    INTERVAL              - Set rekeying interval (in minutes) [0: disable]
> >    now                   - Trigger one (first) rekeying immediately
> >
> > EXAMPLES
> >    tipc node set key 0x746869735F69735F615F6B657931365F73616C74
> >    tipc node set key this_is_a_key16_salt algname "gcm(aes)" nodeid 1001002
> >    tipc node set key this_is_a_master_key master rekeying now
> >    tipc node set key rekeying 600
> > ---------
> >
> > So, please help check the patches and give your comments, thanks a lot!
> >
> > BR/Tuong
> I haven't reviewed this yet, but still have a comment and a question.
> 1) It would sound less scary if we call this a "cluster key" instead of
> a "master key"
Yes, in terms of encryption/decryption, a "master key" is actually a "cluster key" (because the same key will be used for both TX/RX in cluster...) but it is a bit different from our traditional cluster or per-node key concepts in terms of use or key management, especially by the following points:
- The master key will not be used for data or traffic encryption like our traditional keys, but act like a "key-encryption key" to encrypt those subordinate data keys (i.e. in the key exchange).
- The master key should be a long-term or static key, instead of the traditional ones which will be "ephemeral" (i.e. in the rekeying...). 
- The master key will allow a new node to join the cluster when it completely doesn't know what the current active (ephemeral) keys in the existing nodes. It will also act like an "authentication" key, so ensure that the new node has the right authenticity first. 
- The master key will be almost the only key that needs to be provided by (privileged) user, along with the other automatic key mechanisms we provide here, "ephemeral" keys will be generated/used for all the TIPC traffic encryption frequently, so getting a higher secure level.
So, I think we can call it as a "cluster master key", just to distinguish it from the "cluster key" while still retain its nature 😊.

> 2) Do you have any thoughts about how we can replace this key if it ever
> gets compromised?
>      To me it sounds like will need a user space TLS based framework
> after all to achieve this.
Yes, I have thought about this. It will require a "framework" for setting or updating the same master key in the cluster, but it will be much simpler now since that is the only one we need (as said above)... In a secure system, I believe they had such a framework already, like a "security service" for the whole cluster system (used for SSH, CLI, etc... not only TIPC), for example: the key can be simply stored in a private/protected directory or database which is shared between nodes in the cluster. So once it is updated, the key will be sent to TIPC and set as the master key, that's it.

In general, the achievement from these commits is to make things much convenient for user (he just needs to set the master key...) but help increase the security for TIPC encryption since the traffic encryption keys can be replaced very frequently now...

BR/Tuong
> 
> Regards
> ///jon
> 
> >
> > Tuong Lien (5):
> >    tipc: fix using smp_processor_id() in preemptible
> >    tipc: optimize key switching time and logic
> >    tipc: introduce encryption master key
> >    tipc: add automatic session key exchange
> >    tipc: add automatic rekeying for encryption key
> >
> >   include/uapi/linux/tipc.h         |   2 +
> >   include/uapi/linux/tipc_netlink.h |   2 +
> >   net/tipc/crypto.c                 | 986 ++++++++++++++++++++++++++++----------
> >   net/tipc/crypto.h                 |  41 +-
> >   net/tipc/link.c                   |   5 +
> >   net/tipc/msg.h                    |  10 +-
> >   net/tipc/netlink.c                |   2 +
> >   net/tipc/node.c                   |  89 ++--
> >   net/tipc/node.h                   |   2 +
> >   net/tipc/sysctl.c                 |   9 +
> >   10 files changed, 862 insertions(+), 286 deletions(-)
> >

Re: [tipc-discussion] [PATCH RFC 0/5] tipc: add more features to TIPC encryption

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

On 7/10/20 6:11 AM, Tuong Lien wrote:
> Hi Jon/all,
>
> As mentioned, I'd like to share the series that I have added some new
> features in order to complete the TIPC encryption:
>
> - Patch 1 ("tipc: fix using smp_processor_id() in preemptible"):
> - Patch 2 ("tipc: optimize key switching time and logic"):
> These two patches just do a bug-fix and optimization for the code as
> a preparation for later commits.
>
> - Patch 3 ("tipc: introduce encryption master key"):
> This will introduce a 'master key' support which is set by user as a
> 'long-term' or static key (e.g. shared between nodes in the cluster in
> user control way). It will act like a key encryption key for the later
> key exchange, as well as allow a new node joins the cluster while it
> has no knowledge of current active session keys in the existing nodes.
>
> The master key setting will use the same 'tipc node set key' command
> but with a 'master' flag (see below).
>
> - Patch 4 ("tipc: add automatic session key exchange"):
> TX key of a node will now be able to be exchanged to peer nodes (
> encrypted/decrypted by the master key) and attached as the
> corresponding RX keys automatically. A node can also 'request' for a TX
> key from peer whenever needed.
>
> This will enable us to do the later rekeying, and also make a new node
> being able to obtain the session keys from existing nodes.
>
> - Patch 5 ("tipc: add automatic rekeying for encryption key"):
> Finally, this patch will add the automatic rekeying which will generate
> a session key on each node at a specific interval. The key will be
> also distributed automatically to peer nodes, so it will be switched to
> be active shortly and traffic will be finally encrypted/decrypted by
> that new key.
>
> The rekeying interval is configurable as well, also user can disable or
> trigger an immediate rekeying if he wants.
>
> Besides, there will be a patch in the 'iproute2/tipc' including the new
> 'tipc node set key' command options, basically it will look like this:
>
> ---------
> $tipc node set key --help
> Usage: tipc node set key KEY [algname ALGNAME] [PROPERTIES]
>         tipc node set key rekeying REKEYING
>
> KEY
>    Symmetric KEY & SALT as a composite ASCII or hex string (0x...) in form:
>    [KEY: 16, 24 or 32 octets][SALT: 4 octets]
>
> ALGNAME
>    Cipher algorithm [default: "gcm(aes)"]
>
> PROPERTIES
>    master                - Set KEY as a cluster master key
>    <empty>               - Set KEY as a cluster key
>    nodeid NODEID         - Set KEY as a per-node key for own or peer
>
> REKEYING
>    INTERVAL              - Set rekeying interval (in minutes) [0: disable]
>    now                   - Trigger one (first) rekeying immediately
>
> EXAMPLES
>    tipc node set key 0x746869735F69735F615F6B657931365F73616C74
>    tipc node set key this_is_a_key16_salt algname "gcm(aes)" nodeid 1001002
>    tipc node set key this_is_a_master_key master rekeying now
>    tipc node set key rekeying 600
> ---------
>
> So, please help check the patches and give your comments, thanks a lot!
>
> BR/Tuong
I haven't reviewed this yet, but still have a comment and a question.
1) It would sound less scary if we call this a "cluster key" instead of 
a "master key"
2) Do you have any thoughts about how we can replace this key if it ever 
gets compromised?
     To me it sounds like will need a user space TLS based framework 
after all to achieve this.

Regards
///jon

>
> Tuong Lien (5):
>    tipc: fix using smp_processor_id() in preemptible
>    tipc: optimize key switching time and logic
>    tipc: introduce encryption master key
>    tipc: add automatic session key exchange
>    tipc: add automatic rekeying for encryption key
>
>   include/uapi/linux/tipc.h         |   2 +
>   include/uapi/linux/tipc_netlink.h |   2 +
>   net/tipc/crypto.c                 | 986 ++++++++++++++++++++++++++++----------
>   net/tipc/crypto.h                 |  41 +-
>   net/tipc/link.c                   |   5 +
>   net/tipc/msg.h                    |  10 +-
>   net/tipc/netlink.c                |   2 +
>   net/tipc/node.c                   |  89 ++--
>   net/tipc/node.h                   |   2 +
>   net/tipc/sysctl.c                 |   9 +
>   10 files changed, 862 insertions(+), 286 deletions(-)
>

[tipc-discussion] [PATCH RFC 5/5] tipc: add automatic rekeying for encryption key

[Tuong L. <tuo...@de...>]

Rekeying is required for security since a key is less secure when using
for a long time. Also, key will be detached when its nonce value (or
seqno ...) is exhausted. We now make the rekeying process automatic and
configurable by user.

Basically, TIPC will at a specific interval generate a new key by using
the kernel 'Random Number Generator' cipher, then attach it as the node
TX key and securely distribute to others in the cluster as RX keys (-
the key exchange). The automatic key switching will then take over, and
make the new key active shortly. Afterwards, the traffic from this node
will be encrypted with the new session key. The same can happen in peer
nodes but not necessarily at the same time.

For simplicity, the automatically generated key will be initiated as a
per node key. It is not too hard to also support a cluster key rekeying
(e.g. a given node will generate a unique cluster key and update to the
others in the cluster...), but that doesn't bring much benefit, while a
per-node key is even more security.

We also enable user to force a rekeying or change the rekeying interval
via netlink, the new 'set key' command option: 'TIPC_NLA_NODE_REKEYING'
is added for these purposes as follows:
- A value >= 1 will be set as the rekeying interval (in minutes);
- A value of 0 will disable the rekeying;
- A value of 'TIPC_REKEYING_NOW' (~0) will force an immediate rekeying;

The default rekeying interval is (60 * 24) minutes i.e. done every day.
There isn't any restriction for the value but user shouldn't set it too
small or too large which results in an "ineffective" rekeying (thats ok
for testing though).

Signed-off-by: Tuong Lien <tuo...@de...>
---
 include/uapi/linux/tipc.h         |   2 +
 include/uapi/linux/tipc_netlink.h |   1 +
 net/tipc/crypto.c                 | 119 +++++++++++++++++++++++++++++++++++++-
 net/tipc/crypto.h                 |   2 +
 net/tipc/netlink.c                |   1 +
 net/tipc/node.c                   |  28 ++++++++-
 6 files changed, 148 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/tipc.h b/include/uapi/linux/tipc.h
index add01db1daef..80ea15e12113 100644
--- a/include/uapi/linux/tipc.h
+++ b/include/uapi/linux/tipc.h
@@ -254,6 +254,8 @@ static inline int tipc_aead_key_size(struct tipc_aead_key *key)
 	return sizeof(*key) + key->keylen;
 }
 
+#define TIPC_REKEYING_NOW		(~0U)
+
 /* The macros and functions below are deprecated:
  */
 
diff --git a/include/uapi/linux/tipc_netlink.h b/include/uapi/linux/tipc_netlink.h
index d484baa9d365..d847dd671d79 100644
--- a/include/uapi/linux/tipc_netlink.h
+++ b/include/uapi/linux/tipc_netlink.h
@@ -166,6 +166,7 @@ enum {
 	TIPC_NLA_NODE_ID,		/* data */
 	TIPC_NLA_NODE_KEY,		/* data */
 	TIPC_NLA_NODE_KEY_MASTER,	/* flag */
+	TIPC_NLA_NODE_REKEYING,		/* u32 */
 
 	__TIPC_NLA_NODE_MAX,
 	TIPC_NLA_NODE_MAX = __TIPC_NLA_NODE_MAX - 1
diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index 9d00555ede83..d497585010ee 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -36,6 +36,7 @@
 
 #include <crypto/aead.h>
 #include <crypto/aes.h>
+#include <crypto/rng.h>
 #include "crypto.h"
 #include "msg.h"
 #include "bcast.h"
@@ -48,6 +49,8 @@
 #define TIPC_MAX_TFMS_DEF	10
 #define TIPC_MAX_TFMS_LIM	1000
 
+#define TIPC_REKEYING_INTV_DEF	(60 * 24) /* default: 1 day */
+
 /**
  * TIPC Key ids
  */
@@ -181,6 +184,7 @@ struct tipc_crypto_stats {
  * @wq: common workqueue on TX crypto
  * @work: delayed work sched for TX/RX
  * @key_distr: key distributing state
+ * @rekeying_intv: rekeying interval (in minutes)
  * @stats: the crypto statistics
  * @name: the crypto name
  * @sndnxt: the per-peer sndnxt (TX)
@@ -206,6 +210,7 @@ struct tipc_crypto {
 #define KEY_DISTR_SCHED		1
 #define KEY_DISTR_COMPL		2
 	atomic_t key_distr;
+	u32 rekeying_intv;
 
 	struct tipc_crypto_stats __percpu *stats;
 	char name[48];
@@ -294,7 +299,9 @@ static char *tipc_key_change_dump(struct tipc_key old, struct tipc_key new,
 static int tipc_crypto_key_xmit(struct net *net, struct tipc_aead_key *skey,
 				u16 gen, u8 mode, u32 dnode);
 static bool tipc_crypto_key_rcv(struct tipc_crypto *rx, struct tipc_msg *hdr);
+static void tipc_crypto_work_tx(struct work_struct *work);
 static void tipc_crypto_work_rx(struct work_struct *work);
+static int tipc_aead_key_generate(struct tipc_aead_key *skey);
 
 #define is_tx(crypto) (!(crypto)->node)
 #define is_rx(crypto) (!is_tx(crypto))
@@ -342,6 +349,27 @@ int tipc_aead_key_validate(struct tipc_aead_key *ukey)
 	return 0;
 }
 
+/**
+ * tipc_aead_key_generate - Generate new session key
+ * @skey: input/output key with new content
+ *
+ * Return: 0 in case of success, otherwise < 0
+ */
+static int tipc_aead_key_generate(struct tipc_aead_key *skey)
+{
+	int rc = 0;
+
+	/* Fill the key's content with a random value via RNG cipher */
+	rc = crypto_get_default_rng();
+	if (likely(!rc)) {
+		rc = crypto_rng_get_bytes(crypto_default_rng, skey->key,
+					  skey->keylen);
+		crypto_put_default_rng();
+	}
+
+	return rc;
+}
+
 static struct tipc_aead *tipc_aead_get(struct tipc_aead __rcu *aead)
 {
 	struct tipc_aead *tmp;
@@ -1471,13 +1499,16 @@ int tipc_crypto_start(struct tipc_crypto **crypto, struct net *net,
 	atomic64_set(&c->sndnxt, 0);
 	c->timer1 = jiffies;
 	c->timer2 = jiffies;
+	c->rekeying_intv = TIPC_REKEYING_INTV_DEF;
 	spin_lock_init(&c->lock);
 	scnprintf(c->name, 48, "%s(%s)", (is_rx(c)) ? "RX" : "TX",
 		  (is_rx(c)) ? tipc_node_get_id_str(c->node) :
 			       tipc_own_id_string(c->net));
 
-        if (is_rx(c))
-                INIT_DELAYED_WORK(&c->work, tipc_crypto_work_rx);
+	if (is_rx(c))
+		INIT_DELAYED_WORK(&c->work, tipc_crypto_work_rx);
+	else
+		INIT_DELAYED_WORK(&c->work, tipc_crypto_work_tx);
 
 	*crypto = c;
 	return 0;
@@ -1492,8 +1523,11 @@ void tipc_crypto_stop(struct tipc_crypto **crypto)
 		return;
 
 	/* Flush any queued works & destroy wq */
-	if (is_tx(c))
+	if (is_tx(c)) {
+		c->rekeying_intv = 0;
+		cancel_delayed_work_sync(&c->work);
 		destroy_workqueue(c->wq);
+	}
 
 	/* Release AEAD keys */
 	rcu_read_lock();
@@ -2346,3 +2380,82 @@ static void tipc_crypto_work_rx(struct work_struct *work)
 
 	tipc_node_put(rx->node);
 }
+
+/**
+ * tipc_crypto_rekeying_sched - (Re)schedule rekeying w/o new interval
+ * @tx: TX crypto
+ * @changed: if the rekeying needs to be rescheduled with new interval
+ * @new_intv: new rekeying interval (when "changed" = true)
+ */
+void tipc_crypto_rekeying_sched(struct tipc_crypto *tx, bool changed,
+				u32 new_intv)
+{
+	unsigned long delay;
+	bool now = false;
+
+	if (changed) {
+		if (new_intv == TIPC_REKEYING_NOW)
+			now = true;
+		else
+			tx->rekeying_intv = new_intv;
+		cancel_delayed_work_sync(&tx->work);
+	}
+
+	if (tx->rekeying_intv || now) {
+		delay = (now) ? 0 : tx->rekeying_intv * 60 * 1000;
+		queue_delayed_work(tx->wq, &tx->work, msecs_to_jiffies(delay));
+	}
+}
+
+/**
+ * tipc_crypto_work_tx - Scheduled TX works handler
+ * @work: the struct TX work
+ *
+ * The function processes the previous scheduled work, i.e. key rekeying, by
+ * generating a new session key based on current one, then attaching it to the
+ * TX crypto and finally distributing it to peers. It also re-schedules the
+ * rekeying if needed.
+ */
+static void tipc_crypto_work_tx(struct work_struct *work)
+{
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct tipc_crypto *tx = container_of(dwork, struct tipc_crypto, work);
+	struct tipc_aead_key *skey = NULL;
+	struct tipc_key key = tx->key;
+	struct tipc_aead *aead;
+	int rc = -ENOMEM;
+
+	if (unlikely(key.pending))
+		goto resched;
+
+	/* Take current key as a template */
+	rcu_read_lock();
+	aead = rcu_dereference(tx->aead[key.active ?: KEY_MASTER]);
+	if (unlikely(!aead)) {
+		rcu_read_unlock();
+		/* At least one key should exist for securing */
+		return;
+	}
+
+	/* Lets duplicate it first */
+	skey = kmemdup(aead->key, tipc_aead_key_size(aead->key), GFP_ATOMIC);
+	rcu_read_unlock();
+
+	/* Now, generate new key, initiate & distribute it */
+	if (likely(skey)) {
+		rc = tipc_aead_key_generate(skey) ?:
+		     tipc_crypto_key_init(tx, skey, PER_NODE_KEY, false);
+		if (likely(rc > 0))
+			rc = tipc_crypto_key_distr(tx, rc, NULL);
+		kzfree(skey);
+	}
+
+	if (likely(!rc))
+		pr_info("%s: rekeying has been done\n", tx->name);
+	else
+		pr_warn_ratelimited("%s: rekeying returns %d\n", tx->name, rc);
+
+resched:
+	/* Re-schedule rekeying if any */
+	tipc_crypto_rekeying_sched(tx, false, 0);
+}
diff --git a/net/tipc/crypto.h b/net/tipc/crypto.h
index 70bda3d7e174..e1f4e8fb5c10 100644
--- a/net/tipc/crypto.h
+++ b/net/tipc/crypto.h
@@ -171,6 +171,8 @@ void tipc_crypto_key_flush(struct tipc_crypto *c);
 int tipc_crypto_key_distr(struct tipc_crypto *tx, u8 key,
 			  struct tipc_node *dest);
 void tipc_crypto_msg_rcv(struct net *net, struct sk_buff *skb);
+void tipc_crypto_rekeying_sched(struct tipc_crypto *tx, bool changed,
+				u32 new_intv);
 int tipc_aead_key_validate(struct tipc_aead_key *ukey);
 bool tipc_ehdr_validate(struct sk_buff *skb);
 
diff --git a/net/tipc/netlink.c b/net/tipc/netlink.c
index 1ec00fcc26ee..c447cb5f879e 100644
--- a/net/tipc/netlink.c
+++ b/net/tipc/netlink.c
@@ -109,6 +109,7 @@ const struct nla_policy tipc_nl_node_policy[TIPC_NLA_NODE_MAX + 1] = {
 	[TIPC_NLA_NODE_KEY]		= { .type = NLA_BINARY,
 					    .len = TIPC_AEAD_KEY_SIZE_MAX},
 	[TIPC_NLA_NODE_KEY_MASTER]	= { .type = NLA_FLAG },
+	[TIPC_NLA_NODE_REKEYING]	= { .type = NLA_U32 },
 };
 
 /* Properties valid for media, bearer and link */
diff --git a/net/tipc/node.c b/net/tipc/node.c
index 186951b690d2..95f045cf9f55 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -2877,6 +2877,17 @@ static int tipc_nl_retrieve_nodeid(struct nlattr **attrs, u8 **node_id)
 	return 0;
 }
 
+static int tipc_nl_retrieve_rekeying(struct nlattr **attrs, u32 *intv)
+{
+	struct nlattr *attr = attrs[TIPC_NLA_NODE_REKEYING];
+
+	if (!attr)
+		return -ENODATA;
+
+	*intv = nla_get_u32(attr);
+	return 0;
+}
+
 static int __tipc_nl_node_set_key(struct sk_buff *skb, struct genl_info *info)
 {
 	struct nlattr *attrs[TIPC_NLA_NODE_MAX + 1];
@@ -2884,8 +2895,9 @@ static int __tipc_nl_node_set_key(struct sk_buff *skb, struct genl_info *info)
 	struct tipc_crypto *tx = tipc_net(net)->crypto_tx, *c = tx;
 	struct tipc_node *n = NULL;
 	struct tipc_aead_key *ukey;
-	bool master_key = false;
+	bool rekeying = true, master_key = false;
 	u8 *id, *own_id, mode;
+	u32 intv = 0;
 	int rc = 0;
 
 	if (!info->attrs[TIPC_NLA_NODE])
@@ -2901,9 +2913,17 @@ static int __tipc_nl_node_set_key(struct sk_buff *skb, struct genl_info *info)
 	if (!own_id)
 		return -EPERM;
 
+	rc = tipc_nl_retrieve_rekeying(attrs, &intv);
+	if (rc == -ENODATA)
+		rekeying = false;
+
 	rc = tipc_nl_retrieve_key(attrs, &ukey);
-	if (rc)
+	if (rc == -ENODATA && rekeying) {
+		rc = 0;
+		goto rekeying;
+	} else if (rc) {
 		return rc;
+	}
 
 	rc = tipc_aead_key_validate(ukey);
 	if (rc)
@@ -2938,6 +2958,10 @@ static int __tipc_nl_node_set_key(struct sk_buff *skb, struct genl_info *info)
 	if (!master_key)
 		tipc_crypto_key_distr(tx, rc, NULL);
 
+rekeying:
+	/* Schedule TX rekeying if needed */
+	tipc_crypto_rekeying_sched(tx, rekeying, intv);
+
 exit:
 	if (n)
 		tipc_node_put(n);
-- 
2.13.7

[tipc-discussion] [PATCH RFC 4/5] tipc: add automatic session key exchange

[Tuong L. <tuo...@de...>]

With support from the master key option in the previous commit, we have
the ability to exchange keys between the authenticated nodes in cluster
easily. Basically, there are two situations where the key exchange will
take in place:

- When a new node joins the cluster (with the master key), it will need
  to get its peer's TX key, so that be able to decrypt further messages
  from that peer.

- When a new session key is generated (by either user manual setting or
  later automatic rekeying feature), the key will be distributed to all
  peer nodes in the cluster.

A key to be exchanged is encapsulated in the data part of a 'MSG_CRYPTO
/KEY_DISTR_MSG' TIPC v2 message, then xmit-ed as usual and encrypted by
using the master key before sending out. Upon receipt of the message it
will be decrypted in the same way as regular messages, then attached as
the sender's RX key in the receiver node.

In this way, the key exchange is reliable by the link layer, as well as
security, integrity and authenticity by the crypto layer.

Also, the forward security will be easily achieved by user changing the
master key actively but not required very frequently.

The key exchange feature is independent to the master key (but required
when a new node joins the cluster). Also the feature can be turned off/
on via the sysfs: 'net/tipc/key_exchange_enabled' (default 1: enabled).

Signed-off-by: Tuong Lien <tuo...@de...>
---
 net/tipc/crypto.c | 359 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 net/tipc/crypto.h |  24 ++++
 net/tipc/link.c   |   5 +
 net/tipc/msg.h    |   6 +
 net/tipc/node.c   |  19 ++-
 net/tipc/node.h   |   2 +
 net/tipc/sysctl.c |   9 ++
 7 files changed, 404 insertions(+), 20 deletions(-)

diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index 7c688cd0b13e..9d00555ede83 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -37,6 +37,8 @@
 #include <crypto/aead.h>
 #include <crypto/aes.h>
 #include "crypto.h"
+#include "msg.h"
+#include "bcast.h"
 
 #define TIPC_TX_GRACE_PERIOD	msecs_to_jiffies(5000) /* 5s */
 #define TIPC_TX_LASTING_TIME	msecs_to_jiffies(10000) /* 10s */
@@ -82,6 +84,8 @@ static const char *hstats[MAX_STATS] = {"ok", "nok", "async", "async_ok",
 
 /* Max TFMs number per key */
 int sysctl_tipc_max_tfms __read_mostly = TIPC_MAX_TFMS_DEF;
+/* Key exchange switch, default: on */
+int sysctl_tipc_key_exchange_enabled __read_mostly = 1;
 
 /**
  * struct tipc_key - TIPC keys' status indicator
@@ -133,6 +137,8 @@ struct tipc_tfm {
  * @mode: crypto mode is applied to the key
  * @hint[]: a hint for user key
  * @rcu: struct rcu_head
+ * @key: the aead key
+ * @gen: the key's generation
  * @seqno: the key seqno (cluster scope)
  * @refcnt: the key reference counter
  */
@@ -147,6 +153,8 @@ struct tipc_aead {
 	u8 mode;
 	char hint[2 * TIPC_AEAD_HINT_LEN + 1];
 	struct rcu_head rcu;
+	struct tipc_aead_key *key;
+	u16 gen;
 
 	atomic64_t seqno ____cacheline_aligned;
 	refcount_t refcnt ____cacheline_aligned;
@@ -166,7 +174,13 @@ struct tipc_crypto_stats {
  * @node: TIPC node (RX)
  * @aead: array of pointers to AEAD keys for encryption/decryption
  * @peer_rx_active: replicated peer RX active key index
+ * @key_gen: TX/RX key generation
  * @key: the key states
+ * @skey_mode: session key's mode
+ * @skey: received session key
+ * @wq: common workqueue on TX crypto
+ * @work: delayed work sched for TX/RX
+ * @key_distr: key distributing state
  * @stats: the crypto statistics
  * @name: the crypto name
  * @sndnxt: the per-peer sndnxt (TX)
@@ -175,6 +189,7 @@ struct tipc_crypto_stats {
  * @working: the crypto is working or not
  * @key_master: flag indicates if master key exists
  * @legacy_user: flag indicates if a peer joins w/o master key (for bwd comp.)
+ * @nokey: no key indication
  * @lock: tipc_key lock
  */
 struct tipc_crypto {
@@ -182,7 +197,16 @@ struct tipc_crypto {
 	struct tipc_node *node;
 	struct tipc_aead __rcu *aead[KEY_MAX + 1];
 	atomic_t peer_rx_active;
+	u16 key_gen;
 	struct tipc_key key;
+	u8 skey_mode;
+	struct tipc_aead_key *skey;
+	struct workqueue_struct *wq;
+	struct delayed_work work;
+#define KEY_DISTR_SCHED		1
+#define KEY_DISTR_COMPL		2
+	atomic_t key_distr;
+
 	struct tipc_crypto_stats __percpu *stats;
 	char name[48];
 
@@ -194,6 +218,7 @@ struct tipc_crypto {
 			u8 working:1;
 			u8 key_master:1;
 			u8 legacy_user:1;
+			u8 nokey: 1;
 		};
 		u8 flags;
 	};
@@ -266,6 +291,11 @@ static void tipc_crypto_do_cmd(struct net *net, int cmd);
 static char *tipc_crypto_key_dump(struct tipc_crypto *c, char *buf);
 static char *tipc_key_change_dump(struct tipc_key old, struct tipc_key new,
 				  char *buf);
+static int tipc_crypto_key_xmit(struct net *net, struct tipc_aead_key *skey,
+				u16 gen, u8 mode, u32 dnode);
+static bool tipc_crypto_key_rcv(struct tipc_crypto *rx, struct tipc_msg *hdr);
+static void tipc_crypto_work_rx(struct work_struct *work);
+
 #define is_tx(crypto) (!(crypto)->node)
 #define is_rx(crypto) (!is_tx(crypto))
 
@@ -356,6 +386,7 @@ static void tipc_aead_free(struct rcu_head *rp)
 		kfree(head);
 	}
 	free_percpu(aead->tfm_entry);
+	kzfree(aead->key);
 	kfree(aead);
 }
 
@@ -526,6 +557,7 @@ static int tipc_aead_init(struct tipc_aead **aead, struct tipc_aead_key *ukey,
 	tmp->mode = mode;
 	tmp->cloned = NULL;
 	tmp->authsize = TIPC_AES_GCM_TAG_SIZE;
+	tmp->key = kmemdup(ukey, tipc_aead_key_size(ukey), GFP_KERNEL);
 	memcpy(&tmp->salt, ukey->key + keylen, TIPC_AES_GCM_SALT_SIZE);
 	atomic_set(&tmp->users, 0);
 	atomic64_set(&tmp->seqno, 0);
@@ -1005,7 +1037,7 @@ static int tipc_ehdr_build(struct net *net, struct tipc_aead *aead,
 	ehdr->tx_key = tx_key;
 	ehdr->destined = (__rx) ? 1 : 0;
 	ehdr->rx_key_active = (__rx) ? __rx->key.active : 0;
-	ehdr->rx_nokey = (__rx) ? !__rx->key.keys : 0;
+	ehdr->rx_nokey = (__rx) ? __rx->nokey : 0;
 	ehdr->master_key = aead->crypto->key_master;
 	ehdr->reserved_1 = 0;
 	ehdr->reserved_2 = 0;
@@ -1130,11 +1162,13 @@ static int tipc_crypto_key_attach(struct tipc_crypto *c,
 
 attach:
 	aead->crypto = c;
+	aead->gen = (is_tx(c)) ? ++c->key_gen : c->key_gen;
 	tipc_aead_rcu_replace(c->aead[new_key], aead, &c->lock);
 	if (likely(c->key.keys != key.keys))
 		tipc_crypto_key_set_state(c, key.passive, key.active,
 					  key.pending);
 	c->working = 1;
+	c->nokey = 0;
 	c->key_master |= master_key;
 	rc = new_key;
 
@@ -1145,14 +1179,33 @@ static int tipc_crypto_key_attach(struct tipc_crypto *c,
 
 void tipc_crypto_key_flush(struct tipc_crypto *c)
 {
+	struct tipc_crypto *tx, *rx;
 	int k;
 
 	spin_lock_bh(&c->lock);
+	if (is_rx(c)) {
+		/* Try to cancel pending work */
+		rx = c;
+		tx = tipc_net(rx->net)->crypto_tx;
+		if (cancel_delayed_work(&rx->work)) {
+			kfree(rx->skey);
+			rx->skey = NULL;
+			atomic_xchg(&rx->key_distr, 0);
+			tipc_node_put(rx->node);
+		}
+		/* RX stopping => decrease TX key users if any */
+		k = atomic_xchg(&rx->peer_rx_active, 0);
+		if (k) {
+			tipc_aead_users_dec(tx->aead[k], 0);
+			/* Mark the point TX key users changed */
+			tx->timer1 = jiffies;
+		}
+	}
+
 	c->flags = 0;
 	tipc_crypto_key_set_state(c, 0, 0, 0);
 	for (k = KEY_MIN; k <= KEY_MAX; k++)
 		tipc_crypto_key_detach(c->aead[k], &c->lock);
-	atomic_set(&c->peer_rx_active, 0);
 	atomic64_set(&c->sndnxt, 0);
 	spin_unlock_bh(&c->lock);
 }
@@ -1298,7 +1351,8 @@ static struct tipc_aead *tipc_crypto_key_pick_tx(struct tipc_crypto *tx,
  * decreased correspondingly.
  *
  * It also considers if peer has no key, then we need to make own master key
- * (if any) taking over i.e. starting grace period.
+ * (if any) taking over i.e. starting grace period and also trigger key
+ * distributing process.
  *
  * The "per-peer" sndnxt is also reset when the peer key has switched.
  */
@@ -1309,6 +1363,7 @@ static void tipc_crypto_key_synch(struct tipc_crypto *rx, struct sk_buff *skb)
 	struct tipc_msg *hdr = buf_msg(skb);
 	u32 self = tipc_own_addr(rx->net);
 	u8 cur, new;
+	unsigned long delay;
 
 	/* Update RX 'key_master' flag according to peer, also mark "legacy" if
 	 * a peer has no master key.
@@ -1322,9 +1377,22 @@ static void tipc_crypto_key_synch(struct tipc_crypto *rx, struct sk_buff *skb)
 		return;
 
 	/* Case 1: Peer has no keys, let's make master key taking over */
-	if (ehdr->rx_nokey)
+	if (ehdr->rx_nokey) {
 		/* Set or extend grace period */
 		tx->timer2 = jiffies;
+		/* Schedule key distributing for the peer if not yet */
+		if (tx->key.keys &&
+		    !atomic_cmpxchg(&rx->key_distr, 0, KEY_DISTR_SCHED)) {
+			get_random_bytes(&delay, 2);
+			delay %= 5;
+			delay = msecs_to_jiffies(500 * ++delay);
+			if (queue_delayed_work(tx->wq, &rx->work, delay))
+				tipc_node_get(rx->node);
+		}
+	} else {
+		/* Cancel a pending key distributing if any */
+		atomic_xchg(&rx->key_distr, 0);
+	}
 
 	/* Case 2: Peer RX active key has changed, let's update own TX users */
 	cur = atomic_read(&rx->peer_rx_active);
@@ -1377,6 +1445,15 @@ int tipc_crypto_start(struct tipc_crypto **crypto, struct net *net,
 	if (!c)
 		return -ENOMEM;
 
+	/* Allocate workqueue on TX */
+	if (!node) {
+		c->wq = alloc_ordered_workqueue("tipc_crypto", 0);
+		if (!c->wq) {
+			kfree(c);
+			return -ENOMEM;
+		}
+	}
+
 	/* Allocate statistic structure */
 	c->stats = alloc_percpu_gfp(struct tipc_crypto_stats, GFP_ATOMIC);
 	if (!c->stats) {
@@ -1387,7 +1464,9 @@ int tipc_crypto_start(struct tipc_crypto **crypto, struct net *net,
 	c->flags = 0;
 	c->net = net;
 	c->node = node;
+	get_random_bytes(&c->key_gen, 2);
 	tipc_crypto_key_set_state(c, 0, 0, 0);
+	atomic_set(&c->key_distr, 0);
 	atomic_set(&c->peer_rx_active, 0);
 	atomic64_set(&c->sndnxt, 0);
 	c->timer1 = jiffies;
@@ -1397,32 +1476,27 @@ int tipc_crypto_start(struct tipc_crypto **crypto, struct net *net,
 		  (is_rx(c)) ? tipc_node_get_id_str(c->node) :
 			       tipc_own_id_string(c->net));
 
+        if (is_rx(c))
+                INIT_DELAYED_WORK(&c->work, tipc_crypto_work_rx);
+
 	*crypto = c;
 	return 0;
 }
 
 void tipc_crypto_stop(struct tipc_crypto **crypto)
 {
-	struct tipc_crypto *c = *crypto, *tx, *rx;
+	struct tipc_crypto *c = *crypto;
 	u8 k;
 
 	if (!c)
 		return;
 
-	rcu_read_lock();
-	/* RX stopping? => decrease TX key users if any */
-	if (is_rx(c)) {
-		rx = c;
-		tx = tipc_net(rx->net)->crypto_tx;
-		k = atomic_read(&rx->peer_rx_active);
-		if (k) {
-			tipc_aead_users_dec(tx->aead[k], 0);
-			/* Mark the point TX key users changed */
-			tx->timer1 = jiffies;
-		}
-	}
+	/* Flush any queued works & destroy wq */
+	if (is_tx(c))
+		destroy_workqueue(c->wq);
 
 	/* Release AEAD keys */
+	rcu_read_lock();
 	for (k = KEY_MIN; k <= KEY_MAX; k++)
 		tipc_aead_put(rcu_dereference(c->aead[k]));
 	rcu_read_unlock();
@@ -1621,6 +1695,7 @@ int tipc_crypto_xmit(struct net *net, struct sk_buff **skb,
 		}
 		if (user == LINK_CONFIG ||
 		    (user == LINK_PROTOCOL && type == RESET_MSG) ||
+		    (user == MSG_CRYPTO && type == KEY_DISTR_MSG) ||
 		    time_before(jiffies, tx->timer2 + TIPC_TX_GRACE_PERIOD)) {
 			if (__rx && __rx->key_master &&
 			    !atomic_read(&__rx->peer_rx_active))
@@ -1705,7 +1780,7 @@ int tipc_crypto_rcv(struct net *net, struct tipc_crypto *rx,
 	struct tipc_aead *aead = NULL;
 	struct tipc_key key;
 	int rc = -ENOKEY;
-	u8 tx_key;
+	u8 tx_key, n;
 
 	tx_key = ((struct tipc_ehdr *)(*skb)->data)->tx_key;
 
@@ -1755,8 +1830,19 @@ int tipc_crypto_rcv(struct net *net, struct tipc_crypto *rx,
 		if (rc == -ENOKEY) {
 			kfree_skb(*skb);
 			*skb = NULL;
-			if (rx)
+			if (rx) {
+				/* Mark rx->nokey only if we dont have a
+				 * pending received session key, nor a newer
+				 * one i.e. in the next slot.
+				 */
+				n = key_next(tx_key);
+				rx->nokey = !(rx->skey ||
+					      rcu_access_pointer(rx->aead[n]));
+				pr_debug_ratelimited("%s: nokey %d, key %d/%x\n",
+						     rx->name, rx->nokey,
+						     tx_key, rx->key.keys);
 				tipc_node_put(rx->node);
+			}
 			this_cpu_inc(stats->stat[STAT_NOKEYS]);
 			return rc;
 		} else if (rc == -EBADMSG) {
@@ -2025,3 +2111,238 @@ static char *tipc_key_change_dump(struct tipc_key old, struct tipc_key new,
 	i += scnprintf(buf + i, 32 - i, "]");
 	return buf;
 }
+
+/**
+ * tipc_crypto_msg_rcv - Common 'MSG_CRYPTO' processing point
+ * @net: the struct net
+ * @skb: the receving message buffer
+ */
+void tipc_crypto_msg_rcv(struct net *net, struct sk_buff *skb)
+{
+	struct tipc_msg *hdr = buf_msg(skb);
+	struct tipc_crypto *rx;
+
+	rx = tipc_node_crypto_rx_by_addr(net, msg_prevnode(hdr));
+	if (unlikely(!rx))
+		goto exit;
+
+	switch (msg_type(hdr)) {
+	case KEY_DISTR_MSG:
+		if (tipc_crypto_key_rcv(rx, hdr))
+			goto exit;
+		break;
+	default:
+		break;
+	}
+
+	tipc_node_put(rx->node);
+
+exit:
+	kfree_skb(skb);
+}
+
+/**
+ * tipc_crypto_key_distr - Distribute a TX key
+ * @tx: the TX crypto
+ * @key: the key's index
+ * @dest: the destination tipc node, = NULL if distributing to all nodes
+ *
+ * Return: 0 in case of success, otherwise < 0
+ */
+int tipc_crypto_key_distr(struct tipc_crypto *tx, u8 key,
+			  struct tipc_node *dest)
+{
+	struct tipc_aead *aead;
+	char *dstr = (dest) ? tipc_node_get_id_str(dest) : "all";
+	u32 dnode = tipc_node_get_addr(dest);
+	int rc = -ENOKEY;
+
+	if (!sysctl_tipc_key_exchange_enabled)
+		return 0;
+
+	if (key) {
+		rcu_read_lock();
+		aead = tipc_aead_get(tx->aead[key]);
+		if (likely(aead)) {
+			rc = tipc_crypto_key_xmit(tx->net, aead->key,
+						  aead->gen, aead->mode,
+						  dnode);
+			tipc_aead_put(aead);
+		}
+		rcu_read_unlock();
+	}
+
+	if (likely(!rc))
+		pr_info("%s: key[%d] is distributed to %s\n",
+			tx->name, key, dstr);
+	else
+		pr_warn("%s: unable to distr key[%d] to %s, err %d\n",
+			tx->name, key, dstr, rc);
+
+	return rc;
+}
+
+/**
+ * tipc_crypto_key_xmit - Send a session key
+ * @net: the struct net
+ * @skey: the session key to be sent
+ * @gen: the key's generation
+ * @mode: the key's mode
+ * @dnode: the destination node address, = 0 if broadcasting to all nodes
+ *
+ * The session key 'skey' is packed in a TIPC v2 'MSG_CRYPTO/KEY_DISTR_MSG'
+ * as its data section, then xmit-ed through the uc/bc link.
+ *
+ * Return: 0 in case of success, otherwise < 0
+ */
+static int tipc_crypto_key_xmit(struct net *net, struct tipc_aead_key *skey,
+				u16 gen, u8 mode, u32 dnode)
+{
+	struct sk_buff_head pkts;
+	struct tipc_msg *hdr;
+	struct sk_buff *skb;
+	u16 size, cong_link_cnt;
+	u8 *data;
+	int rc;
+
+	size = tipc_aead_key_size(skey);
+	skb = tipc_buf_acquire(INT_H_SIZE + size, GFP_ATOMIC);
+	if (!skb)
+		return -ENOMEM;
+
+	hdr = buf_msg(skb);
+	tipc_msg_init(tipc_own_addr(net), hdr, MSG_CRYPTO, KEY_DISTR_MSG,
+		      INT_H_SIZE, dnode);
+	msg_set_size(hdr, INT_H_SIZE + size);
+	msg_set_key_gen(hdr, gen);
+	msg_set_key_mode(hdr, mode);
+
+	data = msg_data(hdr);
+	*((__be32 *)(data + TIPC_AEAD_ALG_NAME)) = htonl(skey->keylen);
+	memcpy(data, skey->alg_name, TIPC_AEAD_ALG_NAME);
+	memcpy(data + TIPC_AEAD_ALG_NAME + sizeof(__be32), skey->key,
+	       skey->keylen);
+
+	__skb_queue_head_init(&pkts);
+	__skb_queue_tail(&pkts, skb);
+	if (dnode)
+		rc = tipc_node_xmit(net, &pkts, dnode, 0);
+	else
+		rc = tipc_bcast_xmit(net, &pkts, &cong_link_cnt);
+
+	return rc;
+}
+
+/**
+ * tipc_crypto_key_rcv - Receive a session key
+ * @rx: the RX crypto
+ * @hdr: the TIPC v2 message incl. the receiving session key in its data
+ *
+ * This function retrieves the session key in the message from peer, then
+ * schedules a RX work to attach the key to the corresponding RX crypto.
+ *
+ * Return: "true" if the key has been scheduled for attaching, otherwise
+ * "false".
+ */
+static bool tipc_crypto_key_rcv(struct tipc_crypto *rx, struct tipc_msg *hdr)
+{
+	struct tipc_crypto *tx = tipc_net(rx->net)->crypto_tx;
+	struct tipc_aead_key *skey = NULL;
+	u16 key_gen = msg_key_gen(hdr);
+	u16 size = msg_data_sz(hdr);
+	u8 *data = msg_data(hdr);
+
+	spin_lock(&rx->lock);
+	if (unlikely(rx->skey || (key_gen == rx->key_gen && rx->key.keys))) {
+		pr_err("%s: key existed <%px>, gen %d vs %d\n", rx->name,
+		       rx->skey, key_gen, rx->key_gen);
+		goto exit;
+	}
+
+	/* Allocate memory for the key */
+	skey = kmalloc(size, GFP_ATOMIC);
+	if (unlikely(!skey)) {
+		pr_err("%s: unable to allocate memory for skey\n", rx->name);
+		goto exit;
+	}
+
+	/* Copy key from msg data */
+	skey->keylen = ntohl(*((__be32*)(data + TIPC_AEAD_ALG_NAME)));
+	memcpy(skey->alg_name, data, TIPC_AEAD_ALG_NAME);
+	memcpy(skey->key, data + TIPC_AEAD_ALG_NAME + sizeof(__be32),
+	       skey->keylen);
+
+	/* Sanity check */
+	if (unlikely(size != tipc_aead_key_size(skey))) {
+		kfree(skey);
+		skey = NULL;
+		goto exit;
+	}
+
+	rx->key_gen = key_gen;
+	rx->skey_mode = msg_key_mode(hdr);
+	rx->skey = skey;
+	rx->nokey = 0;
+	mb();
+
+exit:
+	spin_unlock(&rx->lock);
+
+	/* Schedule the key attaching on this crypto */
+	if (likely(skey && queue_delayed_work(tx->wq, &rx->work, 0)))
+		return true;
+
+	return false;
+}
+
+/**
+ * tipc_crypto_work_rx - Scheduled RX works handler
+ * @work: the struct RX work
+ *
+ * The function processes the previous scheduled works i.e. distributing TX key
+ * or attaching a received session key on RX crypto.
+ */
+static void tipc_crypto_work_rx(struct work_struct *work)
+{
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct tipc_crypto *rx = container_of(dwork, struct tipc_crypto, work);
+	struct tipc_crypto *tx = tipc_net(rx->net)->crypto_tx;
+	unsigned long delay = msecs_to_jiffies(5000);
+	bool resched = false;
+	int rc;
+
+	/* Case 1: Distribute TX key to peer if scheduled */
+	if (atomic_cmpxchg(&rx->key_distr,
+			   KEY_DISTR_SCHED,
+			   KEY_DISTR_COMPL) == KEY_DISTR_SCHED) {
+		/* Always pick the newest one for distributing */
+		tipc_crypto_key_distr(tx,
+				      tx->key.pending ?: tx->key.active,
+				      rx->node);
+		/* Sched for key_distr releasing */
+		resched = true;
+	} else {
+		atomic_cmpxchg(&rx->key_distr, KEY_DISTR_COMPL, 0);
+	}
+
+	/* Case 2: Attach a pending received session key from peer if any */
+	if (rx->skey) {
+		rc = tipc_crypto_key_init(rx, rx->skey, rx->skey_mode, false);
+		switch (rc) {
+		case -EBUSY:
+		case -ENOMEM:
+			/* Resched the key attaching */
+			resched = true;
+			break;
+		default:
+			synchronize_rcu();
+			kfree(rx->skey);
+			rx->skey = NULL;
+		}
+	}
+
+	if (resched && queue_delayed_work(tx->wq, &rx->work, delay))
+		return;
+
+	tipc_node_put(rx->node);
+}
diff --git a/net/tipc/crypto.h b/net/tipc/crypto.h
index 7fcb80cb0e8a..70bda3d7e174 100644
--- a/net/tipc/crypto.h
+++ b/net/tipc/crypto.h
@@ -67,6 +67,7 @@ enum {
 };
 
 extern int sysctl_tipc_max_tfms __read_mostly;
+extern int sysctl_tipc_key_exchange_enabled __read_mostly;
 
 /**
  * TIPC encryption message format:
@@ -167,8 +168,31 @@ int tipc_crypto_rcv(struct net *net, struct tipc_crypto *rx,
 int tipc_crypto_key_init(struct tipc_crypto *c, struct tipc_aead_key *ukey,
 			 u8 mode, bool master_key);
 void tipc_crypto_key_flush(struct tipc_crypto *c);
+int tipc_crypto_key_distr(struct tipc_crypto *tx, u8 key,
+			  struct tipc_node *dest);
+void tipc_crypto_msg_rcv(struct net *net, struct sk_buff *skb);
 int tipc_aead_key_validate(struct tipc_aead_key *ukey);
 bool tipc_ehdr_validate(struct sk_buff *skb);
 
+static inline u32 msg_key_gen(struct tipc_msg *m)
+{
+	return msg_bits(m, 4, 16, 0xffff);
+}
+
+static inline void msg_set_key_gen(struct tipc_msg *m, u32 gen)
+{
+	msg_set_bits(m, 4, 16, 0xffff, gen);
+}
+
+static inline u32 msg_key_mode(struct tipc_msg *m)
+{
+	return msg_bits(m, 4, 0, 0xf);
+}
+
+static inline void msg_set_key_mode(struct tipc_msg *m, u32 mode)
+{
+	msg_set_bits(m, 4, 0, 0xf, mode);
+}
+
 #endif /* _TIPC_CRYPTO_H */
 #endif
diff --git a/net/tipc/link.c b/net/tipc/link.c
index 1c579357ccdf..9142e264f020 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1244,6 +1244,11 @@ static bool tipc_data_input(struct tipc_link *l, struct sk_buff *skb,
 	case MSG_FRAGMENTER:
 	case BCAST_PROTOCOL:
 		return false;
+#ifdef CONFIG_TIPC_CRYPTO
+	case MSG_CRYPTO:
+		tipc_crypto_msg_rcv(l->net, skb);
+		return true;
+#endif
 	default:
 		pr_warn("Dropping received illegal msg type\n");
 		kfree_skb(skb);
diff --git a/net/tipc/msg.h b/net/tipc/msg.h
index 25e5c5c8a6ff..2b514c600c2d 100644
--- a/net/tipc/msg.h
+++ b/net/tipc/msg.h
@@ -82,6 +82,7 @@ struct plist;
 #define  NAME_DISTRIBUTOR     11
 #define  MSG_FRAGMENTER       12
 #define  LINK_CONFIG          13
+#define  MSG_CRYPTO           14
 #define  SOCK_WAKEUP          14       /* pseudo user */
 #define  TOP_SRV              15       /* pseudo user */
 
@@ -750,6 +751,11 @@ static inline void msg_set_nameupper(struct tipc_msg *m, u32 n)
 #define GRP_REMIT_MSG        5
 
 /*
+ * Crypto message types
+ */
+#define KEY_DISTR_MSG		0
+
+/*
  * Word 1
  */
 static inline u32 msg_seq_gap(struct tipc_msg *m)
diff --git a/net/tipc/node.c b/net/tipc/node.c
index 55f012d1ea74..186951b690d2 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -278,6 +278,14 @@ struct tipc_crypto *tipc_node_crypto_rx_by_list(struct list_head *pos)
 {
 	return container_of(pos, struct tipc_node, list)->crypto_rx;
 }
+
+struct tipc_crypto *tipc_node_crypto_rx_by_addr(struct net *net, u32 addr)
+{
+	struct tipc_node *n;
+
+	n = tipc_node_find(net, addr);
+	return (n) ? n->crypto_rx : NULL;
+}
 #endif
 
 static void tipc_node_free(struct rcu_head *rp)
@@ -303,7 +311,7 @@ void tipc_node_put(struct tipc_node *node)
 	kref_put(&node->kref, tipc_node_kref_release);
 }
 
-static void tipc_node_get(struct tipc_node *node)
+void tipc_node_get(struct tipc_node *node)
 {
 	kref_get(&node->kref);
 }
@@ -584,6 +592,7 @@ static void tipc_node_calculate_timer(struct tipc_node *n, struct tipc_link *l)
 
 static void tipc_node_delete_from_list(struct tipc_node *node)
 {
+	tipc_crypto_key_flush(node->crypto_rx);
 	list_del_rcu(&node->list);
 	hlist_del_rcu(&node->hash);
 	tipc_node_put(node);
@@ -2922,6 +2931,14 @@ static int __tipc_nl_node_set_key(struct sk_buff *skb, struct genl_info *info)
 
 	/* Initiate the TX/RX key */
 	rc = tipc_crypto_key_init(c, ukey, mode, master_key);
+	if (rc < 0 || c != tx)
+		goto exit;
+
+	/* Distribute TX key but not master one */
+	if (!master_key)
+		tipc_crypto_key_distr(tx, rc, NULL);
+
+exit:
 	if (n)
 		tipc_node_put(n);
 
diff --git a/net/tipc/node.h b/net/tipc/node.h
index 9f6f13f1604f..154a5bbb0d29 100644
--- a/net/tipc/node.h
+++ b/net/tipc/node.h
@@ -79,12 +79,14 @@ bool tipc_node_get_id(struct net *net, u32 addr, u8 *id);
 u32 tipc_node_get_addr(struct tipc_node *node);
 char *tipc_node_get_id_str(struct tipc_node *node);
 void tipc_node_put(struct tipc_node *node);
+void tipc_node_get(struct tipc_node *node);
 struct tipc_node *tipc_node_create(struct net *net, u32 addr, u8 *peer_id,
 				   u16 capabilities, u32 hash_mixes,
 				   bool preliminary);
 #ifdef CONFIG_TIPC_CRYPTO
 struct tipc_crypto *tipc_node_crypto_rx(struct tipc_node *__n);
 struct tipc_crypto *tipc_node_crypto_rx_by_list(struct list_head *pos);
+struct tipc_crypto *tipc_node_crypto_rx_by_addr(struct net *net, u32 addr);
 #endif
 u32 tipc_node_try_addr(struct net *net, u8 *id, u32 addr);
 void tipc_node_check_dest(struct net *net, u32 onode, u8 *peer_id128,
diff --git a/net/tipc/sysctl.c b/net/tipc/sysctl.c
index 97a6264a2993..9fb65c988f7f 100644
--- a/net/tipc/sysctl.c
+++ b/net/tipc/sysctl.c
@@ -74,6 +74,15 @@ static struct ctl_table tipc_table[] = {
 		.proc_handler	= proc_dointvec_minmax,
 		.extra1         = SYSCTL_ONE,
 	},
+	{
+		.procname	= "key_exchange_enabled",
+		.data		= &sysctl_tipc_key_exchange_enabled,
+		.maxlen		= sizeof(sysctl_tipc_key_exchange_enabled),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1         = SYSCTL_ZERO,
+		.extra2         = SYSCTL_ONE,
+	},
 #endif
 	{
 		.procname	= "bc_retruni",
-- 
2.13.7

[tipc-discussion] [PATCH RFC 3/5] tipc: introduce encryption master key

[Tuong L. <tuo...@de...>]

In addition to the supported cluster & per-node encryption keys for the
en/decryption of TIPC messages, we now introduce one option for user to
set a cluster key as 'master key', which is simply a symmetric key like
the former but is a long-term one for the sake of:

- Encryption of subordinate keys as is precondition for later automatic
  key exchange feature.

- Rekeying, where a new node having no knowledge of current session
  keys in the existing ones will be able to join the cluster as long as
  authenticated with the master key by user.

Particularly we utilize the unused slot 0 in the key array to which the
master key is attached and in-use instantly once user sets it.

Also, when present, the master key will be used to encrypt all *vital*
message users such as the 'LINK_CONFIG' or discovery messages sent from
a node, thus ensuring the authenticity of a new node before it can join
the cluster (i.e. acting as an 'authentication key' as well).

Besides, after joining, a node with only the master key should be fully
communicable to existing nodes in the cluster, although those nodes may
have their own session keys activated (i.e. not the master one). To
support this, we simply define a 'grace period', starting from the time
a node itself reports having no RX keys, so the existing nodes will use
the master key for encryption instead. The grace period can be extended
but will automatically stop after, e.g. 5 seconds without a new report.
This is also the basis for later key exchanging since the new node will
be impossible to decrypt anything without the support from master key.

For user to set a master key, we define a new netlink flag -
'TIPC_NLA_NODE_KEY_MASTER', so it can be added to the current 'set key'
netlink command to specify the setting key to be a master key.

Above all, the traditional cluster/per-node key mechanism is guaranteed
to work when user comes not to use this master key option. This is also
compatible to legacy nodes without the feature supported.

Signed-off-by: Tuong Lien <tuo...@de...>
---
 include/uapi/linux/tipc_netlink.h |   1 +
 net/tipc/crypto.c                 | 206 ++++++++++++++++++++++++++++----------
 net/tipc/crypto.h                 |  15 ++-
 net/tipc/msg.h                    |   4 +-
 net/tipc/netlink.c                |   1 +
 net/tipc/node.c                   |  46 ++++-----
 6 files changed, 189 insertions(+), 84 deletions(-)

diff --git a/include/uapi/linux/tipc_netlink.h b/include/uapi/linux/tipc_netlink.h
index dc0d23a50e69..d484baa9d365 100644
--- a/include/uapi/linux/tipc_netlink.h
+++ b/include/uapi/linux/tipc_netlink.h
@@ -165,6 +165,7 @@ enum {
 	TIPC_NLA_NODE_UP,		/* flag */
 	TIPC_NLA_NODE_ID,		/* data */
 	TIPC_NLA_NODE_KEY,		/* data */
+	TIPC_NLA_NODE_KEY_MASTER,	/* flag */
 
 	__TIPC_NLA_NODE_MAX,
 	TIPC_NLA_NODE_MAX = __TIPC_NLA_NODE_MAX - 1
diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index f1046ab4bd01..7c688cd0b13e 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -38,6 +38,7 @@
 #include <crypto/aes.h>
 #include "crypto.h"
 
+#define TIPC_TX_GRACE_PERIOD	msecs_to_jiffies(5000) /* 5s */
 #define TIPC_TX_LASTING_TIME	msecs_to_jiffies(10000) /* 10s */
 #define TIPC_RX_ACTIVE_LIM	msecs_to_jiffies(3000) /* 3s */
 #define TIPC_RX_PASSIVE_LIM	msecs_to_jiffies(15000) /* 15s */
@@ -49,9 +50,9 @@
  * TIPC Key ids
  */
 enum {
-	KEY_UNUSED = 0,
-	KEY_MIN,
-	KEY_1 = KEY_MIN,
+	KEY_MASTER = 0,
+	KEY_MIN = KEY_MASTER,
+	KEY_1 = 1,
 	KEY_2,
 	KEY_3,
 	KEY_MAX = KEY_3,
@@ -166,27 +167,36 @@ struct tipc_crypto_stats {
  * @aead: array of pointers to AEAD keys for encryption/decryption
  * @peer_rx_active: replicated peer RX active key index
  * @key: the key states
- * @working: the crypto is working or not
  * @stats: the crypto statistics
  * @name: the crypto name
  * @sndnxt: the per-peer sndnxt (TX)
  * @timer1: general timer 1 (jiffies)
  * @timer2: general timer 2 (jiffies)
+ * @working: the crypto is working or not
+ * @key_master: flag indicates if master key exists
+ * @legacy_user: flag indicates if a peer joins w/o master key (for bwd comp.)
  * @lock: tipc_key lock
  */
 struct tipc_crypto {
 	struct net *net;
 	struct tipc_node *node;
-	struct tipc_aead __rcu *aead[KEY_MAX + 1]; /* key[0] is UNUSED */
+	struct tipc_aead __rcu *aead[KEY_MAX + 1];
 	atomic_t peer_rx_active;
 	struct tipc_key key;
-	u8 working:1;
 	struct tipc_crypto_stats __percpu *stats;
 	char name[48];
 
 	atomic64_t sndnxt ____cacheline_aligned;
 	unsigned long timer1;
 	unsigned long timer2;
+	union {
+		struct {
+			u8 working:1;
+			u8 key_master:1;
+			u8 legacy_user:1;
+		};
+		u8 flags;
+	};
 	spinlock_t lock; /* crypto lock */
 
 } ____cacheline_aligned;
@@ -236,13 +246,19 @@ static inline void tipc_crypto_key_set_state(struct tipc_crypto *c,
 					     u8 new_active,
 					     u8 new_pending);
 static int tipc_crypto_key_attach(struct tipc_crypto *c,
-				  struct tipc_aead *aead, u8 pos);
+				  struct tipc_aead *aead, u8 pos,
+				  bool master_key);
 static bool tipc_crypto_key_try_align(struct tipc_crypto *rx, u8 new_pending);
 static struct tipc_aead *tipc_crypto_key_pick_tx(struct tipc_crypto *tx,
 						 struct tipc_crypto *rx,
-						 struct sk_buff *skb);
+						 struct sk_buff *skb,
+						 u8 tx_key);
 static void tipc_crypto_key_synch(struct tipc_crypto *rx, struct sk_buff *skb);
 static int tipc_crypto_key_revoke(struct net *net, u8 tx_key);
+static inline void tipc_crypto_clone_msg(struct net *net, struct sk_buff *_skb,
+					 struct tipc_bearer *b,
+					 struct tipc_media_addr *dst,
+					 struct tipc_node *__dnode, u8 type);
 static void tipc_crypto_rcv_complete(struct net *net, struct tipc_aead *aead,
 				     struct tipc_bearer *b,
 				     struct sk_buff **skb, int err);
@@ -937,8 +953,6 @@ bool tipc_ehdr_validate(struct sk_buff *skb)
 		return false;
 	if (unlikely(skb->len <= ehsz + TIPC_AES_GCM_TAG_SIZE))
 		return false;
-	if (unlikely(!ehdr->tx_key))
-		return false;
 
 	return true;
 }
@@ -991,6 +1005,8 @@ static int tipc_ehdr_build(struct net *net, struct tipc_aead *aead,
 	ehdr->tx_key = tx_key;
 	ehdr->destined = (__rx) ? 1 : 0;
 	ehdr->rx_key_active = (__rx) ? __rx->key.active : 0;
+	ehdr->rx_nokey = (__rx) ? !__rx->key.keys : 0;
+	ehdr->master_key = aead->crypto->key_master;
 	ehdr->reserved_1 = 0;
 	ehdr->reserved_2 = 0;
 
@@ -1033,6 +1049,7 @@ static inline void tipc_crypto_key_set_state(struct tipc_crypto *c,
  * @c: TIPC crypto to which new key is attached
  * @ukey: the user key
  * @mode: the key mode (CLUSTER_KEY or PER_NODE_KEY)
+ * @master_key: specify this is a cluster master key
  *
  * A new TIPC AEAD key will be allocated and initiated with the specified user
  * key, then attached to the TIPC crypto.
@@ -1040,7 +1057,7 @@ static inline void tipc_crypto_key_set_state(struct tipc_crypto *c,
  * Return: new key id in case of success, otherwise: < 0
  */
 int tipc_crypto_key_init(struct tipc_crypto *c, struct tipc_aead_key *ukey,
-			 u8 mode)
+			 u8 mode, bool master_key)
 {
 	struct tipc_aead *aead = NULL;
 	int rc = 0;
@@ -1053,7 +1070,7 @@ int tipc_crypto_key_init(struct tipc_crypto *c, struct tipc_aead_key *ukey,
 	}
 
 	/* Attach it to the crypto */
-	rc = tipc_crypto_key_attach(c, aead, 0);
+	rc = tipc_crypto_key_attach(c, aead, 0, master_key);
 	if (rc < 0) {
 		pr_err("%s: unable to attach key, err %d\n", c->name, rc);
 		tipc_aead_free(&aead->rcu);
@@ -1069,11 +1086,13 @@ int tipc_crypto_key_init(struct tipc_crypto *c, struct tipc_aead_key *ukey,
  * @c: TIPC crypto to which the new AEAD key is attached
  * @aead: the new AEAD key pointer
  * @pos: desired slot in the crypto key array, = 0 if any!
+ * @master_key: specify this is a cluster master key
  *
  * Return: new key id in case of success, otherwise: -EBUSY
  */
 static int tipc_crypto_key_attach(struct tipc_crypto *c,
-				  struct tipc_aead *aead, u8 pos)
+				  struct tipc_aead *aead, u8 pos,
+				  bool master_key)
 {
 	struct tipc_key key;
 	int rc = -EBUSY;
@@ -1081,6 +1100,10 @@ static int tipc_crypto_key_attach(struct tipc_crypto *c,
 
 	spin_lock_bh(&c->lock);
 	key = c->key;
+	if (master_key) {
+		new_key = KEY_MASTER;
+		goto attach;
+	}
 	if (key.active && key.passive)
 		goto exit;
 	if (key.pending) {
@@ -1112,8 +1135,7 @@ static int tipc_crypto_key_attach(struct tipc_crypto *c,
 		tipc_crypto_key_set_state(c, key.passive, key.active,
 					  key.pending);
 	c->working = 1;
-	c->timer1 = jiffies;
-	c->timer2 = jiffies;
+	c->key_master |= master_key;
 	rc = new_key;
 
 exit:
@@ -1126,7 +1148,7 @@ void tipc_crypto_key_flush(struct tipc_crypto *c)
 	int k;
 
 	spin_lock_bh(&c->lock);
-	c->working = 0;
+	c->flags = 0;
 	tipc_crypto_key_set_state(c, 0, 0, 0);
 	for (k = KEY_MIN; k <= KEY_MAX; k++)
 		tipc_crypto_key_detach(c->aead[k], &c->lock);
@@ -1202,6 +1224,7 @@ static bool tipc_crypto_key_try_align(struct tipc_crypto *rx, u8 new_pending)
  * @tx: TX crypto handle
  * @rx: RX crypto handle (can be NULL)
  * @skb: the message skb which will be decrypted later
+ * @tx_key: peer TX key id
  *
  * This function looks up the existing TX keys and pick one which is suitable
  * for the message decryption, that must be a cluster key and not used before
@@ -1211,7 +1234,8 @@ static bool tipc_crypto_key_try_align(struct tipc_crypto *rx, u8 new_pending)
  */
 static struct tipc_aead *tipc_crypto_key_pick_tx(struct tipc_crypto *tx,
 						 struct tipc_crypto *rx,
-						 struct sk_buff *skb)
+						 struct sk_buff *skb,
+						 u8 tx_key)
 {
 	struct tipc_skb_cb *skb_cb = TIPC_SKB_CB(skb);
 	struct tipc_aead *aead = NULL;
@@ -1230,6 +1254,10 @@ static struct tipc_aead *tipc_crypto_key_pick_tx(struct tipc_crypto *tx,
 
 	/* Pick one TX key */
 	spin_lock(&tx->lock);
+	if (tx_key == KEY_MASTER) {
+		aead = tipc_aead_rcu_ptr(tx->aead[KEY_MASTER], &tx->lock);
+		goto done;
+	}
 	do {
 		k = (i == 0) ? key.pending :
 			((i == 1) ? key.active : key.passive);
@@ -1249,9 +1277,12 @@ static struct tipc_aead *tipc_crypto_key_pick_tx(struct tipc_crypto *tx,
 		skb->next = skb_clone(skb, GFP_ATOMIC);
 		if (unlikely(!skb->next))
 			pr_warn("Failed to clone skb for next round if any\n");
-		WARN_ON(!refcount_inc_not_zero(&aead->refcnt));
 		break;
 	} while (++i < 3);
+
+done:
+	if (likely(aead))
+		WARN_ON(!refcount_inc_not_zero(&aead->refcnt));
 	spin_unlock(&tx->lock);
 
 	return aead;
@@ -1266,6 +1297,9 @@ static struct tipc_aead *tipc_crypto_key_pick_tx(struct tipc_crypto *tx,
  * has changed, so the number of TX keys' users on this node are increased and
  * decreased correspondingly.
  *
+ * It also considers if peer has no key, then we need to make own master key
+ * (if any) taking over i.e. starting grace period.
+ *
  * The "per-peer" sndnxt is also reset when the peer key has switched.
  */
 static void tipc_crypto_key_synch(struct tipc_crypto *rx, struct sk_buff *skb)
@@ -1276,11 +1310,23 @@ static void tipc_crypto_key_synch(struct tipc_crypto *rx, struct sk_buff *skb)
 	u32 self = tipc_own_addr(rx->net);
 	u8 cur, new;
 
-	/* Ensure this message is destined to us first */
+	/* Update RX 'key_master' flag according to peer, also mark "legacy" if
+	 * a peer has no master key.
+	 */
+	rx->key_master = ehdr->master_key;
+	if (!rx->key_master)
+		tx->legacy_user = 1;
+
+	/* For later cases, apply only if message is destined to this node */
 	if (!ehdr->destined || msg_short(hdr) || msg_destnode(hdr) != self)
 		return;
 
-	/* Peer RX active key has changed, let's update own TX users */
+	/* Case 1: Peer has no keys, let's make master key taking over */
+	if (ehdr->rx_nokey)
+		/* Set or extend grace period */
+		tx->timer2 = jiffies;
+
+	/* Case 2: Peer RX active key has changed, let's update own TX users */
 	cur = atomic_read(&rx->peer_rx_active);
 	new = ehdr->rx_key_active;
 	if (tx->key.keys &&
@@ -1338,7 +1384,7 @@ int tipc_crypto_start(struct tipc_crypto **crypto, struct net *net,
 		return -ENOMEM;
 	}
 
-	c->working = 0;
+	c->flags = 0;
 	c->net = net;
 	c->node = node;
 	tipc_crypto_key_set_state(c, 0, 0, 0);
@@ -1473,6 +1519,12 @@ void tipc_crypto_timeout(struct tipc_crypto *rx)
 s5:
 	spin_unlock(&rx->lock);
 
+	/* Relax it here, the flag will be set again if it really is, but only
+	 * when we are not in grace period for safety!
+	 */
+	if (time_after(jiffies, tx->timer2 + TIPC_TX_GRACE_PERIOD))
+		tx->legacy_user = 0;
+
 	/* Limit max_tfms & do debug commands if needed */
 	if (likely(sysctl_tipc_max_tfms <= TIPC_MAX_TFMS_LIM))
 		return;
@@ -1482,6 +1534,22 @@ void tipc_crypto_timeout(struct tipc_crypto *rx)
 	tipc_crypto_do_cmd(rx->net, cmd);
 }
 
+static inline void tipc_crypto_clone_msg(struct net *net, struct sk_buff *_skb,
+					 struct tipc_bearer *b,
+					 struct tipc_media_addr *dst,
+					 struct tipc_node *__dnode, u8 type)
+{
+	struct sk_buff *skb;
+
+	skb = skb_clone(_skb, GFP_ATOMIC);
+	if (skb) {
+		TIPC_SKB_CB(skb)->xmit_type = type;
+		tipc_crypto_xmit(net, &skb, b, dst, __dnode);
+		if (skb)
+			b->media->send_msg(net, skb, b, dst);
+	}
+}
+
 /**
  * tipc_crypto_xmit - Build & encrypt TIPC message for xmit
  * @net: struct net
@@ -1491,7 +1559,8 @@ void tipc_crypto_timeout(struct tipc_crypto *rx)
  * @__dnode: destination node for reference if any
  *
  * First, build an encryption message header on the top of the message, then
- * encrypt the original TIPC message by using the active or pending TX key.
+ * encrypt the original TIPC message by using the pending, master or active
+ * key with this preference order.
  * If the encryption is successful, the encrypted skb is returned directly or
  * via the callback.
  * Otherwise, the skb is freed!
@@ -1514,46 +1583,63 @@ int tipc_crypto_xmit(struct net *net, struct sk_buff **skb,
 	struct tipc_msg *hdr = buf_msg(*skb);
 	struct tipc_key key = tx->key;
 	struct tipc_aead *aead = NULL;
-	struct sk_buff *_skb;
-	int rc = -ENOKEY;
 	u32 user = msg_user(hdr);
-	u8 tx_key;
+	u32 type = msg_type(hdr);
+	int rc = -ENOKEY;
+	u8 tx_key = 0;
 
 	/* No encryption? */
 	if (!tx->working)
 		return 0;
 
-	/* Try with the pending key if available and:
-	 * 1) This is the only choice (i.e. no active key) or;
-	 * 2) Peer has switched to this key (unicast only) or;
-	 * 3) It is time to do a pending key probe;
-	 */
+	/* Pending key if peer has active on it or probing time */
 	if (unlikely(key.pending)) {
 		tx_key = key.pending;
-		if (!key.active)
+		if (!tx->key_master && !key.active)
 			goto encrypt;
 		if (__rx && atomic_read(&__rx->peer_rx_active) == tx_key)
 			goto encrypt;
-		if (TIPC_SKB_CB(*skb)->probe) {
+		if (TIPC_SKB_CB(*skb)->xmit_type == SKB_PROBING) {
 			pr_debug("%s: probing for key[%d]\n", tx->name,
 				 key.pending);
 			goto encrypt;
 		}
-		if (user == LINK_CONFIG || user == LINK_PROTOCOL) {
-			_skb = skb_clone(*skb, GFP_ATOMIC);
-			if (_skb) {
-				TIPC_SKB_CB(_skb)->probe = 1;
-				tipc_crypto_xmit(net, &_skb, b, dst, __dnode);
-				if (_skb)
-					b->media->send_msg(net, _skb, b, dst);
+		if (user == LINK_CONFIG || user == LINK_PROTOCOL)
+			tipc_crypto_clone_msg(net, *skb, b, dst, __dnode,
+					      SKB_PROBING);
+	}
+
+	/* Master key if this is a *vital* message or in grace period */
+	if (tx->key_master) {
+		tx_key = KEY_MASTER;
+		if (!key.active)
+			goto encrypt;
+		if (TIPC_SKB_CB(*skb)->xmit_type == SKB_GRACING) {
+			pr_debug("%s: gracing for msg (%d %d)\n", tx->name,
+				 user, type);
+			goto encrypt;
+		}
+		if (user == LINK_CONFIG ||
+		    (user == LINK_PROTOCOL && type == RESET_MSG) ||
+		    time_before(jiffies, tx->timer2 + TIPC_TX_GRACE_PERIOD)) {
+			if (__rx && __rx->key_master &&
+			    !atomic_read(&__rx->peer_rx_active))
+				goto encrypt;
+			if (!__rx) {
+				if (likely(!tx->legacy_user))
+					goto encrypt;
+				tipc_crypto_clone_msg(net, *skb, b, dst,
+						      __dnode, SKB_GRACING);
 			}
 		}
 	}
+
 	/* Else, use the active key if any */
 	if (likely(key.active)) {
 		tx_key = key.active;
 		goto encrypt;
 	}
+
 	goto exit;
 
 encrypt:
@@ -1619,15 +1705,16 @@ int tipc_crypto_rcv(struct net *net, struct tipc_crypto *rx,
 	struct tipc_aead *aead = NULL;
 	struct tipc_key key;
 	int rc = -ENOKEY;
-	u8 tx_key = 0;
+	u8 tx_key;
+
+	tx_key = ((struct tipc_ehdr *)(*skb)->data)->tx_key;
 
 	/* New peer?
 	 * Let's try with TX key (i.e. cluster mode) & verify the skb first!
 	 */
-	if (unlikely(!rx))
+	if (unlikely(!rx || tx_key == KEY_MASTER))
 		goto pick_tx;
 
-	tx_key = ((struct tipc_ehdr *)(*skb)->data)->tx_key;
 	/* Pick RX key according to TX key if any */
 	key = rx->key;
 	if (tx_key == key.active || tx_key == key.pending ||
@@ -1640,7 +1727,7 @@ int tipc_crypto_rcv(struct net *net, struct tipc_crypto *rx,
 
 pick_tx:
 	/* No key suitable? Try to pick one from TX... */
-	aead = tipc_crypto_key_pick_tx(tx, rx, *skb);
+	aead = tipc_crypto_key_pick_tx(tx, rx, *skb, tx_key);
 	if (aead)
 		goto decrypt;
 	goto exit;
@@ -1722,9 +1809,12 @@ static void tipc_crypto_rcv_complete(struct net *net, struct tipc_aead *aead,
 				goto free_skb;
 		}
 
+		/* Ignore cloning if it was TX master key */
+		if (ehdr->tx_key == KEY_MASTER)
+			goto rcv;
 		if (tipc_aead_clone(&tmp, aead) < 0)
 			goto rcv;
-		if (tipc_crypto_key_attach(rx, tmp, ehdr->tx_key) < 0) {
+		if (tipc_crypto_key_attach(rx, tmp, ehdr->tx_key, false) < 0) {
 			tipc_aead_free(&tmp->rcu);
 			goto rcv;
 		}
@@ -1740,10 +1830,10 @@ static void tipc_crypto_rcv_complete(struct net *net, struct tipc_aead *aead,
 	/* Set the RX key's user */
 	tipc_aead_users_set(aead, 1);
 
-rcv:
 	/* Mark this point, RX works */
 	rx->timer1 = jiffies;
 
+rcv:
 	/* Remove ehdr & auth. tag prior to tipc_rcv() */
 	ehdr = (struct tipc_ehdr *)(*skb)->data;
 
@@ -1865,14 +1955,24 @@ static char *tipc_crypto_key_dump(struct tipc_crypto *c, char *buf)
 	char *s;
 
 	for (k = KEY_MIN; k <= KEY_MAX; k++) {
-		if (k == key.passive)
-			s = "PAS";
-		else if (k == key.active)
-			s = "ACT";
-		else if (k == key.pending)
-			s = "PEN";
-		else
-			s = "-";
+		if (k == KEY_MASTER) {
+			if (is_rx(c))
+				continue;
+			if (time_before(jiffies,
+					c->timer2 + TIPC_TX_GRACE_PERIOD))
+				s = "ACT";
+			else
+				s = "PAS";
+		} else {
+			if (k == key.passive)
+				s = "PAS";
+			else if (k == key.active)
+				s = "ACT";
+			else if (k == key.pending)
+				s = "PEN";
+			else
+				s = "-";
+		}
 		i += scnprintf(buf + i, 200 - i, "\tKey%d: %s", k, s);
 
 		rcu_read_lock();
diff --git a/net/tipc/crypto.h b/net/tipc/crypto.h
index c3de769f49e8..7fcb80cb0e8a 100644
--- a/net/tipc/crypto.h
+++ b/net/tipc/crypto.h
@@ -74,7 +74,7 @@ extern int sysctl_tipc_max_tfms __read_mostly;
  *     3 3 2 2 2 2 2 2 2 2 2 2 1 1 1 1 1 1 1 1 1 1 0 0 0 0 0 0 0 0 0 0
  *     1 0 9 8 7 6 5 4|3 2 1 0 9 8 7 6|5 4 3 2 1 0 9 8|7 6 5 4 3 2 1 0
  *    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
- * w0:|Ver=7| User  |D|TX |RX |K|                 Rsvd                |
+ * w0:|Ver=7| User  |D|TX |RX |K|M|N|             Rsvd                |
  *    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  * w1:|                             Seqno                             |
  * w2:|                           (8 octets)                          |
@@ -101,6 +101,9 @@ extern int sysctl_tipc_max_tfms __read_mostly;
  *	RX	: Currently RX active key corresponding to the destination
  *	          node's TX key (when the "D" bit is set)
  *	K	: Keep-alive bit (for RPS, LINK_PROTOCOL/STATE_MSG only)
+ *	M       : Bit indicates if sender has master key
+ *	N	: Bit indicates if sender has no RX keys corresponding to the
+ *	          receiver's TX (when the "D" bit is set)
  *	Rsvd	: Reserved bit, field
  * Word1-2:
  *	Seqno	: The 64-bit sequence number of the encrypted message, also
@@ -117,7 +120,9 @@ struct tipc_ehdr {
 			__u8	destined:1,
 				user:4,
 				version:3;
-			__u8	reserved_1:3,
+			__u8	reserved_1:1,
+				rx_nokey:1,
+				master_key:1,
 				keepalive:1,
 				rx_key_active:2,
 				tx_key:2;
@@ -128,7 +133,9 @@ struct tipc_ehdr {
 			__u8	tx_key:2,
 				rx_key_active:2,
 				keepalive:1,
-				reserved_1:3;
+				master_key:1,
+				rx_nokey:1,
+				reserved_1:1;
 #else
 #error  "Please fix <asm/byteorder.h>"
 #endif
@@ -158,7 +165,7 @@ int tipc_crypto_xmit(struct net *net, struct sk_buff **skb,
 int tipc_crypto_rcv(struct net *net, struct tipc_crypto *rx,
 		    struct sk_buff **skb, struct tipc_bearer *b);
 int tipc_crypto_key_init(struct tipc_crypto *c, struct tipc_aead_key *ukey,
-			 u8 mode);
+			 u8 mode, bool master_key);
 void tipc_crypto_key_flush(struct tipc_crypto *c);
 int tipc_aead_key_validate(struct tipc_aead_key *ukey);
 bool tipc_ehdr_validate(struct sk_buff *skb);
diff --git a/net/tipc/msg.h b/net/tipc/msg.h
index 1016e96db5c4..25e5c5c8a6ff 100644
--- a/net/tipc/msg.h
+++ b/net/tipc/msg.h
@@ -127,7 +127,9 @@ struct tipc_skb_cb {
 #ifdef CONFIG_TIPC_CRYPTO
 			u8 encrypted:1;
 			u8 decrypted:1;
-			u8 probe:1;
+#define SKB_PROBING	1
+#define SKB_GRACING	2
+			u8 xmit_type:2;
 			u8 tx_clone_deferred:1;
 #endif
 		};
diff --git a/net/tipc/netlink.c b/net/tipc/netlink.c
index c4aee6247d55..1ec00fcc26ee 100644
--- a/net/tipc/netlink.c
+++ b/net/tipc/netlink.c
@@ -108,6 +108,7 @@ const struct nla_policy tipc_nl_node_policy[TIPC_NLA_NODE_MAX + 1] = {
 					    .len = TIPC_NODEID_LEN},
 	[TIPC_NLA_NODE_KEY]		= { .type = NLA_BINARY,
 					    .len = TIPC_AEAD_KEY_SIZE_MAX},
+	[TIPC_NLA_NODE_KEY_MASTER]	= { .type = NLA_FLAG },
 };
 
 /* Properties valid for media, bearer and link */
diff --git a/net/tipc/node.c b/net/tipc/node.c
index 030a51c4d1fa..55f012d1ea74 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -2872,11 +2872,11 @@ static int __tipc_nl_node_set_key(struct sk_buff *skb, struct genl_info *info)
 {
 	struct nlattr *attrs[TIPC_NLA_NODE_MAX + 1];
 	struct net *net = sock_net(skb->sk);
-	struct tipc_net *tn = tipc_net(net);
+	struct tipc_crypto *tx = tipc_net(net)->crypto_tx, *c = tx;
 	struct tipc_node *n = NULL;
 	struct tipc_aead_key *ukey;
-	struct tipc_crypto *c;
-	u8 *id, *own_id;
+	bool master_key = false;
+	u8 *id, *own_id, mode;
 	int rc = 0;
 
 	if (!info->attrs[TIPC_NLA_NODE])
@@ -2886,51 +2886,45 @@ static int __tipc_nl_node_set_key(struct sk_buff *skb, struct genl_info *info)
 			      info->attrs[TIPC_NLA_NODE],
 			      tipc_nl_node_policy, info->extack);
 	if (rc)
-		goto exit;
+		return rc;
 
 	own_id = tipc_own_id(net);
-	if (!own_id) {
-		rc = -EPERM;
-		goto exit;
-	}
+	if (!own_id)
+		return -EPERM;
 
 	rc = tipc_nl_retrieve_key(attrs, &ukey);
 	if (rc)
-		goto exit;
+		return rc;
 
 	rc = tipc_aead_key_validate(ukey);
 	if (rc)
-		goto exit;
+		return rc;
 
 	rc = tipc_nl_retrieve_nodeid(attrs, &id);
 	switch (rc) {
 	case -ENODATA:
-		/* Cluster key mode */
-		rc = tipc_crypto_key_init(tn->crypto_tx, ukey, CLUSTER_KEY);
+		mode = CLUSTER_KEY;
+		master_key = !!(attrs[TIPC_NLA_NODE_KEY_MASTER]);
 		break;
 	case 0:
-		/* Per-node key mode */
-		if (!memcmp(id, own_id, NODE_ID_LEN)) {
-			c = tn->crypto_tx;
-		} else {
+		mode = PER_NODE_KEY;
+		if (memcmp(id, own_id, NODE_ID_LEN)) {
 			n = tipc_node_find_by_id(net, id) ?:
 				tipc_node_create(net, 0, id, 0xffffu, 0, true);
-			if (unlikely(!n)) {
-				rc = -ENOMEM;
-				break;
-			}
+			if (unlikely(!n))
+				return -ENOMEM;
 			c = n->crypto_rx;
 		}
-
-		rc = tipc_crypto_key_init(c, ukey, PER_NODE_KEY);
-		if (n)
-			tipc_node_put(n);
 		break;
 	default:
-		break;
+		return rc;
 	}
 
-exit:
+	/* Initiate the TX/RX key */
+	rc = tipc_crypto_key_init(c, ukey, mode, master_key);
+	if (n)
+		tipc_node_put(n);
+
 	return (rc < 0) ? rc : 0;
 }
 
-- 
2.13.7

[tipc-discussion] [PATCH RFC 1/5] tipc: fix using smp_processor_id() in preemptible

[Tuong L. <tuo...@de...>]

The 'this_cpu_ptr()' is used to obtain the AEAD key' TFM on the current
CPU for encryption, however the execution can be preemptible since it's
actually user-space context, so the 'using smp_processor_id() in
preemptible' has been observed.

We fix the issue by using the 'get/put_cpu_ptr()' API which consists of
a 'preempt_disable()' instead.

Signed-off-by: Tuong Lien <tuo...@de...>
---
 net/tipc/crypto.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index c8c47fc72653..1827ce4fac5d 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -326,7 +326,8 @@ static void tipc_aead_free(struct rcu_head *rp)
 	if (aead->cloned) {
 		tipc_aead_put(aead->cloned);
 	} else {
-		head = *this_cpu_ptr(aead->tfm_entry);
+		head = *get_cpu_ptr(aead->tfm_entry);
+		put_cpu_ptr(aead->tfm_entry);
 		list_for_each_entry_safe(tfm_entry, tmp, &head->list, list) {
 			crypto_free_aead(tfm_entry->tfm);
 			list_del(&tfm_entry->list);
@@ -399,10 +400,15 @@ static void tipc_aead_users_set(struct tipc_aead __rcu *aead, int val)
  */
 static struct crypto_aead *tipc_aead_tfm_next(struct tipc_aead *aead)
 {
-	struct tipc_tfm **tfm_entry = this_cpu_ptr(aead->tfm_entry);
+	struct tipc_tfm **tfm_entry;
+	struct crypto_aead *tfm;
 
+	tfm_entry = get_cpu_ptr(aead->tfm_entry);
 	*tfm_entry = list_next_entry(*tfm_entry, list);
-	return (*tfm_entry)->tfm;
+	tfm = (*tfm_entry)->tfm;
+	put_cpu_ptr(tfm_entry);
+
+	return tfm;
 }
 
 /**
-- 
2.13.7

[tipc-discussion] [PATCH RFC 2/5] tipc: optimize key switching time and logic

[Tuong L. <tuo...@de...>]

We reduce the lasting time for a pending TX key to be active as well as
for a passive RX key to be freed which generally helps speed up the key
switching. It is not expected to be too fast but should not be too slow
either. Also the key handling logic is simplified that a pending RX key
will be removed automatically if it is found not working after a number
of times; the probing for a pending TX key is now carried on a specific
message user ('LINK_PROTOCOL' or 'LINK_CONFIG') which is more efficient
than using a timer on broadcast messages, the timer is reserved for use
later as needed.

The kernel logs or 'pr***()' are now made as clear as possible to user.
Some prints are added, removed or changed to the debug-level. The
'TIPC_CRYPTO_DEBUG' definition is removed, and the 'pr_debug()' is used
instead which will be much helpful in runtime.

Besides we also optimize the code in some other places as a preparation
for later commits.

This commit does not change the en/decryption functionalities.

Signed-off-by: Tuong Lien <tuo...@de...>
---
 net/tipc/crypto.c | 344 ++++++++++++++++++++++--------------------------------
 1 file changed, 141 insertions(+), 203 deletions(-)

diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index 1827ce4fac5d..f1046ab4bd01 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -38,10 +38,10 @@
 #include <crypto/aes.h>
 #include "crypto.h"
 
-#define TIPC_TX_PROBE_LIM	msecs_to_jiffies(1000) /* > 1s */
-#define TIPC_TX_LASTING_LIM	msecs_to_jiffies(120000) /* 2 mins */
+#define TIPC_TX_LASTING_TIME	msecs_to_jiffies(10000) /* 10s */
 #define TIPC_RX_ACTIVE_LIM	msecs_to_jiffies(3000) /* 3s */
-#define TIPC_RX_PASSIVE_LIM	msecs_to_jiffies(180000) /* 3 mins */
+#define TIPC_RX_PASSIVE_LIM	msecs_to_jiffies(15000) /* 15s */
+
 #define TIPC_MAX_TFMS_DEF	10
 #define TIPC_MAX_TFMS_LIM	1000
 
@@ -144,7 +144,7 @@ struct tipc_aead {
 	u32 salt;
 	u8 authsize;
 	u8 mode;
-	char hint[TIPC_AEAD_HINT_LEN + 1];
+	char hint[2 * TIPC_AEAD_HINT_LEN + 1];
 	struct rcu_head rcu;
 
 	atomic64_t seqno ____cacheline_aligned;
@@ -168,9 +168,10 @@ struct tipc_crypto_stats {
  * @key: the key states
  * @working: the crypto is working or not
  * @stats: the crypto statistics
+ * @name: the crypto name
  * @sndnxt: the per-peer sndnxt (TX)
  * @timer1: general timer 1 (jiffies)
- * @timer2: general timer 1 (jiffies)
+ * @timer2: general timer 2 (jiffies)
  * @lock: tipc_key lock
  */
 struct tipc_crypto {
@@ -181,6 +182,7 @@ struct tipc_crypto {
 	struct tipc_key key;
 	u8 working:1;
 	struct tipc_crypto_stats __percpu *stats;
+	char name[48];
 
 	atomic64_t sndnxt ____cacheline_aligned;
 	unsigned long timer1;
@@ -239,18 +241,17 @@ static bool tipc_crypto_key_try_align(struct tipc_crypto *rx, u8 new_pending);
 static struct tipc_aead *tipc_crypto_key_pick_tx(struct tipc_crypto *tx,
 						 struct tipc_crypto *rx,
 						 struct sk_buff *skb);
-static void tipc_crypto_key_synch(struct tipc_crypto *rx, u8 new_rx_active,
-				  struct tipc_msg *hdr);
+static void tipc_crypto_key_synch(struct tipc_crypto *rx, struct sk_buff *skb);
 static int tipc_crypto_key_revoke(struct net *net, u8 tx_key);
 static void tipc_crypto_rcv_complete(struct net *net, struct tipc_aead *aead,
 				     struct tipc_bearer *b,
 				     struct sk_buff **skb, int err);
 static void tipc_crypto_do_cmd(struct net *net, int cmd);
 static char *tipc_crypto_key_dump(struct tipc_crypto *c, char *buf);
-#ifdef TIPC_CRYPTO_DEBUG
 static char *tipc_key_change_dump(struct tipc_key old, struct tipc_key new,
 				  char *buf);
-#endif
+#define is_tx(crypto) (!(crypto)->node)
+#define is_rx(crypto) (!is_tx(crypto))
 
 #define key_next(cur) ((cur) % KEY_MAX + 1)
 
@@ -290,7 +291,7 @@ int tipc_aead_key_validate(struct tipc_aead_key *ukey)
 	if (unlikely(keylen != TIPC_AES_GCM_KEY_SIZE_128 &&
 		     keylen != TIPC_AES_GCM_KEY_SIZE_192 &&
 		     keylen != TIPC_AES_GCM_KEY_SIZE_256))
-		return -EINVAL;
+		return -EKEYREJECTED;
 
 	return 0;
 }
@@ -501,9 +502,9 @@ static int tipc_aead_init(struct tipc_aead **aead, struct tipc_aead_key *ukey,
 		return err;
 	}
 
-	/* Copy some chars from the user key as a hint */
-	memcpy(tmp->hint, ukey->key, TIPC_AEAD_HINT_LEN);
-	tmp->hint[TIPC_AEAD_HINT_LEN] = '\0';
+	/* Form a hex string of some last bytes as the key's hint */
+	bin2hex(tmp->hint, ukey->key + keylen - TIPC_AEAD_HINT_LEN,
+		TIPC_AEAD_HINT_LEN);
 
 	/* Initialize the other data */
 	tmp->mode = mode;
@@ -663,13 +664,11 @@ static int tipc_aead_encrypt(struct tipc_aead *aead, struct sk_buff *skb,
 	 * but there is no frag_list, it should be still fine!
 	 * Otherwise, we must cow it to be a writable buffer with the tailroom.
 	 */
-#ifdef TIPC_CRYPTO_DEBUG
 	SKB_LINEAR_ASSERT(skb);
 	if (tailen > skb_tailroom(skb)) {
-		pr_warn("TX: skb tailroom is not enough: %d, requires: %d\n",
-			skb_tailroom(skb), tailen);
+		pr_debug("TX(): skb tailroom is not enough: %d, requires: %d\n",
+			 skb_tailroom(skb), tailen);
 	}
-#endif
 
 	if (unlikely(!skb_cloned(skb) && tailen <= skb_tailroom(skb))) {
 		nsg = 1;
@@ -1017,23 +1016,16 @@ static inline void tipc_crypto_key_set_state(struct tipc_crypto *c,
 					     u8 new_active,
 					     u8 new_pending)
 {
-#ifdef TIPC_CRYPTO_DEBUG
 	struct tipc_key old = c->key;
 	char buf[32];
-#endif
 
 	c->key.keys = ((new_passive & KEY_MASK) << (KEY_BITS * 2)) |
 		      ((new_active  & KEY_MASK) << (KEY_BITS)) |
 		      ((new_pending & KEY_MASK));
 
-#ifdef TIPC_CRYPTO_DEBUG
-	pr_info("%s(%s): key changing %s ::%pS\n",
-		(c->node) ? "RX" : "TX",
-		(c->node) ? tipc_node_get_id_str(c->node) :
-			    tipc_own_id_string(c->net),
-		tipc_key_change_dump(old, c->key, buf),
-		__builtin_return_address(0));
-#endif
+	pr_debug("%s: key changing %s ::%pS\n", c->name,
+		 tipc_key_change_dump(old, c->key, buf),
+		 __builtin_return_address(0));
 }
 
 /**
@@ -1055,20 +1047,20 @@ int tipc_crypto_key_init(struct tipc_crypto *c, struct tipc_aead_key *ukey,
 
 	/* Initiate with the new user key */
 	rc = tipc_aead_init(&aead, ukey, mode);
+	if (unlikely(rc)) {
+		pr_err("%s: unable to init key, err %d\n", c->name, rc);
+		return rc;
+	}
 
 	/* Attach it to the crypto */
-	if (likely(!rc)) {
-		rc = tipc_crypto_key_attach(c, aead, 0);
-		if (rc < 0)
-			tipc_aead_free(&aead->rcu);
+	rc = tipc_crypto_key_attach(c, aead, 0);
+	if (rc < 0) {
+		pr_err("%s: unable to attach key, err %d\n", c->name, rc);
+		tipc_aead_free(&aead->rcu);
+		return rc;
 	}
 
-	pr_info("%s(%s): key initiating, rc %d!\n",
-		(c->node) ? "RX" : "TX",
-		(c->node) ? tipc_node_get_id_str(c->node) :
-			    tipc_own_id_string(c->net),
-		rc);
-
+	pr_info("%s: key[%d] is successfully attached\n", c->name, rc);
 	return rc;
 }
 
@@ -1083,49 +1075,42 @@ int tipc_crypto_key_init(struct tipc_crypto *c, struct tipc_aead_key *ukey,
 static int tipc_crypto_key_attach(struct tipc_crypto *c,
 				  struct tipc_aead *aead, u8 pos)
 {
-	u8 new_pending, new_passive, new_key;
 	struct tipc_key key;
 	int rc = -EBUSY;
+	u8 new_key;
 
 	spin_lock_bh(&c->lock);
 	key = c->key;
 	if (key.active && key.passive)
 		goto exit;
-	if (key.passive && !tipc_aead_users(c->aead[key.passive]))
-		goto exit;
 	if (key.pending) {
-		if (pos)
-			goto exit;
 		if (tipc_aead_users(c->aead[key.pending]) > 0)
 			goto exit;
+		/* if (pos): ok with replacing, will be aligned when needed */
 		/* Replace it */
-		new_pending = key.pending;
-		new_passive = key.passive;
-		new_key = new_pending;
+		new_key = key.pending;
 	} else {
 		if (pos) {
 			if (key.active && pos != key_next(key.active)) {
-				new_pending = key.pending;
-				new_passive = pos;
-				new_key = new_passive;
+				key.passive = pos;
+				new_key = pos;
 				goto attach;
 			} else if (!key.active && !key.passive) {
-				new_pending = pos;
-				new_passive = key.passive;
-				new_key = new_pending;
+				key.pending = pos;
+				new_key = pos;
 				goto attach;
 			}
 		}
-		new_pending = key_next(key.active ?: key.passive);
-		new_passive = key.passive;
-		new_key = new_pending;
+		key.pending = key_next(key.active ?: key.passive);
+		new_key = key.pending;
 	}
 
 attach:
 	aead->crypto = c;
-	tipc_crypto_key_set_state(c, new_passive, key.active, new_pending);
 	tipc_aead_rcu_replace(c->aead[new_key], aead, &c->lock);
-
+	if (likely(c->key.keys != key.keys))
+		tipc_crypto_key_set_state(c, key.passive, key.active,
+					  key.pending);
 	c->working = 1;
 	c->timer1 = jiffies;
 	c->timer2 = jiffies;
@@ -1204,7 +1189,8 @@ static bool tipc_crypto_key_try_align(struct tipc_crypto *rx, u8 new_pending)
 		rcu_assign_pointer(rx->aead[new_passive], tmp2);
 	refcount_set(&tmp1->refcnt, 1);
 	aligned = true;
-	pr_info("RX(%s): key is aligned!\n", tipc_node_get_id_str(rx->node));
+	pr_info_ratelimited("%s: key[%d] -> key[%d]\n", rx->name, key.pending,
+			    new_pending);
 
 exit:
 	spin_unlock(&rx->lock);
@@ -1274,8 +1260,7 @@ static struct tipc_aead *tipc_crypto_key_pick_tx(struct tipc_crypto *tx,
 /**
  * tipc_crypto_key_synch: Synch own key data according to peer key status
  * @rx: RX crypto handle
- * @new_rx_active: latest RX active key from peer
- * @hdr: TIPCv2 message
+ * @skb: TIPCv2 message buffer (incl. the ehdr from peer)
  *
  * This function updates the peer node related data as the peer RX active key
  * has changed, so the number of TX keys' users on this node are increased and
@@ -1283,44 +1268,35 @@ static struct tipc_aead *tipc_crypto_key_pick_tx(struct tipc_crypto *tx,
  *
  * The "per-peer" sndnxt is also reset when the peer key has switched.
  */
-static void tipc_crypto_key_synch(struct tipc_crypto *rx, u8 new_rx_active,
-				  struct tipc_msg *hdr)
+static void tipc_crypto_key_synch(struct tipc_crypto *rx, struct sk_buff *skb)
 {
-	struct net *net = rx->net;
-	struct tipc_crypto *tx = tipc_net(net)->crypto_tx;
-	u8 cur_rx_active;
-
-	/* TX might be even not ready yet */
-	if (unlikely(!tx->key.active && !tx->key.pending))
-		return;
-
-	cur_rx_active = atomic_read(&rx->peer_rx_active);
-	if (likely(cur_rx_active == new_rx_active))
-		return;
+	struct tipc_ehdr *ehdr = (struct tipc_ehdr *)skb_network_header(skb);
+	struct tipc_crypto *tx = tipc_net(rx->net)->crypto_tx;
+	struct tipc_msg *hdr = buf_msg(skb);
+	u32 self = tipc_own_addr(rx->net);
+	u8 cur, new;
 
-	/* Make sure this message destined for this node */
-	if (unlikely(msg_short(hdr) ||
-		     msg_destnode(hdr) != tipc_own_addr(net)))
+	/* Ensure this message is destined to us first */
+	if (!ehdr->destined || msg_short(hdr) || msg_destnode(hdr) != self)
 		return;
 
-	/* Peer RX active key has changed, try to update owns' & TX users */
-	if (atomic_cmpxchg(&rx->peer_rx_active,
-			   cur_rx_active,
-			   new_rx_active) == cur_rx_active) {
-		if (new_rx_active)
-			tipc_aead_users_inc(tx->aead[new_rx_active], INT_MAX);
-		if (cur_rx_active)
-			tipc_aead_users_dec(tx->aead[cur_rx_active], 0);
+	/* Peer RX active key has changed, let's update own TX users */
+	cur = atomic_read(&rx->peer_rx_active);
+	new = ehdr->rx_key_active;
+	if (tx->key.keys &&
+	    cur != new &&
+	    atomic_cmpxchg(&rx->peer_rx_active, cur, new) == cur) {
+		if (new)
+			tipc_aead_users_inc(tx->aead[new], INT_MAX);
+		if (cur)
+			tipc_aead_users_dec(tx->aead[cur], 0);
 
 		atomic64_set(&rx->sndnxt, 0);
 		/* Mark the point TX key users changed */
 		tx->timer1 = jiffies;
 
-#ifdef TIPC_CRYPTO_DEBUG
-		pr_info("TX(%s): key users changed %d-- %d++, peer RX(%s)\n",
-			tipc_own_id_string(net), cur_rx_active,
-			new_rx_active, tipc_node_get_id_str(rx->node));
-#endif
+		pr_debug("%s: key users changed %d-- %d++, peer %s\n",
+			 tx->name, cur, new, rx->name);
 	}
 }
 
@@ -1338,7 +1314,7 @@ static int tipc_crypto_key_revoke(struct net *net, u8 tx_key)
 	tipc_crypto_key_detach(tx->aead[key.active], &tx->lock);
 	spin_unlock(&tx->lock);
 
-	pr_warn("TX(%s): key is revoked!\n", tipc_own_id_string(net));
+	pr_warn("%s: key is revoked\n", tx->name);
 	return -EKEYREVOKED;
 }
 
@@ -1371,25 +1347,26 @@ int tipc_crypto_start(struct tipc_crypto **crypto, struct net *net,
 	c->timer1 = jiffies;
 	c->timer2 = jiffies;
 	spin_lock_init(&c->lock);
-	*crypto = c;
+	scnprintf(c->name, 48, "%s(%s)", (is_rx(c)) ? "RX" : "TX",
+		  (is_rx(c)) ? tipc_node_get_id_str(c->node) :
+			       tipc_own_id_string(c->net));
 
+	*crypto = c;
 	return 0;
 }
 
 void tipc_crypto_stop(struct tipc_crypto **crypto)
 {
-	struct tipc_crypto *c, *tx, *rx;
-	bool is_rx;
+	struct tipc_crypto *c = *crypto, *tx, *rx;
 	u8 k;
 
-	if (!*crypto)
+	if (!c)
 		return;
 
 	rcu_read_lock();
 	/* RX stopping? => decrease TX key users if any */
-	is_rx = !!((*crypto)->node);
-	if (is_rx) {
-		rx = *crypto;
+	if (is_rx(c)) {
+		rx = c;
 		tx = tipc_net(rx->net)->crypto_tx;
 		k = atomic_read(&rx->peer_rx_active);
 		if (k) {
@@ -1400,15 +1377,10 @@ void tipc_crypto_stop(struct tipc_crypto **crypto)
 	}
 
 	/* Release AEAD keys */
-	c = *crypto;
 	for (k = KEY_MIN; k <= KEY_MAX; k++)
 		tipc_aead_put(rcu_dereference(c->aead[k]));
 	rcu_read_unlock();
-
-	pr_warn("%s(%s) has been purged, node left!\n",
-		(is_rx) ? "RX" : "TX",
-		(is_rx) ? tipc_node_get_id_str((*crypto)->node) :
-			  tipc_own_id_string((*crypto)->net));
+	pr_debug("%s: has been stopped\n", c->name);
 
 	/* Free this crypto statistics */
 	free_percpu(c->stats);
@@ -1422,102 +1394,81 @@ void tipc_crypto_timeout(struct tipc_crypto *rx)
 	struct tipc_net *tn = tipc_net(rx->net);
 	struct tipc_crypto *tx = tn->crypto_tx;
 	struct tipc_key key;
-	u8 new_pending, new_passive;
 	int cmd;
 
-	/* TX key activating:
-	 * The pending key (users > 0) -> active
-	 * The active key if any (users == 0) -> free
-	 */
+	/* TX pending: taking all users & stable -> active */
 	spin_lock(&tx->lock);
 	key = tx->key;
 	if (key.active && tipc_aead_users(tx->aead[key.active]) > 0)
 		goto s1;
 	if (!key.pending || tipc_aead_users(tx->aead[key.pending]) <= 0)
 		goto s1;
-	if (time_before(jiffies, tx->timer1 + TIPC_TX_LASTING_LIM))
+	if (time_before(jiffies, tx->timer1 + TIPC_TX_LASTING_TIME))
 		goto s1;
 
 	tipc_crypto_key_set_state(tx, key.passive, key.pending, 0);
 	if (key.active)
 		tipc_crypto_key_detach(tx->aead[key.active], &tx->lock);
 	this_cpu_inc(tx->stats->stat[STAT_SWITCHES]);
-	pr_info("TX(%s): key %d is activated!\n", tipc_own_id_string(tx->net),
-		key.pending);
+	pr_info("%s: key[%d] is activated\n", tx->name, key.pending);
 
 s1:
 	spin_unlock(&tx->lock);
 
-	/* RX key activating:
-	 * The pending key (users > 0) -> active
-	 * The active key if any -> passive, freed later
-	 */
+	/* RX pending: having user -> active */
 	spin_lock(&rx->lock);
 	key = rx->key;
 	if (!key.pending || tipc_aead_users(rx->aead[key.pending]) <= 0)
 		goto s2;
 
-	new_pending = (key.passive &&
-		       !tipc_aead_users(rx->aead[key.passive])) ?
-				       key.passive : 0;
-	new_passive = (key.active) ?: ((new_pending) ? 0 : key.passive);
-	tipc_crypto_key_set_state(rx, new_passive, key.pending, new_pending);
+	if (key.active)
+		key.passive = key.active;
+	key.active = key.pending;
+	rx->timer2 = jiffies;
+	tipc_crypto_key_set_state(rx, key.passive, key.active, 0);
 	this_cpu_inc(rx->stats->stat[STAT_SWITCHES]);
-	pr_info("RX(%s): key %d is activated!\n",
-		tipc_node_get_id_str(rx->node),	key.pending);
+	pr_info("%s: key[%d] is activated\n", rx->name, key.pending);
 	goto s5;
 
 s2:
-	/* RX key "faulty" switching:
-	 * The faulty pending key (users < -30) -> passive
-	 * The passive key (users = 0) -> pending
-	 * Note: This only happens after RX deactivated - s3!
-	 */
-	key = rx->key;
-	if (!key.pending || tipc_aead_users(rx->aead[key.pending]) > -30)
-		goto s3;
-	if (!key.passive || tipc_aead_users(rx->aead[key.passive]) != 0)
+	/* RX pending: not working -> remove */
+	if (!key.pending || tipc_aead_users(rx->aead[key.pending]) > -10)
 		goto s3;
 
-	new_pending = key.passive;
-	new_passive = key.pending;
-	tipc_crypto_key_set_state(rx, new_passive, key.active, new_pending);
+	tipc_crypto_key_set_state(rx, key.passive, key.active, 0);
+	tipc_crypto_key_detach(rx->aead[key.pending], &rx->lock);
+	pr_info("%s: key[%d] is removed\n", rx->name, key.pending);
 	goto s5;
 
 s3:
-	/* RX key deactivating:
-	 * The passive key if any -> pending
-	 * The active key -> passive (users = 0) / pending
-	 * The pending key if any -> passive (users = 0)
-	 */
-	key = rx->key;
+	/* RX active: timed out or no user -> pending */
 	if (!key.active)
 		goto s4;
-	if (time_before(jiffies, rx->timer1 + TIPC_RX_ACTIVE_LIM))
+	if (time_before(jiffies, rx->timer1 + TIPC_RX_ACTIVE_LIM) &&
+	    tipc_aead_users(rx->aead[key.active]) > 0)
 		goto s4;
 
-	new_pending = (key.passive) ?: key.active;
-	new_passive = (key.passive) ? key.active : key.pending;
-	tipc_aead_users_set(rx->aead[new_pending], 0);
-	if (new_passive)
-		tipc_aead_users_set(rx->aead[new_passive], 0);
-	tipc_crypto_key_set_state(rx, new_passive, 0, new_pending);
-	pr_info("RX(%s): key %d is deactivated!\n",
-		tipc_node_get_id_str(rx->node), key.active);
+	if (key.pending)
+		key.passive = key.active;
+	else
+		key.pending = key.active;
+	rx->timer2 = jiffies;
+	tipc_crypto_key_set_state(rx, key.passive, 0, key.pending);
+	tipc_aead_users_set(rx->aead[key.pending], 0);
+	pr_info("%s: key[%d] is deactivated\n", rx->name, key.active);
 	goto s5;
 
 s4:
-	/* RX key passive -> freed: */
-	key = rx->key;
-	if (!key.passive || !tipc_aead_users(rx->aead[key.passive]))
+	/* RX passive: outdated or not working -> free */
+	if (!key.passive)
 		goto s5;
-	if (time_before(jiffies, rx->timer2 + TIPC_RX_PASSIVE_LIM))
+	if (time_before(jiffies, rx->timer2 + TIPC_RX_PASSIVE_LIM) &&
+	    tipc_aead_users(rx->aead[key.passive]) > -10)
 		goto s5;
 
 	tipc_crypto_key_set_state(rx, 0, key.active, key.pending);
 	tipc_crypto_key_detach(rx->aead[key.passive], &rx->lock);
-	pr_info("RX(%s): key %d is freed!\n", tipc_node_get_id_str(rx->node),
-		key.passive);
+	pr_info("%s: key[%d] is freed\n", rx->name, key.passive);
 
 s5:
 	spin_unlock(&rx->lock);
@@ -1560,10 +1511,12 @@ int tipc_crypto_xmit(struct net *net, struct sk_buff **skb,
 	struct tipc_crypto *__rx = tipc_node_crypto_rx(__dnode);
 	struct tipc_crypto *tx = tipc_net(net)->crypto_tx;
 	struct tipc_crypto_stats __percpu *stats = tx->stats;
+	struct tipc_msg *hdr = buf_msg(*skb);
 	struct tipc_key key = tx->key;
 	struct tipc_aead *aead = NULL;
-	struct sk_buff *probe;
+	struct sk_buff *_skb;
 	int rc = -ENOKEY;
+	u32 user = msg_user(hdr);
 	u8 tx_key;
 
 	/* No encryption? */
@@ -1581,17 +1534,18 @@ int tipc_crypto_xmit(struct net *net, struct sk_buff **skb,
 			goto encrypt;
 		if (__rx && atomic_read(&__rx->peer_rx_active) == tx_key)
 			goto encrypt;
-		if (TIPC_SKB_CB(*skb)->probe)
+		if (TIPC_SKB_CB(*skb)->probe) {
+			pr_debug("%s: probing for key[%d]\n", tx->name,
+				 key.pending);
 			goto encrypt;
-		if (!__rx &&
-		    time_after(jiffies, tx->timer2 + TIPC_TX_PROBE_LIM)) {
-			tx->timer2 = jiffies;
-			probe = skb_clone(*skb, GFP_ATOMIC);
-			if (probe) {
-				TIPC_SKB_CB(probe)->probe = 1;
-				tipc_crypto_xmit(net, &probe, b, dst, __dnode);
-				if (probe)
-					b->media->send_msg(net, probe, b, dst);
+		}
+		if (user == LINK_CONFIG || user == LINK_PROTOCOL) {
+			_skb = skb_clone(*skb, GFP_ATOMIC);
+			if (_skb) {
+				TIPC_SKB_CB(_skb)->probe = 1;
+				tipc_crypto_xmit(net, &_skb, b, dst, __dnode);
+				if (_skb)
+					b->media->send_msg(net, _skb, b, dst);
 			}
 		}
 	}
@@ -1673,22 +1627,12 @@ int tipc_crypto_rcv(struct net *net, struct tipc_crypto *rx,
 	if (unlikely(!rx))
 		goto pick_tx;
 
-	/* Pick RX key according to TX key, three cases are possible:
-	 * 1) The current active key (likely) or;
-	 * 2) The pending (new or deactivated) key (if any) or;
-	 * 3) The passive or old active key (i.e. users > 0);
-	 */
 	tx_key = ((struct tipc_ehdr *)(*skb)->data)->tx_key;
+	/* Pick RX key according to TX key if any */
 	key = rx->key;
-	if (likely(tx_key == key.active))
+	if (tx_key == key.active || tx_key == key.pending ||
+	    tx_key == key.passive)
 		goto decrypt;
-	if (tx_key == key.pending)
-		goto decrypt;
-	if (tx_key == key.passive) {
-		rx->timer2 = jiffies;
-		if (tipc_aead_users(rx->aead[key.passive]) > 0)
-			goto decrypt;
-	}
 
 	/* Unknown key, let's try to align RX key(s) */
 	if (tipc_crypto_key_try_align(rx, tx_key))
@@ -1747,21 +1691,17 @@ static void tipc_crypto_rcv_complete(struct net *net, struct tipc_aead *aead,
 	struct tipc_aead *tmp = NULL;
 	struct tipc_ehdr *ehdr;
 	struct tipc_node *n;
-	u8 rx_key_active;
-	bool destined;
 
 	/* Is this completed by TX? */
-	if (unlikely(!rx->node)) {
+	if (unlikely(is_tx(aead->crypto))) {
 		rx = skb_cb->tx_clone_ctx.rx;
-#ifdef TIPC_CRYPTO_DEBUG
-		pr_info("TX->RX(%s): err %d, aead %p, skb->next %p, flags %x\n",
-			(rx) ? tipc_node_get_id_str(rx->node) : "-", err, aead,
-			(*skb)->next, skb_cb->flags);
-		pr_info("skb_cb [recurs %d, last %p], tx->aead [%p %p %p]\n",
-			skb_cb->tx_clone_ctx.recurs, skb_cb->tx_clone_ctx.last,
-			aead->crypto->aead[1], aead->crypto->aead[2],
-			aead->crypto->aead[3]);
-#endif
+		pr_debug("TX->RX(%s): err %d, aead %p, skb->next %p, flags %x\n",
+			 (rx) ? tipc_node_get_id_str(rx->node) : "-", err, aead,
+			 (*skb)->next, skb_cb->flags);
+		pr_debug("skb_cb [recurs %d, last %p], tx->aead [%p %p %p]\n",
+			 skb_cb->tx_clone_ctx.recurs, skb_cb->tx_clone_ctx.last,
+			 aead->crypto->aead[1], aead->crypto->aead[2],
+			 aead->crypto->aead[3]);
 		if (unlikely(err)) {
 			if (err == -EBADMSG && (*skb)->next)
 				tipc_rcv(net, (*skb)->next, b);
@@ -1782,9 +1722,6 @@ static void tipc_crypto_rcv_complete(struct net *net, struct tipc_aead *aead,
 				goto free_skb;
 		}
 
-		/* Skip cloning this time as we had a RX pending key */
-		if (rx->key.pending)
-			goto rcv;
 		if (tipc_aead_clone(&tmp, aead) < 0)
 			goto rcv;
 		if (tipc_crypto_key_attach(rx, tmp, ehdr->tx_key) < 0) {
@@ -1809,8 +1746,12 @@ static void tipc_crypto_rcv_complete(struct net *net, struct tipc_aead *aead,
 
 	/* Remove ehdr & auth. tag prior to tipc_rcv() */
 	ehdr = (struct tipc_ehdr *)(*skb)->data;
-	destined = ehdr->destined;
-	rx_key_active = ehdr->rx_key_active;
+
+	/* Mark this point, RX passive still works */
+	if (rx->key.passive && ehdr->tx_key == rx->key.passive)
+		rx->timer2 = jiffies;
+
+	skb_reset_network_header(*skb);
 	skb_pull(*skb, tipc_ehdr_size(ehdr));
 	pskb_trim(*skb, (*skb)->len - aead->authsize);
 
@@ -1820,9 +1761,8 @@ static void tipc_crypto_rcv_complete(struct net *net, struct tipc_aead *aead,
 		goto free_skb;
 	}
 
-	/* Update peer RX active key & TX users */
-	if (destined)
-		tipc_crypto_key_synch(rx, rx_key_active, buf_msg(*skb));
+	/* Ok, everything's fine, try to synch own keys according to peers' */
+	tipc_crypto_key_synch(rx, *skb);
 
 	/* Mark skb decrypted */
 	skb_cb->decrypted = 1;
@@ -1881,7 +1821,7 @@ static void tipc_crypto_do_cmd(struct net *net, int cmd)
 	/* Print crypto statistics */
 	for (i = 0, j = 0; i < MAX_STATS; i++)
 		j += scnprintf(buf + j, 200 - j, "|%11s ", hstats[i]);
-	pr_info("\nCounter     %s", buf);
+	pr_info("Counter     %s", buf);
 
 	memset(buf, '-', 115);
 	buf[115] = '\0';
@@ -1939,7 +1879,7 @@ static char *tipc_crypto_key_dump(struct tipc_crypto *c, char *buf)
 		aead = rcu_dereference(c->aead[k]);
 		if (aead)
 			i += scnprintf(buf + i, 200 - i,
-				       "{\"%s...\", \"%s\"}/%d:%d",
+				       "{\"0x...%s\", \"%s\"}/%d:%d",
 				       aead->hint,
 				       (aead->mode == CLUSTER_KEY) ? "c" : "p",
 				       atomic_read(&aead->users),
@@ -1948,14 +1888,13 @@ static char *tipc_crypto_key_dump(struct tipc_crypto *c, char *buf)
 		i += scnprintf(buf + i, 200 - i, "\n");
 	}
 
-	if (c->node)
+	if (is_rx(c))
 		i += scnprintf(buf + i, 200 - i, "\tPeer RX active: %d\n",
 			       atomic_read(&c->peer_rx_active));
 
 	return buf;
 }
 
-#ifdef TIPC_CRYPTO_DEBUG
 static char *tipc_key_change_dump(struct tipc_key old, struct tipc_key new,
 				  char *buf)
 {
@@ -1986,4 +1925,3 @@ static char *tipc_key_change_dump(struct tipc_key old, struct tipc_key new,
 	i += scnprintf(buf + i, 32 - i, "]");
 	return buf;
 }
-#endif
-- 
2.13.7

[tipc-discussion] [PATCH RFC 0/5] tipc: add more features to TIPC encryption

[Tuong L. <tuo...@de...>]

Hi Jon/all,

As mentioned, I'd like to share the series that I have added some new
features in order to complete the TIPC encryption:

- Patch 1 ("tipc: fix using smp_processor_id() in preemptible"):
- Patch 2 ("tipc: optimize key switching time and logic"):
These two patches just do a bug-fix and optimization for the code as
a preparation for later commits.

- Patch 3 ("tipc: introduce encryption master key"):
This will introduce a 'master key' support which is set by user as a
'long-term' or static key (e.g. shared between nodes in the cluster in
user control way). It will act like a key encryption key for the later
key exchange, as well as allow a new node joins the cluster while it
has no knowledge of current active session keys in the existing nodes.

The master key setting will use the same 'tipc node set key' command
but with a 'master' flag (see below).

- Patch 4 ("tipc: add automatic session key exchange"):
TX key of a node will now be able to be exchanged to peer nodes (
encrypted/decrypted by the master key) and attached as the
corresponding RX keys automatically. A node can also 'request' for a TX
key from peer whenever needed.

This will enable us to do the later rekeying, and also make a new node
being able to obtain the session keys from existing nodes.

- Patch 5 ("tipc: add automatic rekeying for encryption key"):
Finally, this patch will add the automatic rekeying which will generate
a session key on each node at a specific interval. The key will be
also distributed automatically to peer nodes, so it will be switched to
be active shortly and traffic will be finally encrypted/decrypted by
that new key.

The rekeying interval is configurable as well, also user can disable or
trigger an immediate rekeying if he wants.

Besides, there will be a patch in the 'iproute2/tipc' including the new
'tipc node set key' command options, basically it will look like this:

---------
$tipc node set key --help
Usage: tipc node set key KEY [algname ALGNAME] [PROPERTIES]
       tipc node set key rekeying REKEYING

KEY
  Symmetric KEY & SALT as a composite ASCII or hex string (0x...) in form:
  [KEY: 16, 24 or 32 octets][SALT: 4 octets]

ALGNAME
  Cipher algorithm [default: "gcm(aes)"]

PROPERTIES
  master                - Set KEY as a cluster master key
  <empty>               - Set KEY as a cluster key
  nodeid NODEID         - Set KEY as a per-node key for own or peer

REKEYING
  INTERVAL              - Set rekeying interval (in minutes) [0: disable]
  now                   - Trigger one (first) rekeying immediately

EXAMPLES
  tipc node set key 0x746869735F69735F615F6B657931365F73616C74
  tipc node set key this_is_a_key16_salt algname "gcm(aes)" nodeid 1001002
  tipc node set key this_is_a_master_key master rekeying now
  tipc node set key rekeying 600
---------

So, please help check the patches and give your comments, thanks a lot!

BR/Tuong

Tuong Lien (5):
  tipc: fix using smp_processor_id() in preemptible
  tipc: optimize key switching time and logic
  tipc: introduce encryption master key
  tipc: add automatic session key exchange
  tipc: add automatic rekeying for encryption key

 include/uapi/linux/tipc.h         |   2 +
 include/uapi/linux/tipc_netlink.h |   2 +
 net/tipc/crypto.c                 | 986 ++++++++++++++++++++++++++++----------
 net/tipc/crypto.h                 |  41 +-
 net/tipc/link.c                   |   5 +
 net/tipc/msg.h                    |  10 +-
 net/tipc/netlink.c                |   2 +
 net/tipc/node.c                   |  89 ++--
 net/tipc/node.h                   |   2 +
 net/tipc/sysctl.c                 |   9 +
 10 files changed, 862 insertions(+), 286 deletions(-)

-- 
2.13.7

Re: [tipc-discussion] [iproute2-next] tipc: fixed a compile warning in tipc/link.c

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

On 7/9/20 12:25 AM, Hoang Huu Le wrote:
> Fixes: 5027f233e35b ("tipc: add link broadcast get")
> Signed-off-by: Hoang Huu Le <hoa...@de...>
> ---
>   tipc/link.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tipc/link.c b/tipc/link.c
> index ba77a20152ea..192736eaa154 100644
> --- a/tipc/link.c
> +++ b/tipc/link.c
> @@ -217,7 +217,7 @@ static int cmd_link_get_bcast_cb(const struct nlmsghdr *nlh, void *data)
>   		print_string(PRINT_ANY, "method", "%s", "AUTOSELECT");
>   		close_json_object();
>   		open_json_object(NULL);
> -		print_uint(PRINT_ANY, "ratio", " ratio:%u%\n",
> +		print_uint(PRINT_ANY, "ratio", " ratio:%u\n",
>   			   mnl_attr_get_u32(props[prop_ratio]));
>   		break;
>   	default:
Acked-by: Jon Maloy <jm...@re...>

[tipc-discussion] [iproute2-next] tipc: fixed a compile warning in tipc/link.c

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

Fixes: 5027f233e35b ("tipc: add link broadcast get")
Signed-off-by: Hoang Huu Le <hoa...@de...>
---
 tipc/link.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tipc/link.c b/tipc/link.c
index ba77a20152ea..192736eaa154 100644
--- a/tipc/link.c
+++ b/tipc/link.c
@@ -217,7 +217,7 @@ static int cmd_link_get_bcast_cb(const struct nlmsghdr *nlh, void *data)
 		print_string(PRINT_ANY, "method", "%s", "AUTOSELECT");
 		close_json_object();
 		open_json_object(NULL);
-		print_uint(PRINT_ANY, "ratio", " ratio:%u%\n",
+		print_uint(PRINT_ANY, "ratio", " ratio:%u\n",
 			   mnl_attr_get_u32(props[prop_ratio]));
 		break;
 	default:
-- 
2.25.1

Re: [tipc-discussion] [net] tipc: fix incorrect unicast link window

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

On 7/3/20 12:48 AM, Tuong Tong Lien wrote:
>
>> -----Original Message-----
>> From: Jon Maloy <jm...@re...>
>> Sent: Friday, July 3, 2020 5:36 AM
>> To: Tuong Tong Lien <tuo...@de...>; ma...@do...; yin...@wi...; tipc-
>> dis...@li...
>> Cc: tipc-dek <tip...@de...>
>> Subject: Re: [tipc-discussion] [net] tipc: fix incorrect unicast link window
>>
>>
>>
>> On 7/2/20 6:28 PM, Jon Maloy wrote:
>>
[...]
>>> Thinking more about this, I would go for the second option.
>>> Either way we will have to be ready to explain to the users why we
>>> have made this change, and that there is no reason for them to touch
>>> this value in the future.
>>>
> An alternative is to introduce an additional property for user to set/get the link max window which will be 8191 by default and keep the legacy one but for the link min window instead. This way, we have no backward compatible issues while still can go beyond that up to the max value which is fully under control as well (if he/she wants to limit the threshold, e.g. for other traffic like TCP, etc., he can consider to lower the max window...).
> In addition, we will still retain the true meaning of the window setting
Changing the meaning of this setting from "maximum window" to "minimum 
window" is for me a far bigger semantic change that changing it from 
"maximum window" to "upper limit for maximum window" as we have done. I 
fear this will be even more confusing for the user.
And we would have to set a limit "maximum minimum window" to avoid that 
the user screws up performance. What should that be? Should we make even 
that limit configurable? And some users might still see their current 
setting being rejected, which I suspect is exactly what they want to 
avoid, and the reason for this issue being raised at all.
> from user that increasing the min window will help increase performance somewhat (i.e. shorten the slow start phase) and vice versa, the same happens for the max window setting as well.
I think the risk is obvious that the user anyway will run into 
surprises. If he has set his window to e.g. 300 he will end up with no 
slow start at all, or his setting being rejected. So he would be better 
off removing this setting anyway.
> We can also display the max value in the link statistics (for example, "Window: 50 packets, up to 8191" or "Window: 123 packets, range 50 - 8191") which acts like a "statement"
or maybe Window (min..cur..max):  [50..123..1000] packets??
This is not a bad idea, whatever solution we choose.

> saying that we currently support variable window and the actual link window can be in that range instead of a fixed one as before... so, user will be completely aware of this.
> There is however a concern for the media & bearer layer's window setting commands, do we need to apply the same change(s) for them as well (current it sets and gets the max window value...)?
We would definitely have to change here too.

I would still suggest we go for my second proposal above. We select a 
default value, e.g., 1000, where we see that there is no improvement in 
increasing it further. Then we let the user set it differently if he 
wants, any value between 50 and 8191.
This would be easy to explain to the user: "we have improved the 
protocol so that we now can set the default window much larger than 
before". How can he object to that, especially if he sees what can be 
perceived as a sane value (1000 instead of 8191) ? The users should 
*not* be allowed to dictate our internal default settings just because 
they are unwilling to change their own. As long as we don't break 
anything or reduce their current performance we remain backwards 
compatible, in my view.

Finally, I apologize for not attending the meeting this morning. It is 
at 6 AM for me, and easy to miss since it is bi-weekly. I will see if I 
can set up Bluejens so that a moderator is not needed, or if we can use 
some other tool. Any suggestions?

///jon


> What do you think?
>
>>> ///jon
>>>
>>>
>>>
>>>> BR/Tuong
>>>>> ///jon
>>>>>
>>>>>> Fixes: 16ad3f4022bb ("tipc: introduce variable window congestion
>>>>>> control")
>>>>>> Reported-by: Hoang Le <hoa...@de...>
>>>>>> Reported-by: Thang Ngo <tha...@de...>
>>>>>> Signed-off-by: Tuong Lien <tuo...@de...>
>>>>>> ---
>>>>>>     net/tipc/eth_media.c | 2 +-
>>>>>>     net/tipc/link.c      | 2 +-
>>>>>>     2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
>>>>>> index 8b0bb600602d..675b947eab89 100644
>>>>>> --- a/net/tipc/eth_media.c
>>>>>> +++ b/net/tipc/eth_media.c
>>>>>> @@ -93,7 +93,7 @@ struct tipc_media eth_media_info = {
>>>>>>         .priority    = TIPC_DEF_LINK_PRI,
>>>>>>         .tolerance    = TIPC_DEF_LINK_TOL,
>>>>>>         .min_win    = TIPC_DEF_LINK_WIN,
>>>>>> -    .max_win    = TIPC_MAX_LINK_WIN,
>>>>>> +    .max_win    = TIPC_DEF_LINK_WIN,
>>>>>>         .type_id    = TIPC_MEDIA_TYPE_ETH,
>>>>>>         .hwaddr_len    = ETH_ALEN,
>>>>>>         .name        = "eth"
>>>>>> diff --git a/net/tipc/link.c b/net/tipc/link.c
>>>>>> index ee3b8d0576b8..28834dafdc98 100644
>>>>>> --- a/net/tipc/link.c
>>>>>> +++ b/net/tipc/link.c
>>>>>> @@ -2662,7 +2662,7 @@ int __tipc_nl_add_link(struct net *net,
>>>>>> struct tipc_nl_msg *msg,
>>>>>>         if (nla_put_u32(msg->skb, TIPC_NLA_PROP_TOL, link->tolerance))
>>>>>>             goto prop_msg_full;
>>>>>>         if (nla_put_u32(msg->skb, TIPC_NLA_PROP_WIN,
>>>>>> -            link->window))
>>>>>> +            link->max_win))
>>>>>>             goto prop_msg_full;
>>>>>>         if (nla_put_u32(msg->skb, TIPC_NLA_PROP_PRIO, link->priority))
>>>>>>             goto prop_msg_full;
>>>
>>>
>>> _______________________________________________
>>> tipc-discussion mailing list
>>> tip...@li...
>>> https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [PATCH] tipc: fix retransmission on unicast links

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

On 7/2/20 8:56 PM, Hamish Martin wrote:
> A scenario has been observed where a 'bc_init' message for a link is not
> retransmitted if it fails to be received by the peer. This leads to the
> peer never establishing the link fully and it discarding all other data
> received on the link.
>
> The issue is traced to the 'nxt_retr' field of the skb not being
> initialised for links that aren't a bc_sndlink. This leads to the
> comparison in tipc_link_advance_transmq() that gates whether to attempt
> retransmission of a message performing in an undesirable way.
> Depending on the relative value of 'jiffies', this comparison:
>      time_before(jiffies, TIPC_SKB_CB(skb)->nxt_retr)
> may return true or false given that 'nxt_retr' remains at the
> uninitialised value of 0 for non bc_sndlinks.
>
> This is most noticeable shortly after boot when jiffies is initialised
> to a high value (to flush out rollover bugs) and we compare a jiffies of,
> say, 4294940189 to zero. In that case time_before returns 'true' leading
> to the skb not being retransmitted.
>
> The fix is to ensure that all skbs have a valid 'nxt_retr' time set for
> them and this is achieved by refactoring the setting of this value into
> a central function.
> With this fix, transmission losses of 'bc_init' messages do not stall
> the link establishment forever because the 'bc_init' message is
> retransmitted and the link eventually establishes correctly.
>
> Signed-off-by: Hamish Martin <ham...@al...>
> ---
>   net/tipc/link.c | 26 ++++++++++++++++++--------
>   1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/net/tipc/link.c b/net/tipc/link.c
> index ee3b8d0576b8..263d950e70e9 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -921,6 +921,21 @@ static void link_prepare_wakeup(struct tipc_link *l)
>   
>   }
>   
> +/**
> + * tipc_link_set_skb_retransmit_time - set the time at which retransmission of
> + *                                     the given skb should be next attempted
> + * @skb: skb to set a future retransmission time for
> + * @l: link the skb will be transmitted on
> + */
> +static void tipc_link_set_skb_retransmit_time(struct sk_buff *skb,
> +					      struct tipc_link *l)
> +{
> +	if (link_is_bc_sndlink(l))
> +		TIPC_SKB_CB(skb)->nxt_retr = TIPC_BC_RETR_LIM;
> +	else
> +		TIPC_SKB_CB(skb)->nxt_retr = TIPC_UC_RETR_TIME;
> +}
> +
>   void tipc_link_reset(struct tipc_link *l)
>   {
>   	struct sk_buff_head list;
> @@ -1036,9 +1051,7 @@ int tipc_link_xmit(struct tipc_link *l, struct sk_buff_head *list,
>   				return -ENOBUFS;
>   			}
>   			__skb_queue_tail(transmq, skb);
> -			/* next retransmit attempt */
> -			if (link_is_bc_sndlink(l))
> -				TIPC_SKB_CB(skb)->nxt_retr = TIPC_BC_RETR_LIM;
> +			tipc_link_set_skb_retransmit_time(skb, l);
>   			__skb_queue_tail(xmitq, _skb);
>   			TIPC_SKB_CB(skb)->ackers = l->ackers;
>   			l->rcv_unacked = 0;
> @@ -1139,9 +1152,7 @@ static void tipc_link_advance_backlog(struct tipc_link *l,
>   		if (unlikely(skb == l->backlog[imp].target_bskb))
>   			l->backlog[imp].target_bskb = NULL;
>   		__skb_queue_tail(&l->transmq, skb);
> -		/* next retransmit attempt */
> -		if (link_is_bc_sndlink(l))
> -			TIPC_SKB_CB(skb)->nxt_retr = TIPC_BC_RETR_LIM;
> +		tipc_link_set_skb_retransmit_time(skb, l);
>   
>   		__skb_queue_tail(xmitq, _skb);
>   		TIPC_SKB_CB(skb)->ackers = l->ackers;
> @@ -1584,8 +1595,7 @@ static int tipc_link_advance_transmq(struct tipc_link *l, struct tipc_link *r,
>   			/* retransmit skb if unrestricted*/
>   			if (time_before(jiffies, TIPC_SKB_CB(skb)->nxt_retr))
>   				continue;
> -			TIPC_SKB_CB(skb)->nxt_retr = (is_uc) ?
> -					TIPC_UC_RETR_TIME : TIPC_BC_RETR_LIM;
> +			tipc_link_set_skb_retransmit_time(skb, l);
>   			_skb = pskb_copy(skb, GFP_ATOMIC);
>   			if (!_skb)
>   				continue;
Nice job!
Acked-by: Jon Maloy <jm...@re...>

Re: [tipc-discussion] [net] tipc: fix incorrect unicast link window

[Tuong T. L. <tuo...@de...>]

> -----Original Message-----
> From: Jon Maloy <jm...@re...>
> Sent: Friday, July 3, 2020 5:36 AM
> To: Tuong Tong Lien <tuo...@de...>; ma...@do...; yin...@wi...; tipc-
> dis...@li...
> Cc: tipc-dek <tip...@de...>
> Subject: Re: [tipc-discussion] [net] tipc: fix incorrect unicast link window
> 
> 
> 
> On 7/2/20 6:28 PM, Jon Maloy wrote:
> >
> >
> > On 6/30/20 9:39 PM, Tuong Tong Lien wrote:
> >>
> >>> -----Original Message-----
> >>> From: Jon Maloy <jm...@re...>
> >>> Sent: Tuesday, June 30, 2020 11:26 PM
> >>> To: Tuong Tong Lien <tuo...@de...>;
> >>> ma...@do...; yin...@wi...; tipc-
> >>> dis...@li...
> >>> Cc: tipc-dek <tip...@de...>
> >>> Subject: Re: [net] tipc: fix incorrect unicast link window
> >>>
> >>>
> >>>
> >>> On 6/30/20 12:56 AM, Tuong Lien wrote:
> >>>> In commit 16ad3f4022bb ("tipc: introduce variable window congestion
> >>>> control"), we enable user to set the link 'max_win' value which is
> >>>> used
> >>>> as the upper threshold for the variable link window algorithm later.
> >>>>
> >>>> However, since it is done by the same netlink command property
> >>>> 'TIPC_NLA_PROP_WIN' that was used to set the actual link window
> >>>> before,
> >>>> it appears to be a non backward compatible issue when user tries to
> >>>> get
> >>>> the value back but finds a different value (i.e. the variable
> >>>> window at
> >>>> that moment).
> >>>>
> >>>> Besides, there is another flaw with the 'max_win' where it is set
> >>>> to be
> >>>> 'TIPC_MAX_LINK_WIN' (i.e. 8191) by default that is obviously
> >>>> unexpected
> >>>> (the variable link window will take place and go beyond that might
> >>>> harm
> >>>> the underlying device...). The value is actually derived from the
> >>>> lower
> >>>> bearer & media layers (i.e. 'eth' media) at the initializing time
> >>>> where
> >>>> the default value should be 'TIPC_DEF_LINK_WIN' (i.e. 50) instead.
> >>>>
> >>>> We fix the issue #1 by returning the 'max_win' which is exactly the
> >>>> one
> >>>> set by user, while setting back the 'max_win' for the 'eth' media
> >>>> to be
> >>>> the said value for the #2.
> >>>>
> >>>> Note: the changes do not affect the variable link window mechanism,
> >>>> but
> >>>> make the right thing i.e. it will work only when user really wants.
> >>> If understand this right variable window flow control will now in
> >>> practice be disabled by default, and only be activated if the user
> >>> explicitly sets max_win to something >> 50. Right?
> >> Yes, I suppose this is the right way, the default value should be at
> >> 50 as before and user has its control fully. Or, do you want it to be
> >> 8181, but I think it is too big by default and we will need to
> >> explain it somehow because one will certainly be surprised to see
> >> that value in TIPC printouts... ?
> > One important property of TIPC is to keep all need for configuration
> > at a minimum. If a user explicitly has to set a configuration value to
> > make TIPC performance optimal  (and we know now that the difference
> > between a fix 50 buffer window and a variable one is huge) I don't
> > find that very satisfactory.
> >
> > But I admit we have a dilemma. What we have done is to change the
> > semantic meaning of setting/getting the window from "fix sliding
> > protocol window" to "max allowable window".  I can quite well
> > understand that someone within /// found it confusing to discover that
> > a value that was previously 50 now is 8181, and wants it back to how
> > it  used to be.
> >
> > However, I don't think this kind of "user expectation" about a deeply
> > internal setting of TIPC is a valid reason to cripple protocol
> > performance. In the past the users would set this value to achieve
> > better performance, while it in reality is irrelevant now, -it can
> > only make performance poorer.
> >
> > In my view we have two options:
> > 1) We could add a dummy value that sets and reads the user's value
> > just to make him happy.
> > 2) We could set the default value to e.g. 1000 to make it look more
> > reasonable to the user, but with no practical effect on the protocol.
> This has to be verified of course.
> ///jon
> > Then, if the user has his own legacy setting at e.g. 300 he will
> > probably discover poorer performance, and choose to keep the default
> > value. And if he doesn't then it is his own problem.
> >
> > Thinking more about this, I would go for the second option.
> > Either way we will have to be ready to explain to the users why we
> > have made this change, and that there is no reason for them to touch
> > this value in the future.
> >
An alternative is to introduce an additional property for user to set/get the link max window which will be 8191 by default and keep the legacy one but for the link min window instead. This way, we have no backward compatible issues while still can go beyond that up to the max value which is fully under control as well (if he/she wants to limit the threshold, e.g. for other traffic like TCP, etc., he can consider to lower the max window...). In addition, we will still retain the true meaning of the window setting from user that increasing the min window will help increase performance somewhat (i.e. shorten the slow start phase) and vice versa, the same happens for the max window setting as well.
We can also display the max value in the link statistics (for example, "Window: 50 packets, up to 8191" or "Window: 123 packets, range 50 - 8191") which acts like a "statement" saying that we currently support variable window and the actual link window can be in that range instead of a fixed one as before... so, user will be completely aware of this.
There is however a concern for the media & bearer layer's window setting commands, do we need to apply the same change(s) for them as well (current it sets and gets the max window value...)?
What do you think?

> > ///jon
> >
> >
> >
> >>
> >> BR/Tuong
> >>> ///jon
> >>>
> >>>> Fixes: 16ad3f4022bb ("tipc: introduce variable window congestion
> >>>> control")
> >>>> Reported-by: Hoang Le <hoa...@de...>
> >>>> Reported-by: Thang Ngo <tha...@de...>
> >>>> Signed-off-by: Tuong Lien <tuo...@de...>
> >>>> ---
> >>>>    net/tipc/eth_media.c | 2 +-
> >>>>    net/tipc/link.c      | 2 +-
> >>>>    2 files changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
> >>>> index 8b0bb600602d..675b947eab89 100644
> >>>> --- a/net/tipc/eth_media.c
> >>>> +++ b/net/tipc/eth_media.c
> >>>> @@ -93,7 +93,7 @@ struct tipc_media eth_media_info = {
> >>>>        .priority    = TIPC_DEF_LINK_PRI,
> >>>>        .tolerance    = TIPC_DEF_LINK_TOL,
> >>>>        .min_win    = TIPC_DEF_LINK_WIN,
> >>>> -    .max_win    = TIPC_MAX_LINK_WIN,
> >>>> +    .max_win    = TIPC_DEF_LINK_WIN,
> >>>>        .type_id    = TIPC_MEDIA_TYPE_ETH,
> >>>>        .hwaddr_len    = ETH_ALEN,
> >>>>        .name        = "eth"
> >>>> diff --git a/net/tipc/link.c b/net/tipc/link.c
> >>>> index ee3b8d0576b8..28834dafdc98 100644
> >>>> --- a/net/tipc/link.c
> >>>> +++ b/net/tipc/link.c
> >>>> @@ -2662,7 +2662,7 @@ int __tipc_nl_add_link(struct net *net,
> >>>> struct tipc_nl_msg *msg,
> >>>>        if (nla_put_u32(msg->skb, TIPC_NLA_PROP_TOL, link->tolerance))
> >>>>            goto prop_msg_full;
> >>>>        if (nla_put_u32(msg->skb, TIPC_NLA_PROP_WIN,
> >>>> -            link->window))
> >>>> +            link->max_win))
> >>>>            goto prop_msg_full;
> >>>>        if (nla_put_u32(msg->skb, TIPC_NLA_PROP_PRIO, link->priority))
> >>>>            goto prop_msg_full;
> >
> >
> >
> > _______________________________________________
> > tipc-discussion mailing list
> > tip...@li...
> > https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net] tipc: fix incorrect unicast link window

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

On 7/2/20 6:28 PM, Jon Maloy wrote:
>
>
> On 6/30/20 9:39 PM, Tuong Tong Lien wrote:
>>
>>> -----Original Message-----
>>> From: Jon Maloy <jm...@re...>
>>> Sent: Tuesday, June 30, 2020 11:26 PM
>>> To: Tuong Tong Lien <tuo...@de...>; 
>>> ma...@do...; yin...@wi...; tipc-
>>> dis...@li...
>>> Cc: tipc-dek <tip...@de...>
>>> Subject: Re: [net] tipc: fix incorrect unicast link window
>>>
>>>
>>>
>>> On 6/30/20 12:56 AM, Tuong Lien wrote:
>>>> In commit 16ad3f4022bb ("tipc: introduce variable window congestion
>>>> control"), we enable user to set the link 'max_win' value which is 
>>>> used
>>>> as the upper threshold for the variable link window algorithm later.
>>>>
>>>> However, since it is done by the same netlink command property
>>>> 'TIPC_NLA_PROP_WIN' that was used to set the actual link window 
>>>> before,
>>>> it appears to be a non backward compatible issue when user tries to 
>>>> get
>>>> the value back but finds a different value (i.e. the variable 
>>>> window at
>>>> that moment).
>>>>
>>>> Besides, there is another flaw with the 'max_win' where it is set 
>>>> to be
>>>> 'TIPC_MAX_LINK_WIN' (i.e. 8191) by default that is obviously 
>>>> unexpected
>>>> (the variable link window will take place and go beyond that might 
>>>> harm
>>>> the underlying device...). The value is actually derived from the 
>>>> lower
>>>> bearer & media layers (i.e. 'eth' media) at the initializing time 
>>>> where
>>>> the default value should be 'TIPC_DEF_LINK_WIN' (i.e. 50) instead.
>>>>
>>>> We fix the issue #1 by returning the 'max_win' which is exactly the 
>>>> one
>>>> set by user, while setting back the 'max_win' for the 'eth' media 
>>>> to be
>>>> the said value for the #2.
>>>>
>>>> Note: the changes do not affect the variable link window mechanism, 
>>>> but
>>>> make the right thing i.e. it will work only when user really wants.
>>> If understand this right variable window flow control will now in
>>> practice be disabled by default, and only be activated if the user
>>> explicitly sets max_win to something >> 50. Right?
>> Yes, I suppose this is the right way, the default value should be at 
>> 50 as before and user has its control fully. Or, do you want it to be 
>> 8181, but I think it is too big by default and we will need to 
>> explain it somehow because one will certainly be surprised to see 
>> that value in TIPC printouts... ?
> One important property of TIPC is to keep all need for configuration 
> at a minimum. If a user explicitly has to set a configuration value to 
> make TIPC performance optimal  (and we know now that the difference 
> between a fix 50 buffer window and a variable one is huge) I don't 
> find that very satisfactory.
>
> But I admit we have a dilemma. What we have done is to change the 
> semantic meaning of setting/getting the window from "fix sliding 
> protocol window" to "max allowable window".  I can quite well 
> understand that someone within /// found it confusing to discover that 
> a value that was previously 50 now is 8181, and wants it back to how 
> it  used to be.
>
> However, I don't think this kind of "user expectation" about a deeply 
> internal setting of TIPC is a valid reason to cripple protocol 
> performance. In the past the users would set this value to achieve 
> better performance, while it in reality is irrelevant now, -it can 
> only make performance poorer.
>
> In my view we have two options:
> 1) We could add a dummy value that sets and reads the user's value 
> just to make him happy.
> 2) We could set the default value to e.g. 1000 to make it look more 
> reasonable to the user, but with no practical effect on the protocol. 
This has to be verified of course.
///jon
> Then, if the user has his own legacy setting at e.g. 300 he will 
> probably discover poorer performance, and choose to keep the default 
> value. And if he doesn't then it is his own problem.
>
> Thinking more about this, I would go for the second option.
> Either way we will have to be ready to explain to the users why we 
> have made this change, and that there is no reason for them to touch 
> this value in the future.
>
> ///jon
>
>
>
>>
>> BR/Tuong
>>> ///jon
>>>
>>>> Fixes: 16ad3f4022bb ("tipc: introduce variable window congestion 
>>>> control")
>>>> Reported-by: Hoang Le <hoa...@de...>
>>>> Reported-by: Thang Ngo <tha...@de...>
>>>> Signed-off-by: Tuong Lien <tuo...@de...>
>>>> ---
>>>>    net/tipc/eth_media.c | 2 +-
>>>>    net/tipc/link.c      | 2 +-
>>>>    2 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
>>>> index 8b0bb600602d..675b947eab89 100644
>>>> --- a/net/tipc/eth_media.c
>>>> +++ b/net/tipc/eth_media.c
>>>> @@ -93,7 +93,7 @@ struct tipc_media eth_media_info = {
>>>>        .priority    = TIPC_DEF_LINK_PRI,
>>>>        .tolerance    = TIPC_DEF_LINK_TOL,
>>>>        .min_win    = TIPC_DEF_LINK_WIN,
>>>> -    .max_win    = TIPC_MAX_LINK_WIN,
>>>> +    .max_win    = TIPC_DEF_LINK_WIN,
>>>>        .type_id    = TIPC_MEDIA_TYPE_ETH,
>>>>        .hwaddr_len    = ETH_ALEN,
>>>>        .name        = "eth"
>>>> diff --git a/net/tipc/link.c b/net/tipc/link.c
>>>> index ee3b8d0576b8..28834dafdc98 100644
>>>> --- a/net/tipc/link.c
>>>> +++ b/net/tipc/link.c
>>>> @@ -2662,7 +2662,7 @@ int __tipc_nl_add_link(struct net *net, 
>>>> struct tipc_nl_msg *msg,
>>>>        if (nla_put_u32(msg->skb, TIPC_NLA_PROP_TOL, link->tolerance))
>>>>            goto prop_msg_full;
>>>>        if (nla_put_u32(msg->skb, TIPC_NLA_PROP_WIN,
>>>> -            link->window))
>>>> +            link->max_win))
>>>>            goto prop_msg_full;
>>>>        if (nla_put_u32(msg->skb, TIPC_NLA_PROP_PRIO, link->priority))
>>>>            goto prop_msg_full;
>
>
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net] tipc: fix incorrect unicast link window

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

On 6/30/20 9:39 PM, Tuong Tong Lien wrote:
>
>> -----Original Message-----
>> From: Jon Maloy <jm...@re...>
>> Sent: Tuesday, June 30, 2020 11:26 PM
>> To: Tuong Tong Lien <tuo...@de...>; ma...@do...; yin...@wi...; tipc-
>> dis...@li...
>> Cc: tipc-dek <tip...@de...>
>> Subject: Re: [net] tipc: fix incorrect unicast link window
>>
>>
>>
>> On 6/30/20 12:56 AM, Tuong Lien wrote:
>>> In commit 16ad3f4022bb ("tipc: introduce variable window congestion
>>> control"), we enable user to set the link 'max_win' value which is used
>>> as the upper threshold for the variable link window algorithm later.
>>>
>>> However, since it is done by the same netlink command property
>>> 'TIPC_NLA_PROP_WIN' that was used to set the actual link window before,
>>> it appears to be a non backward compatible issue when user tries to get
>>> the value back but finds a different value (i.e. the variable window at
>>> that moment).
>>>
>>> Besides, there is another flaw with the 'max_win' where it is set to be
>>> 'TIPC_MAX_LINK_WIN' (i.e. 8191) by default that is obviously unexpected
>>> (the variable link window will take place and go beyond that might harm
>>> the underlying device...). The value is actually derived from the lower
>>> bearer & media layers (i.e. 'eth' media) at the initializing time where
>>> the default value should be 'TIPC_DEF_LINK_WIN' (i.e. 50) instead.
>>>
>>> We fix the issue #1 by returning the 'max_win' which is exactly the one
>>> set by user, while setting back the 'max_win' for the 'eth' media to be
>>> the said value for the #2.
>>>
>>> Note: the changes do not affect the variable link window mechanism, but
>>> make the right thing i.e. it will work only when user really wants.
>> If understand this right variable window flow control will now in
>> practice be disabled by default, and only be activated if the user
>> explicitly sets max_win to something >> 50. Right?
> Yes, I suppose this is the right way, the default value should be at 50 as before and user has its control fully. Or, do you want it to be 8181, but I think it is too big by default and we will need to explain it somehow because one will certainly be surprised to see that value in TIPC printouts... ?
One important property of TIPC is to keep all need for configuration at 
a minimum. If a user explicitly has to set a configuration value to make 
TIPC performance optimal  (and we know now that the difference between a 
fix 50 buffer window and a variable one is huge) I don't find that very 
satisfactory.

But I admit we have a dilemma. What we have done is to change the 
semantic meaning of setting/getting the window from "fix sliding 
protocol window" to "max allowable window".  I can quite well understand 
that someone within /// found it confusing to discover that a value that 
was previously 50 now is 8181, and wants it back to how it  used to be.

However, I don't think this kind of "user expectation" about a deeply 
internal setting of TIPC is a valid reason to cripple protocol 
performance. In the past the users would set this value to achieve 
better performance, while it in reality is irrelevant now, -it can only 
make performance poorer.

In my view we have two options:
1) We could add a dummy value that sets and reads the user's value just 
to make him happy.
2) We could set the default value to e.g. 1000 to make it look more 
reasonable to the user, but with no practical effect on the protocol. 
Then, if the user has his own legacy setting at e.g. 300 he will 
probably discover poorer performance, and choose to keep the default 
value. And if he doesn't then it is his own problem.

Thinking more about this, I would go for the second option.
Either way we will have to be ready to explain to the users why we have 
made this change, and that there is no reason for them to touch this 
value in the future.

///jon



>
> BR/Tuong
>> ///jon
>>
>>> Fixes: 16ad3f4022bb ("tipc: introduce variable window congestion control")
>>> Reported-by: Hoang Le <hoa...@de...>
>>> Reported-by: Thang Ngo <tha...@de...>
>>> Signed-off-by: Tuong Lien <tuo...@de...>
>>> ---
>>>    net/tipc/eth_media.c | 2 +-
>>>    net/tipc/link.c      | 2 +-
>>>    2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
>>> index 8b0bb600602d..675b947eab89 100644
>>> --- a/net/tipc/eth_media.c
>>> +++ b/net/tipc/eth_media.c
>>> @@ -93,7 +93,7 @@ struct tipc_media eth_media_info = {
>>>    	.priority	= TIPC_DEF_LINK_PRI,
>>>    	.tolerance	= TIPC_DEF_LINK_TOL,
>>>    	.min_win	= TIPC_DEF_LINK_WIN,
>>> -	.max_win	= TIPC_MAX_LINK_WIN,
>>> +	.max_win	= TIPC_DEF_LINK_WIN,
>>>    	.type_id	= TIPC_MEDIA_TYPE_ETH,
>>>    	.hwaddr_len	= ETH_ALEN,
>>>    	.name		= "eth"
>>> diff --git a/net/tipc/link.c b/net/tipc/link.c
>>> index ee3b8d0576b8..28834dafdc98 100644
>>> --- a/net/tipc/link.c
>>> +++ b/net/tipc/link.c
>>> @@ -2662,7 +2662,7 @@ int __tipc_nl_add_link(struct net *net, struct tipc_nl_msg *msg,
>>>    	if (nla_put_u32(msg->skb, TIPC_NLA_PROP_TOL, link->tolerance))
>>>    		goto prop_msg_full;
>>>    	if (nla_put_u32(msg->skb, TIPC_NLA_PROP_WIN,
>>> -			link->window))
>>> +			link->max_win))
>>>    		goto prop_msg_full;
>>>    	if (nla_put_u32(msg->skb, TIPC_NLA_PROP_PRIO, link->priority))
>>>    		goto prop_msg_full;

Re: [tipc-discussion] [net] tipc: fix incorrect unicast link window

[Tuong T. L. <tuo...@de...>]

> -----Original Message-----
> From: Jon Maloy <jm...@re...>
> Sent: Tuesday, June 30, 2020 11:26 PM
> To: Tuong Tong Lien <tuo...@de...>; ma...@do...; yin...@wi...; tipc-
> dis...@li...
> Cc: tipc-dek <tip...@de...>
> Subject: Re: [net] tipc: fix incorrect unicast link window
> 
> 
> 
> On 6/30/20 12:56 AM, Tuong Lien wrote:
> > In commit 16ad3f4022bb ("tipc: introduce variable window congestion
> > control"), we enable user to set the link 'max_win' value which is used
> > as the upper threshold for the variable link window algorithm later.
> >
> > However, since it is done by the same netlink command property
> > 'TIPC_NLA_PROP_WIN' that was used to set the actual link window before,
> > it appears to be a non backward compatible issue when user tries to get
> > the value back but finds a different value (i.e. the variable window at
> > that moment).
> >
> > Besides, there is another flaw with the 'max_win' where it is set to be
> > 'TIPC_MAX_LINK_WIN' (i.e. 8191) by default that is obviously unexpected
> > (the variable link window will take place and go beyond that might harm
> > the underlying device...). The value is actually derived from the lower
> > bearer & media layers (i.e. 'eth' media) at the initializing time where
> > the default value should be 'TIPC_DEF_LINK_WIN' (i.e. 50) instead.
> >
> > We fix the issue #1 by returning the 'max_win' which is exactly the one
> > set by user, while setting back the 'max_win' for the 'eth' media to be
> > the said value for the #2.
> >
> > Note: the changes do not affect the variable link window mechanism, but
> > make the right thing i.e. it will work only when user really wants.
> If understand this right variable window flow control will now in
> practice be disabled by default, and only be activated if the user
> explicitly sets max_win to something >> 50. Right?
Yes, I suppose this is the right way, the default value should be at 50 as before and user has its control fully. Or, do you want it to be 8181, but I think it is too big by default and we will need to explain it somehow because one will certainly be surprised to see that value in TIPC printouts... ?

BR/Tuong
> ///jon
> 
> >
> > Fixes: 16ad3f4022bb ("tipc: introduce variable window congestion control")
> > Reported-by: Hoang Le <hoa...@de...>
> > Reported-by: Thang Ngo <tha...@de...>
> > Signed-off-by: Tuong Lien <tuo...@de...>
> > ---
> >   net/tipc/eth_media.c | 2 +-
> >   net/tipc/link.c      | 2 +-
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
> > index 8b0bb600602d..675b947eab89 100644
> > --- a/net/tipc/eth_media.c
> > +++ b/net/tipc/eth_media.c
> > @@ -93,7 +93,7 @@ struct tipc_media eth_media_info = {
> >   	.priority	= TIPC_DEF_LINK_PRI,
> >   	.tolerance	= TIPC_DEF_LINK_TOL,
> >   	.min_win	= TIPC_DEF_LINK_WIN,
> > -	.max_win	= TIPC_MAX_LINK_WIN,
> > +	.max_win	= TIPC_DEF_LINK_WIN,
> >   	.type_id	= TIPC_MEDIA_TYPE_ETH,
> >   	.hwaddr_len	= ETH_ALEN,
> >   	.name		= "eth"
> > diff --git a/net/tipc/link.c b/net/tipc/link.c
> > index ee3b8d0576b8..28834dafdc98 100644
> > --- a/net/tipc/link.c
> > +++ b/net/tipc/link.c
> > @@ -2662,7 +2662,7 @@ int __tipc_nl_add_link(struct net *net, struct tipc_nl_msg *msg,
> >   	if (nla_put_u32(msg->skb, TIPC_NLA_PROP_TOL, link->tolerance))
> >   		goto prop_msg_full;
> >   	if (nla_put_u32(msg->skb, TIPC_NLA_PROP_WIN,
> > -			link->window))
> > +			link->max_win))
> >   		goto prop_msg_full;
> >   	if (nla_put_u32(msg->skb, TIPC_NLA_PROP_PRIO, link->priority))
> >   		goto prop_msg_full;

Re: [tipc-discussion] [net] tipc: fix incorrect unicast link window

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

On 6/30/20 12:56 AM, Tuong Lien wrote:
> In commit 16ad3f4022bb ("tipc: introduce variable window congestion
> control"), we enable user to set the link 'max_win' value which is used
> as the upper threshold for the variable link window algorithm later.
>
> However, since it is done by the same netlink command property
> 'TIPC_NLA_PROP_WIN' that was used to set the actual link window before,
> it appears to be a non backward compatible issue when user tries to get
> the value back but finds a different value (i.e. the variable window at
> that moment).
>
> Besides, there is another flaw with the 'max_win' where it is set to be
> 'TIPC_MAX_LINK_WIN' (i.e. 8191) by default that is obviously unexpected
> (the variable link window will take place and go beyond that might harm
> the underlying device...). The value is actually derived from the lower
> bearer & media layers (i.e. 'eth' media) at the initializing time where
> the default value should be 'TIPC_DEF_LINK_WIN' (i.e. 50) instead.
>
> We fix the issue #1 by returning the 'max_win' which is exactly the one
> set by user, while setting back the 'max_win' for the 'eth' media to be
> the said value for the #2.
>
> Note: the changes do not affect the variable link window mechanism, but
> make the right thing i.e. it will work only when user really wants.
If understand this right variable window flow control will now in 
practice be disabled by default, and only be activated if the user 
explicitly sets max_win to something >> 50. Right?

///jon

>
> Fixes: 16ad3f4022bb ("tipc: introduce variable window congestion control")
> Reported-by: Hoang Le <hoa...@de...>
> Reported-by: Thang Ngo <tha...@de...>
> Signed-off-by: Tuong Lien <tuo...@de...>
> ---
>   net/tipc/eth_media.c | 2 +-
>   net/tipc/link.c      | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
> index 8b0bb600602d..675b947eab89 100644
> --- a/net/tipc/eth_media.c
> +++ b/net/tipc/eth_media.c
> @@ -93,7 +93,7 @@ struct tipc_media eth_media_info = {
>   	.priority	= TIPC_DEF_LINK_PRI,
>   	.tolerance	= TIPC_DEF_LINK_TOL,
>   	.min_win	= TIPC_DEF_LINK_WIN,
> -	.max_win	= TIPC_MAX_LINK_WIN,
> +	.max_win	= TIPC_DEF_LINK_WIN,
>   	.type_id	= TIPC_MEDIA_TYPE_ETH,
>   	.hwaddr_len	= ETH_ALEN,
>   	.name		= "eth"
> diff --git a/net/tipc/link.c b/net/tipc/link.c
> index ee3b8d0576b8..28834dafdc98 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -2662,7 +2662,7 @@ int __tipc_nl_add_link(struct net *net, struct tipc_nl_msg *msg,
>   	if (nla_put_u32(msg->skb, TIPC_NLA_PROP_TOL, link->tolerance))
>   		goto prop_msg_full;
>   	if (nla_put_u32(msg->skb, TIPC_NLA_PROP_WIN,
> -			link->window))
> +			link->max_win))
>   		goto prop_msg_full;
>   	if (nla_put_u32(msg->skb, TIPC_NLA_PROP_PRIO, link->priority))
>   		goto prop_msg_full;

[tipc-discussion] [net] tipc: fix incorrect unicast link window

[Tuong L. <tuo...@de...>]

In commit 16ad3f4022bb ("tipc: introduce variable window congestion
control"), we enable user to set the link 'max_win' value which is used
as the upper threshold for the variable link window algorithm later.

However, since it is done by the same netlink command property
'TIPC_NLA_PROP_WIN' that was used to set the actual link window before,
it appears to be a non backward compatible issue when user tries to get
the value back but finds a different value (i.e. the variable window at
that moment).

Besides, there is another flaw with the 'max_win' where it is set to be
'TIPC_MAX_LINK_WIN' (i.e. 8191) by default that is obviously unexpected
(the variable link window will take place and go beyond that might harm
the underlying device...). The value is actually derived from the lower
bearer & media layers (i.e. 'eth' media) at the initializing time where
the default value should be 'TIPC_DEF_LINK_WIN' (i.e. 50) instead.

We fix the issue #1 by returning the 'max_win' which is exactly the one
set by user, while setting back the 'max_win' for the 'eth' media to be
the said value for the #2.

Note: the changes do not affect the variable link window mechanism, but
make the right thing i.e. it will work only when user really wants.

Fixes: 16ad3f4022bb ("tipc: introduce variable window congestion control")
Reported-by: Hoang Le <hoa...@de...>
Reported-by: Thang Ngo <tha...@de...>
Signed-off-by: Tuong Lien <tuo...@de...>
---
 net/tipc/eth_media.c | 2 +-
 net/tipc/link.c      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
index 8b0bb600602d..675b947eab89 100644
--- a/net/tipc/eth_media.c
+++ b/net/tipc/eth_media.c
@@ -93,7 +93,7 @@ struct tipc_media eth_media_info = {
 	.priority	= TIPC_DEF_LINK_PRI,
 	.tolerance	= TIPC_DEF_LINK_TOL,
 	.min_win	= TIPC_DEF_LINK_WIN,
-	.max_win	= TIPC_MAX_LINK_WIN,
+	.max_win	= TIPC_DEF_LINK_WIN,
 	.type_id	= TIPC_MEDIA_TYPE_ETH,
 	.hwaddr_len	= ETH_ALEN,
 	.name		= "eth"
diff --git a/net/tipc/link.c b/net/tipc/link.c
index ee3b8d0576b8..28834dafdc98 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -2662,7 +2662,7 @@ int __tipc_nl_add_link(struct net *net, struct tipc_nl_msg *msg,
 	if (nla_put_u32(msg->skb, TIPC_NLA_PROP_TOL, link->tolerance))
 		goto prop_msg_full;
 	if (nla_put_u32(msg->skb, TIPC_NLA_PROP_WIN,
-			link->window))
+			link->max_win))
 		goto prop_msg_full;
 	if (nla_put_u32(msg->skb, TIPC_NLA_PROP_PRIO, link->priority))
 		goto prop_msg_full;
-- 
2.13.7

Re: [tipc-discussion] [PATCH][next] tipc: Use struct_size() helper

[David M. <da...@da...>]

From: "Gustavo A. R. Silva" <gus...@ke...>
Date: Thu, 18 Jun 2020 08:35:00 -0500

> Make use of the struct_size() helper instead of an open-coded version
> in order to avoid any potential type mistakes.
> 
> This code was detected with the help of Coccinelle and, audited and
> fixed manually.
> 
> Signed-off-by: Gustavo A. R. Silva <gus...@ke...>

Applied, thank you.

Re: [tipc-discussion] [net] tipc: fix NULL pointer dereference in tipc_disc_rcv()

[David M. <da...@da...>]

From: Tuong Lien <tuo...@de...>
Date: Thu, 11 Jun 2020 17:08:08 +0700

> When a bearer is enabled, we create a 'tipc_discoverer' object to store
> the bearer related data along with a timer and a preformatted discovery
> message buffer for later probing... However, this is only carried after
> the bearer was set 'up', that left a race condition resulting in kernel
> panic.
> 
> It occurs when a discovery message from a peer node is received and
> processed in bottom half (since the bearer is 'up' already) just before
> the discoverer object is created but is now accessed in order to update
> the preformatted buffer (with a new trial address, ...) so leads to the
> NULL pointer dereference.
> 
> We solve the problem by simply moving the bearer 'up' setting to later,
> so make sure everything is ready prior to any message receiving.
> 
> Acked-by: Jon Maloy <jm...@re...>
> Signed-off-by: Tuong Lien <tuo...@de...>

Applied, thanks.

Re: [tipc-discussion] [net] tipc: fix kernel WARNING in tipc_msg_append()

[David M. <da...@da...>]

From: Tuong Lien <tuo...@de...>
Date: Thu, 11 Jun 2020 17:07:35 +0700

> syzbot found the following issue:
> 
> WARNING: CPU: 0 PID: 6808 at include/linux/thread_info.h:150 check_copy_size include/linux/thread_info.h:150 [inline]
> WARNING: CPU: 0 PID: 6808 at include/linux/thread_info.h:150 copy_from_iter include/linux/uio.h:144 [inline]
> WARNING: CPU: 0 PID: 6808 at include/linux/thread_info.h:150 tipc_msg_append+0x49a/0x5e0 net/tipc/msg.c:242
> Kernel panic - not syncing: panic_on_warn set ...
> 
> This happens after commit 5e9eeccc58f3 ("tipc: fix NULL pointer
> dereference in streaming") that tried to build at least one buffer even
> when the message data length is zero... However, it now exposes another
> bug that the 'mss' can be zero and the 'cpy' will be negative, thus the
> above kernel WARNING will appear!
> The zero value of 'mss' is never expected because it means Nagle is not
> enabled for the socket (actually the socket type was 'SOCK_SEQPACKET'),
> so the function 'tipc_msg_append()' must not be called at all. But that
> was in this particular case since the message data length was zero, and
> the 'send <= maxnagle' check became true.
> 
> We resolve the issue by explicitly checking if Nagle is enabled for the
> socket, i.e. 'maxnagle != 0' before calling the 'tipc_msg_append()'. We
> also reinforce the function to against such a negative values if any.
> 
> Reported-by: syz...@sy...
> Fixes: c0bceb97db9e ("tipc: add smart nagle feature")
> Acked-by: Jon Maloy <jm...@re...>
> Signed-off-by: Tuong Lien <tuo...@de...>

Applied and queued up for v5.5+ -stable.

[tipc-discussion] [net] tipc: fix kernel WARNING in tipc_msg_append()

[Tuong L. <tuo...@de...>]

syzbot found the following issue:

WARNING: CPU: 0 PID: 6808 at include/linux/thread_info.h:150 check_copy_size include/linux/thread_info.h:150 [inline]
WARNING: CPU: 0 PID: 6808 at include/linux/thread_info.h:150 copy_from_iter include/linux/uio.h:144 [inline]
WARNING: CPU: 0 PID: 6808 at include/linux/thread_info.h:150 tipc_msg_append+0x49a/0x5e0 net/tipc/msg.c:242
Kernel panic - not syncing: panic_on_warn set ...

This happens after commit 5e9eeccc58f3 ("tipc: fix NULL pointer
dereference in streaming") that tried to build at least one buffer even
when the message data length is zero... However, it now exposes another
bug that the 'mss' can be zero and the 'cpy' will be negative, thus the
above kernel WARNING will appear!
The zero value of 'mss' is never expected because it means Nagle is not
enabled for the socket (actually the socket type was 'SOCK_SEQPACKET'),
so the function 'tipc_msg_append()' must not be called at all. But that
was in this particular case since the message data length was zero, and
the 'send <= maxnagle' check became true.

We resolve the issue by explicitly checking if Nagle is enabled for the
socket, i.e. 'maxnagle != 0' before calling the 'tipc_msg_append()'. We
also reinforce the function to against such a negative values if any.

Reported-by: syz...@sy...
Fixes: c0bceb97db9e ("tipc: add smart nagle feature")
Acked-by: Jon Maloy <jm...@re...>
Signed-off-by: Tuong Lien <tuo...@de...>
---
 net/tipc/msg.c    | 4 ++--
 net/tipc/socket.c | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 046e4cb3acea..01b64869a173 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -238,14 +238,14 @@ int tipc_msg_append(struct tipc_msg *_hdr, struct msghdr *m, int dlen,
 		hdr = buf_msg(skb);
 		curr = msg_blocks(hdr);
 		mlen = msg_size(hdr);
-		cpy = min_t(int, rem, mss - mlen);
+		cpy = min_t(size_t, rem, mss - mlen);
 		if (cpy != copy_from_iter(skb->data + mlen, cpy, &m->msg_iter))
 			return -EFAULT;
 		msg_set_size(hdr, mlen + cpy);
 		skb_put(skb, cpy);
 		rem -= cpy;
 		total += msg_blocks(hdr) - curr;
-	} while (rem);
+	} while (rem > 0);
 	return total - accounted;
 }
 
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 26123f4177fd..a94f38333698 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1574,7 +1574,8 @@ static int __tipc_sendstream(struct socket *sock, struct msghdr *m, size_t dlen)
 			break;
 		send = min_t(size_t, dlen - sent, TIPC_MAX_USER_MSG_SIZE);
 		blocks = tsk->snd_backlog;
-		if (tsk->oneway++ >= tsk->nagle_start && send <= maxnagle) {
+		if (tsk->oneway++ >= tsk->nagle_start && maxnagle &&
+		    send <= maxnagle) {
 			rc = tipc_msg_append(hdr, m, send, maxnagle, txq);
 			if (unlikely(rc < 0))
 				break;
-- 
2.13.7

[tipc-discussion] [net] tipc: fix NULL pointer dereference in tipc_disc_rcv()

[Tuong L. <tuo...@de...>]

When a bearer is enabled, we create a 'tipc_discoverer' object to store
the bearer related data along with a timer and a preformatted discovery
message buffer for later probing... However, this is only carried after
the bearer was set 'up', that left a race condition resulting in kernel
panic.

It occurs when a discovery message from a peer node is received and
processed in bottom half (since the bearer is 'up' already) just before
the discoverer object is created but is now accessed in order to update
the preformatted buffer (with a new trial address, ...) so leads to the
NULL pointer dereference.

We solve the problem by simply moving the bearer 'up' setting to later,
so make sure everything is ready prior to any message receiving.

Acked-by: Jon Maloy <jm...@re...>
Signed-off-by: Tuong Lien <tuo...@de...>
---
 net/tipc/bearer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 34ca7b789eba..e366ec9a7e4d 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -316,7 +316,6 @@ static int tipc_enable_bearer(struct net *net, const char *name,
 	b->domain = disc_domain;
 	b->net_plane = bearer_id + 'A';
 	b->priority = prio;
-	test_and_set_bit_lock(0, &b->up);
 	refcount_set(&b->refcnt, 1);
 
 	res = tipc_disc_create(net, b, &b->bcast_addr, &skb);
@@ -326,6 +325,7 @@ static int tipc_enable_bearer(struct net *net, const char *name,
 		goto rejected;
 	}
 
+	test_and_set_bit_lock(0, &b->up);
 	rcu_assign_pointer(tn->bearer_list[bearer_id], b);
 	if (skb)
 		tipc_bearer_xmit_skb(net, bearer_id, skb, &b->bcast_addr);
-- 
2.13.7

Re: [tipc-discussion] [net] tipc: fix kernel WARNING in tipc_msg_append()

[Tuong T. L. <tuo...@de...>]

> -----Original Message-----
> From: Jon Maloy <jm...@re...>
> Sent: Tuesday, June 9, 2020 8:20 PM
> To: Tuong Tong Lien <tuo...@de...>; ma...@do...; yin...@wi...; tipc-
> dis...@li...
> Cc: tipc-dek <tip...@de...>
> Subject: Re: [net] tipc: fix kernel WARNING in tipc_msg_append()
> 
> 
> 
> On 6/8/20 11:55 PM, Tuong Lien wrote:
> > syzbot found the following issue:
> >
> > WARNING: CPU: 0 PID: 6808 at include/linux/thread_info.h:150 check_copy_size include/linux/thread_info.h:150 [inline]
> > WARNING: CPU: 0 PID: 6808 at include/linux/thread_info.h:150 copy_from_iter include/linux/uio.h:144 [inline]
> > WARNING: CPU: 0 PID: 6808 at include/linux/thread_info.h:150 tipc_msg_append+0x49a/0x5e0 net/tipc/msg.c:242
> > Kernel panic - not syncing: panic_on_warn set ...
> >
> > This happens after commit 5e9eeccc58f3 ("tipc: fix NULL pointer
> > dereference in streaming") that tried to build at least one buffer even
> > when the message data length is zero... However, it now exposes another
> > bug that the 'mss' can be zero and the 'cpy' will be negative, thus the
> > above kernel WARNING will appear!
> > The zero value of 'mss' is never expected because it means Nagle is not
> > enabled for the socket (actually the socket type was 'SOCK_SEQPACKET'),
> > so the function 'tipc_msg_append()' must not be called at all. But that
> > was in this particular case since the message data length was zero, and
> > the 'send <= maxnagle' check became true.
> >
> > We resolve the issue by explicitly checking if Nagle is enabled for the
> > socket, i.e. 'maxnagle != 0' before calling the 'tipc_msg_append()'. In
> > addition, we put a sanity check in the function to avoid calling the
> > 'copy_from_iter()' with a negative size and doing an infinite loop.
> ---
> Same suggestion as I had to Hoang; add the three dashes above to avoid that
> the version info by accident becomes part of the commit log.
Thanks Jon, I usually remove it before posting it to netdev as well.

BR/Tuong
> >
> > v2: use 'size_t' in the 'min_t()' to get a proper value of 'cpy' (after
> >      Jon's comment)
> >
> > Reported-by: syz...@sy...
> > Fixes: c0bceb97db9e ("tipc: add smart nagle feature")
> > Signed-off-by: Tuong Lien <tuo...@de...>
> > ---
> >   net/tipc/msg.c    | 4 ++--
> >   net/tipc/socket.c | 3 ++-
> >   2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> > index 046e4cb3acea..01b64869a173 100644
> > --- a/net/tipc/msg.c
> > +++ b/net/tipc/msg.c
> > @@ -238,14 +238,14 @@ int tipc_msg_append(struct tipc_msg *_hdr, struct msghdr *m, int dlen,
> >   		hdr = buf_msg(skb);
> >   		curr = msg_blocks(hdr);
> >   		mlen = msg_size(hdr);
> > -		cpy = min_t(int, rem, mss - mlen);
> > +		cpy = min_t(size_t, rem, mss - mlen);
> >   		if (cpy != copy_from_iter(skb->data + mlen, cpy, &m->msg_iter))
> >   			return -EFAULT;
> >   		msg_set_size(hdr, mlen + cpy);
> >   		skb_put(skb, cpy);
> >   		rem -= cpy;
> >   		total += msg_blocks(hdr) - curr;
> > -	} while (rem);
> > +	} while (rem > 0);
> >   	return total - accounted;
> >   }
> >
> > diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> > index 26123f4177fd..a94f38333698 100644
> > --- a/net/tipc/socket.c
> > +++ b/net/tipc/socket.c
> > @@ -1574,7 +1574,8 @@ static int __tipc_sendstream(struct socket *sock, struct msghdr *m, size_t dlen)
> >   			break;
> >   		send = min_t(size_t, dlen - sent, TIPC_MAX_USER_MSG_SIZE);
> >   		blocks = tsk->snd_backlog;
> > -		if (tsk->oneway++ >= tsk->nagle_start && send <= maxnagle) {
> > +		if (tsk->oneway++ >= tsk->nagle_start && maxnagle &&
> > +		    send <= maxnagle) {
> >   			rc = tipc_msg_append(hdr, m, send, maxnagle, txq);
> >   			if (unlikely(rc < 0))
> >   				break;
> Acked-by: Jon Maloy <jm...@re...>