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

Re: [tipc-discussion] [PATCH net] tipc: use a write lock for keepalive_intv instead of a read lock

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

On 3/31/22 10:28, Paolo Abeni wrote:
> On Tue, 2022-03-29 at 18:12 +0200, Niels Dossche wrote:
>> Currently, n->keepalive_intv is written to while n is locked by a read
>> lock instead of a write lock. This seems to me to break the atomicity
>> against other readers.
>> Change this to a write lock instead to solve the issue.
>>
>> Note:
>> I am currently working on a static analyser to detect missing locks
>> using type-based static analysis as my master's thesis
>> in order to obtain my master's degree.
>> If you would like to have more details, please let me know.
>> This was a reported case. I manually verified the report by looking
>> at the code, so that I do not send wrong information or patches.
>> After concluding that this seems to be a true positive, I created
>> this patch. I have both compile-tested this patch and runtime-tested
>> this patch on x86_64. The effect on a running system could be a
>> potential race condition in exceptional cases.
>> This issue was found on Linux v5.17.
>>
>> Fixes: f5d6c3e5a359 ("tipc: fix node keep alive interval calculation")
>> Signed-off-by: Niels Dossche <dos...@gm...>
>> ---
>>   net/tipc/node.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/tipc/node.c b/net/tipc/node.c
>> index 6ef95ce565bd..da867ddb93f5 100644
>> --- a/net/tipc/node.c
>> +++ b/net/tipc/node.c
>> @@ -806,9 +806,9 @@ static void tipc_node_timeout(struct timer_list *t)
>>   	/* Initial node interval to value larger (10 seconds), then it will be
>>   	 * recalculated with link lowest tolerance
>>   	 */
>> -	tipc_node_read_lock(n);
>> +	tipc_node_write_lock(n);
> I agree with Hoang, this should be safe even without write lock, as
> tipc_node_timeout() is the only function modifying keepalive_intv, and
> such function is invoked only by a timer, so we are guaranteeded there
> are no possible concurrent updates...
>
>>   	n->keepalive_intv = 10000;
>> -	tipc_node_read_unlock(n);
>> +	tipc_node_write_unlock(n);
>>   	for (bearer_id = 0; remains && (bearer_id < MAX_BEARERS); bearer_id++) {
>>   		tipc_node_read_lock(n);
> ...otherwise we have a similar issue here: a few line below
> keepalive_intv is updated via tipc_node_calculate_timer(), still under
> the read lock
>
> Thanks!
>
> Paolo
>
Hoang's and Paolo's conclusion is correct.
The patch is not needed.
///jon

Re: [tipc-discussion] [PATCH net] tipc: use a write lock for keepalive_intv instead of a read lock

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

Hi Niels,

I did consider this function however I guess it is safe to use  tipc_node_read_lock()/unlock() since this value is being apply in this callback function. 

BTW, you must be using tipc_node_write_unlock_fast() instead of tipc_node_write_unlock().
Regards,
Hoang
> -----Original Message-----
> From: Niels Dossche <dos...@gm...>
> Sent: Tuesday, March 29, 2022 11:12 PM
> To: tip...@li...
> Cc: ne...@vg...; Jon Maloy <jm...@re...>; Ying Xue <yin...@wi...>; David S. Miller
> <da...@da...>; Jakub Kicinski <ku...@ke...>; Paolo Abeni <pa...@re...>; Hoang Huu Le
> <hoa...@de...>; Niels Dossche <dos...@gm...>
> Subject: [PATCH net] tipc: use a write lock for keepalive_intv instead of a read lock
> 
> Currently, n->keepalive_intv is written to while n is locked by a read
> lock instead of a write lock. This seems to me to break the atomicity
> against other readers.
> Change this to a write lock instead to solve the issue.
> 
> Note:
> I am currently working on a static analyser to detect missing locks
> using type-based static analysis as my master's thesis
> in order to obtain my master's degree.
> If you would like to have more details, please let me know.
> This was a reported case. I manually verified the report by looking
> at the code, so that I do not send wrong information or patches.
> After concluding that this seems to be a true positive, I created
> this patch. I have both compile-tested this patch and runtime-tested
> this patch on x86_64. The effect on a running system could be a
> potential race condition in exceptional cases.
> This issue was found on Linux v5.17.
> 
> Fixes: f5d6c3e5a359 ("tipc: fix node keep alive interval calculation")
> Signed-off-by: Niels Dossche <dos...@gm...>
> ---
>  net/tipc/node.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/tipc/node.c b/net/tipc/node.c
> index 6ef95ce565bd..da867ddb93f5 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -806,9 +806,9 @@ static void tipc_node_timeout(struct timer_list *t)
>  	/* Initial node interval to value larger (10 seconds), then it will be
>  	 * recalculated with link lowest tolerance
>  	 */
> -	tipc_node_read_lock(n);
> +	tipc_node_write_lock(n);
>  	n->keepalive_intv = 10000;
> -	tipc_node_read_unlock(n);
> +	tipc_node_write_unlock(n);
>  	for (bearer_id = 0; remains && (bearer_id < MAX_BEARERS); bearer_id++) {
>  		tipc_node_read_lock(n);
>  		le = &n->links[bearer_id];
> --
> 2.35.1

[tipc-discussion] [net] tipc: fix the timer expires after interval 100ms

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

In the timer callback function tipc_sk_timeout(), we're trying to
reschedule another timeout to retransmit a setup request if destination
link is congested. But we use the incorrect timeout value
(msecs_to_jiffies(100)) instead of (jiffies + msecs_to_jiffies(100)),
so that the timer expires immediately, it's irrelevant for original
description.

In this commit we correct the timeout value in sk_reset_timer()

Fixes: 6787927475e5 ("tipc: buffer overflow handling in listener socket")
Acked-by: Ying Xue <yin...@wi...>
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/socket.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 7545321c3440..17f8c523e33b 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2852,7 +2852,8 @@ static void tipc_sk_retry_connect(struct sock *sk, struct sk_buff_head *list)
 
 	/* Try again later if dest link is congested */
 	if (tsk->cong_link_cnt) {
-		sk_reset_timer(sk, &sk->sk_timer, msecs_to_jiffies(100));
+		sk_reset_timer(sk, &sk->sk_timer,
+			       jiffies + msecs_to_jiffies(100));
 		return;
 	}
 	/* Prepare SYN for retransmit */
-- 
2.30.2

Re: [tipc-discussion] [net] tipc: fix the timer expires after interval 100ms

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

On 3/18/2022 11:58 AM, Hoang Le wrote:
> In the timer callback function tipc_sk_timeout(), we're trying to
> reschedule another timeout to retransmit a setup request if destination
> link is congested. But we use the incorrect timeout value
> (msecs_to_jiffies(100)) instead of (jiffies + msecs_to_jiffies(100)),
> so that the timer expires immediately, it's irrelevant for original
> description.
> 
> In this commit we correct the timeout value in sk_reset_timer()
> 
> Fixes: 6787927475e5 ("tipc: buffer overflow handling in listener socket")
> Signed-off-by: Hoang Le <hoa...@de...>

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

> ---
>  net/tipc/socket.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 7545321c3440..17f8c523e33b 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -2852,7 +2852,8 @@ static void tipc_sk_retry_connect(struct sock *sk, struct sk_buff_head *list)
>  
>  	/* Try again later if dest link is congested */
>  	if (tsk->cong_link_cnt) {
> -		sk_reset_timer(sk, &sk->sk_timer, msecs_to_jiffies(100));
> +		sk_reset_timer(sk, &sk->sk_timer,
> +			       jiffies + msecs_to_jiffies(100));
>  		return;
>  	}
>  	/* Prepare SYN for retransmit */

[tipc-discussion] [net] tipc: fix the timer expires after interval 100ms

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

In the timer callback function tipc_sk_timeout(), we're trying to
reschedule another timeout to retransmit a setup request if destination
link is congested. But we use the incorrect timeout value
(msecs_to_jiffies(100)) instead of (jiffies + msecs_to_jiffies(100)),
so that the timer expires immediately, it's irrelevant for original
description.

In this commit we correct the timeout value in sk_reset_timer()

Fixes: 6787927475e5 ("tipc: buffer overflow handling in listener socket")
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/socket.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 7545321c3440..17f8c523e33b 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2852,7 +2852,8 @@ static void tipc_sk_retry_connect(struct sock *sk, struct sk_buff_head *list)
 
 	/* Try again later if dest link is congested */
 	if (tsk->cong_link_cnt) {
-		sk_reset_timer(sk, &sk->sk_timer, msecs_to_jiffies(100));
+		sk_reset_timer(sk, &sk->sk_timer,
+			       jiffies + msecs_to_jiffies(100));
 		return;
 	}
 	/* Prepare SYN for retransmit */
-- 
2.30.2

[tipc-discussion] [PATCH net 1/1] tipc: fix incorrect order of state message data sanity check

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

When receiving a state message, function tipc_link_validate_msg()
is called to validate its header portion. Then, its data portion
is validated before it can be accessed correctly. However, current
data sanity  check is done after the message header is accessed to
update some link variables.

This commit fixes this issue by moving the data sanity check to
the beginning of state message handling and right after the header
sanity check.

Fixes: 9aa422ad3266 ("tipc: improve size validations for received domain records")
Acked-by: Jon Maloy <jm...@re...>
Signed-off-by: Tung Nguyen <tun...@de...>
---
 net/tipc/link.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 1e14d7f8f28f..e260c0d557f5 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -2286,6 +2286,11 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
 		break;
 
 	case STATE_MSG:
+		/* Validate Gap ACK blocks, drop if invalid */
+		glen = tipc_get_gap_ack_blks(&ga, l, hdr, true);
+		if (glen > dlen)
+			break;
+
 		l->rcv_nxt_state = msg_seqno(hdr) + 1;
 
 		/* Update own tolerance if peer indicates a non-zero value */
@@ -2311,10 +2316,6 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
 			break;
 		}
 
-		/* Receive Gap ACK blocks from peer if any */
-		glen = tipc_get_gap_ack_blks(&ga, l, hdr, true);
-		if(glen > dlen)
-			break;
 		tipc_mon_rcv(l->net, data + glen, dlen - glen, l->addr,
 			     &l->mon_state, l->bearer_id);
 
-- 
2.25.1

[tipc-discussion] [PATCH net-next] tipc: add lxc_xmit switch for the transmission between netns

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

We need this switch, as lxc_xmit works around the MEDIA to send
the data to another netns directly. It causes all netfilter and
tc rules to not apply to the traffic, which might not be
expected in some cases. Also, it cause that we can't do testing
over UDP/ETH MEDIA properly.

So add a systcl switch for lxc_xmit, so that users can use TIPC
between netns as before by setting it to 0. Note that it only
affects the new created nodes.

Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/node.c   | 4 +++-
 net/tipc/node.h   | 2 ++
 net/tipc/sysctl.c | 9 +++++++++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index 6ef95ce565bd..c7ffb2cdefc3 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -49,6 +49,8 @@
 #define INVALID_NODE_SIG	0x10000
 #define NODE_CLEANUP_AFTER	300000
 
+int sysctl_tipc_lxc_xmit __read_mostly = 1;
+
 /* Flags used to take different actions according to flag type
  * TIPC_NOTIFY_NODE_DOWN: notify node is down
  * TIPC_NOTIFY_NODE_UP: notify node is up
@@ -446,7 +448,7 @@ static void tipc_node_assign_peer_net(struct tipc_node *n, u32 hash_mixes)
 	struct net *tmp;
 	u32 hash_chk;
 
-	if (n->peer_net)
+	if (!sysctl_tipc_lxc_xmit || n->peer_net)
 		return;
 
 	for_each_net_rcu(tmp) {
diff --git a/net/tipc/node.h b/net/tipc/node.h
index 154a5bbb0d29..db57335c9d6c 100644
--- a/net/tipc/node.h
+++ b/net/tipc/node.h
@@ -42,6 +42,8 @@
 #include "bearer.h"
 #include "msg.h"
 
+extern int sysctl_tipc_lxc_xmit __read_mostly;
+
 /* Optional capabilities supported by this code version
  */
 enum {
diff --git a/net/tipc/sysctl.c b/net/tipc/sysctl.c
index 9fb65c988f7f..ca27c1922e71 100644
--- a/net/tipc/sysctl.c
+++ b/net/tipc/sysctl.c
@@ -91,6 +91,15 @@ static struct ctl_table tipc_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_doulongvec_minmax,
 	},
+	{
+		.procname	= "lxc_xmit",
+		.data		= &sysctl_tipc_lxc_xmit,
+		.maxlen		= sizeof(sysctl_tipc_lxc_xmit),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1         = SYSCTL_ZERO,
+		.extra2         = SYSCTL_ONE,
+	},
 	{}
 };
 
-- 
2.31.1

[tipc-discussion] [PATCH net v2 1/1] tipc: fix kernel panic when enabling bearer

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

When enabling a bearer on a node, a kernel panic is observed:

[    4.498085] RIP: 0010:tipc_mon_prep+0x4e/0x130 [tipc]
...
[    4.520030] Call Trace:
[    4.520689]  <IRQ>
[    4.521236]  tipc_link_build_proto_msg+0x375/0x750 [tipc]
[    4.522654]  tipc_link_build_state_msg+0x48/0xc0 [tipc]
[    4.524034]  __tipc_node_link_up+0xd7/0x290 [tipc]
[    4.525292]  tipc_rcv+0x5da/0x730 [tipc]
[    4.526346]  ? __netif_receive_skb_core+0xb7/0xfc0
[    4.527601]  tipc_l2_rcv_msg+0x5e/0x90 [tipc]
[    4.528737]  __netif_receive_skb_list_core+0x20b/0x260
[    4.530068]  netif_receive_skb_list_internal+0x1bf/0x2e0
[    4.531450]  ? dev_gro_receive+0x4c2/0x680
[    4.532512]  napi_complete_done+0x6f/0x180
[    4.533570]  virtnet_poll+0x29c/0x42e [virtio_net]
...

The node in question is receiving activate messages in another
thread after changing bearer status to allow message sending/
receiving in current thread:

         thread 1           |              thread 2
         --------           |              --------
                            |
tipc_enable_bearer()        |
  test_and_set_bit_lock()   |
    tipc_bearer_xmit_skb()  |
                            | tipc_l2_rcv_msg()
                            |   tipc_rcv()
                            |     __tipc_node_link_up()
                            |       tipc_link_build_state_msg()
                            |         tipc_link_build_proto_msg()
                            |           tipc_mon_prep()
                            |           {
                            |             ...
                            |             // null-pointer dereference
                            |             u16 gen = mon->dom_gen;
                            |             ...
                            |           }
  // Not being executed yet |
  tipc_mon_create()         |
  {                         |
    ...                     |
    // allocate             |
    mon = kzalloc();        |
    ...                     |
  }                         |

Monitoring pointer in thread 2 is dereferenced before monitoring data
is allocated in thread 1. This causes kernel panic.

This commit fixes it by allocating the monitoring data before enabling
the bearer to receive messages.

Fixes: 35c55c9877f8 ("tipc: add neighbor monitoring framework")
Reported-by: Shuang Li <sh...@re...>
Acked-by: Jon Maloy <jm...@re...>
Signed-off-by: Tung Nguyen <tun...@de...>
---
 net/tipc/bearer.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 473a790f5894..a2f9c9640716 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -352,16 +352,18 @@ 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);
-
+	/* Create monitoring data before accepting activate messages */
 	if (tipc_mon_create(net, bearer_id)) {
 		bearer_disable(net, b);
+		kfree_skb(skb);
 		return -ENOMEM;
 	}
 
+	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);
+
 	pr_info("Enabled bearer <%s>, priority %u\n", name, prio);
 
 	return res;
-- 
2.25.1

Re: [tipc-discussion] [PATCH net 1/1] tipc: fix kernel panic when enabling bearer

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

On Thu,  3 Mar 2022 04:57:17 +0000 Tung Nguyen wrote:
> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> index 473a790f5894..63460183440d 100644
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c
> @@ -252,7 +252,7 @@ static int tipc_enable_bearer(struct net *net, const char *name,
>  	int with_this_prio = 1;
>  	struct tipc_bearer *b;
>  	struct tipc_media *m;
> -	struct sk_buff *skb;
> +	struct sk_buff *skb = NULL;
>  	int bearer_id = 0;
>  	int res = -EINVAL;
>  	char *errstr = "";

This chunk looks unrelated and unnecessary. The had previously trusted
skb to be initialized by tipc_disc_create().
[Tung]: OK. I will remove it in v2.

[tipc-discussion] [PATCH net 1/1] tipc: fix kernel panic when enabling bearer

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

When enabling a bearer on a node, a kernel panic is observed:

[    4.498085] RIP: 0010:tipc_mon_prep+0x4e/0x130 [tipc]
...
[    4.520030] Call Trace:
[    4.520689]  <IRQ>
[    4.521236]  tipc_link_build_proto_msg+0x375/0x750 [tipc]
[    4.522654]  tipc_link_build_state_msg+0x48/0xc0 [tipc]
[    4.524034]  __tipc_node_link_up+0xd7/0x290 [tipc]
[    4.525292]  tipc_rcv+0x5da/0x730 [tipc]
[    4.526346]  ? __netif_receive_skb_core+0xb7/0xfc0
[    4.527601]  tipc_l2_rcv_msg+0x5e/0x90 [tipc]
[    4.528737]  __netif_receive_skb_list_core+0x20b/0x260
[    4.530068]  netif_receive_skb_list_internal+0x1bf/0x2e0
[    4.531450]  ? dev_gro_receive+0x4c2/0x680
[    4.532512]  napi_complete_done+0x6f/0x180
[    4.533570]  virtnet_poll+0x29c/0x42e [virtio_net]
...

The node in question is receiving activate messages in another
thread after changing bearer status to allow message sending/
receiving in current thread:

         thread 1           |              thread 2
         --------           |              --------
                            |
tipc_enable_bearer()        |
  test_and_set_bit_lock()   |
    tipc_bearer_xmit_skb()  |
                            | tipc_l2_rcv_msg()
                            |   tipc_rcv()
                            |     __tipc_node_link_up()
                            |       tipc_link_build_state_msg()
                            |         tipc_link_build_proto_msg()
                            |           tipc_mon_prep()
                            |           {
                            |             ...
                            |             // null-pointer dereference
                            |             u16 gen = mon->dom_gen;
                            |             ...
                            |           }
  // Not being executed yet |
  tipc_mon_create()         |
  {                         |
    ...                     |
    // allocate             |
    mon = kzalloc();        |
    ...                     |
  }                         |

Monitoring pointer in thread 2 is dereferenced before monitoring data
is allocated in thread 1. This causes kernel panic.

This commit fixes it by allocating the monitoring data before enabling
the bearer to receive messages.

Fixes: 35c55c9877f8 ("tipc: add neighbor monitoring framework")
Reported-by: Shuang Li <sh...@re...>
Acked-by: Jon Maloy <jm...@re...>
Signed-off-by: Tung Nguyen <tun...@de...>
---
 net/tipc/bearer.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 473a790f5894..63460183440d 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -252,7 +252,7 @@ static int tipc_enable_bearer(struct net *net, const char *name,
 	int with_this_prio = 1;
 	struct tipc_bearer *b;
 	struct tipc_media *m;
-	struct sk_buff *skb;
+	struct sk_buff *skb = NULL;
 	int bearer_id = 0;
 	int res = -EINVAL;
 	char *errstr = "";
@@ -352,16 +352,18 @@ 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);
-
+	/* Create monitoring data before accepting activate messages */
 	if (tipc_mon_create(net, bearer_id)) {
 		bearer_disable(net, b);
+		kfree_skb(skb);
 		return -ENOMEM;
 	}
 
+	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);
+
 	pr_info("Enabled bearer <%s>, priority %u\n", name, prio);
 
 	return res;
-- 
2.25.1

Re: [tipc-discussion] [PATCH net] tipc: Fix end of loop tests for list_for_each_entry()

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

On 2/22/22 08:43, Dan Carpenter wrote:
> These tests are supposed to check if the loop exited via a break or not.
> However the tests are wrong because if we did not exit via a break then
> "p" is not a valid pointer.  In that case, it's the equivalent of
> "if (*(u32 *)sr == *last_key) {".  That's going to work most of the time,
> but there is a potential for those to be equal.
>
> Fixes: 1593123a6a49 ("tipc: add name table dump to new netlink api")
> Fixes: 1a1a143daf84 ("tipc: add publication dump to new netlink api")
> Signed-off-by: Dan Carpenter <dan...@or...>
> ---
>   net/tipc/name_table.c | 2 +-
>   net/tipc/socket.c     | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
> index 01396dd1c899..1d8ba233d047 100644
> --- a/net/tipc/name_table.c
> +++ b/net/tipc/name_table.c
> @@ -967,7 +967,7 @@ static int __tipc_nl_add_nametable_publ(struct tipc_nl_msg *msg,
>   		list_for_each_entry(p, &sr->all_publ, all_publ)
>   			if (p->key == *last_key)
>   				break;
> -		if (p->key != *last_key)
> +		if (list_entry_is_head(p, &sr->all_publ, all_publ))
>   			return -EPIPE;
>   	} else {
>   		p = list_first_entry(&sr->all_publ,
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 3e63c83e641c..7545321c3440 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -3749,7 +3749,7 @@ static int __tipc_nl_list_sk_publ(struct sk_buff *skb,
>   			if (p->key == *last_publ)
>   				break;
>   		}
> -		if (p->key != *last_publ) {
> +		if (list_entry_is_head(p, &tsk->publications, binding_sock)) {
>   			/* We never set seq or call nl_dump_check_consistent()
>   			 * this means that setting prev_seq here will cause the
>   			 * consistence check to fail in the netlink callback
Acked-by: Jon Maloy <jm...@re...>

Re: [tipc-discussion] EXTERNAL: Re: TIPC Socket Liveness Check

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

On 1/11/22 18:01, Duzan, Gary D via tipc-discussion wrote:
>     Is there a reliable way for a process to determine if a TIPC socket address points to an open socket without disturbing the target process? I'm hoping to be able to determine the liveness/reachability of a datagram peer, at least roughly, without taking on the complexity of additional messaging (or group membership and join/leave tracking).
>
>     Thanks.
It depends on the socket type and state. If it is a connected socket
there is a built-in mechanism that will make sure that the peer socket
is immediately notified, and the user will receive this notification.
The same is the case with group sockets.
For DGRAM sockets there is no such mechanism, since it is impossible for
TIPC to know which peers need to be supervised.

///jon

   Thanks, Jon. I'm dipping my toes in the group communication waters, and so far it looks like I will be able to use it.

Gary Duzan
FIS GT.M Core Team

________________________________
From: Jon Maloy <jm...@re...>
Sent: Wednesday, January 12, 2022 11:37 AM
To: tip...@li... <tip...@li...>
Subject: EXTERNAL: Re: [tipc-discussion] TIPC Socket Liveness Check

CAUTION: This email originated from outside of the company. Do not click links or open attachments unless you recognize the sender and know the content is safe.



On 1/11/22 18:01, Duzan, Gary D via tipc-discussion wrote:
>     Is there a reliable way for a process to determine if a TIPC socket address points to an open socket without disturbing the target process? I'm hoping to be able to determine the liveness/reachability of a datagram peer, at least roughly, without taking on the complexity of additional messaging (or group membership and join/leave tracking).
>
>     Thanks.
It depends on the socket type and state. If it is a connected socket
there is a built-in mechanism that will make sure that the peer socket
is immediately notified, and the user will receive this notification.
The same is the case with group sockets.
For DGRAM sockets there is no such mechanism, since it is impossible for
TIPC to know which peers need to be supervised.

///jon

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



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

Re: [tipc-discussion] TIPC Socket Liveness Check

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

On 1/11/22 18:01, Duzan, Gary D via tipc-discussion wrote:
>     Is there a reliable way for a process to determine if a TIPC socket address points to an open socket without disturbing the target process? I'm hoping to be able to determine the liveness/reachability of a datagram peer, at least roughly, without taking on the complexity of additional messaging (or group membership and join/leave tracking).
>
>     Thanks.
It depends on the socket type and state. If it is a connected socket 
there is a built-in mechanism that will make sure that the peer socket 
is immediately notified, and the user will receive this notification. 
The same is the case with group sockets.
For DGRAM sockets there is no such mechanism, since it is impossible for 
TIPC to know which peers need to be supervised.

///jon

>
> Gary Duzan
> FIS GT.M Core Team
>
> The information contained in this message is proprietary and/or confidential. If you are not the intended recipient, please: (i) delete the message and all copies; (ii) do not disclose, distribute or use the message in any manner; and (iii) notify the sender immediately. In addition, please be aware that any message addressed to our domain is subject to archiving and review by persons other than the intended recipient. Thank you.
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>

[tipc-discussion] TIPC Socket Liveness Check

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

   Is there a reliable way for a process to determine if a TIPC socket address points to an open socket without disturbing the target process? I'm hoping to be able to determine the liveness/reachability of a datagram peer, at least roughly, without taking on the complexity of additional messaging (or group membership and join/leave tracking).

   Thanks.

Gary Duzan
FIS GT.M Core Team

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

[tipc-discussion] [net-next] Revert "tipc: use consistent GFP flags"

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

This reverts commit 86c3a3e964d910a62eeb277d60b2a60ebefa9feb.

The tipc_aead_init() function can be calling from an interrupt routine.
This allocation might sleep with GFP_KERNEL flag, hence the following BUG
is reported.

[   17.657509] BUG: sleeping function called from invalid context at include/linux/sched/mm.h:230
[   17.660916] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0, name: swapper/3
[   17.664093] preempt_count: 302, expected: 0
[   17.665619] RCU nest depth: 2, expected: 0
[   17.667163] Preemption disabled at:
[   17.667165] [<0000000000000000>] 0x0
[   17.669753] CPU: 3 PID: 0 Comm: swapper/3 Kdump: loaded Tainted: G        W         5.16.0-rc4+ #1
[   17.673006] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
[   17.675540] Call Trace:
[   17.676285]  <IRQ>
[   17.676913]  dump_stack_lvl+0x34/0x44
[   17.678033]  __might_resched.cold+0xd6/0x10f
[   17.679311]  kmem_cache_alloc_trace+0x14d/0x220
[   17.680663]  tipc_crypto_start+0x4a/0x2b0 [tipc]
[   17.682146]  ? kmem_cache_alloc_trace+0xd3/0x220
[   17.683545]  tipc_node_create+0x2f0/0x790 [tipc]
[   17.684956]  tipc_node_check_dest+0x72/0x680 [tipc]
[   17.686706]  ? ___cache_free+0x31/0x350
[   17.688008]  ? skb_release_data+0x128/0x140
[   17.689431]  tipc_disc_rcv+0x479/0x510 [tipc]
[   17.690904]  tipc_rcv+0x71c/0x730 [tipc]
[   17.692219]  ? __netif_receive_skb_core+0xb7/0xf60
[   17.693856]  tipc_l2_rcv_msg+0x5e/0x90 [tipc]
[   17.695333]  __netif_receive_skb_list_core+0x20b/0x260
[   17.697072]  netif_receive_skb_list_internal+0x1bf/0x2e0
[   17.698870]  ? dev_gro_receive+0x4c2/0x680
[   17.700255]  napi_complete_done+0x6f/0x180
[   17.701657]  virtnet_poll+0x29c/0x42e [virtio_net]
[   17.703262]  __napi_poll+0x2c/0x170
[   17.704429]  net_rx_action+0x22f/0x280
[   17.705706]  __do_softirq+0xfd/0x30a
[   17.706921]  common_interrupt+0xa4/0xc0
[   17.708206]  </IRQ>
[   17.708922]  <TASK>
[   17.709651]  asm_common_interrupt+0x1e/0x40
[   17.711078] RIP: 0010:default_idle+0x18/0x20

Fixes: 86c3a3e964d9 ("tipc: use consistent GFP flags")
Acked-by: Jon Maloy <jm...@re...>
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/crypto.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index 81116312b753..9325479295b8 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -524,7 +524,7 @@ static int tipc_aead_init(struct tipc_aead **aead, struct tipc_aead_key *ukey,
 		return -EEXIST;
 
 	/* Allocate a new AEAD */
-	tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
+	tmp = kzalloc(sizeof(*tmp), GFP_ATOMIC);
 	if (unlikely(!tmp))
 		return -ENOMEM;
 
@@ -1463,7 +1463,7 @@ int tipc_crypto_start(struct tipc_crypto **crypto, struct net *net,
 		return -EEXIST;
 
 	/* Allocate crypto */
-	c = kzalloc(sizeof(*c), GFP_KERNEL);
+	c = kzalloc(sizeof(*c), GFP_ATOMIC);
 	if (!c)
 		return -ENOMEM;
 
@@ -1477,7 +1477,7 @@ int tipc_crypto_start(struct tipc_crypto **crypto, struct net *net,
 	}
 
 	/* Allocate statistic structure */
-	c->stats = alloc_percpu(struct tipc_crypto_stats);
+	c->stats = alloc_percpu_gfp(struct tipc_crypto_stats, GFP_ATOMIC);
 	if (!c->stats) {
 		if (c->wq)
 			destroy_workqueue(c->wq);
@@ -2450,7 +2450,7 @@ static void tipc_crypto_work_tx(struct work_struct *work)
 	}
 
 	/* Lets duplicate it first */
-	skey = kmemdup(aead->key, tipc_aead_key_size(aead->key), GFP_KERNEL);
+	skey = kmemdup(aead->key, tipc_aead_key_size(aead->key), GFP_ATOMIC);
 	rcu_read_unlock();
 
 	/* Now, generate new key, initiate & distribute it */
-- 
2.30.2

Re: [tipc-discussion] [net-next] Revert "tipc: use consistent GFP flags"

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

On 12/10/21 01:59, Hoang Le wrote:
> This reverts commit 86c3a3e964d910a62eeb277d60b2a60ebefa9feb.
>
> The tipc_aead_init() function can be calling from an interrupt routine.
> This allocation might sleep with GFP_KERNEL flag, hence the following BUG
> is reported.
>
> [   17.657509] BUG: sleeping function called from invalid context at include/linux/sched/mm.h:230
> [   17.660916] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0, name: swapper/3
> [   17.664093] preempt_count: 302, expected: 0
> [   17.665619] RCU nest depth: 2, expected: 0
> [   17.667163] Preemption disabled at:
> [   17.667165] [<0000000000000000>] 0x0
> [   17.669753] CPU: 3 PID: 0 Comm: swapper/3 Kdump: loaded Tainted: G        W         5.16.0-rc4+ #1
> [   17.673006] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
> [   17.675540] Call Trace:
> [   17.676285]  <IRQ>
> [   17.676913]  dump_stack_lvl+0x34/0x44
> [   17.678033]  __might_resched.cold+0xd6/0x10f
> [   17.679311]  kmem_cache_alloc_trace+0x14d/0x220
> [   17.680663]  tipc_crypto_start+0x4a/0x2b0 [tipc]
> [   17.682146]  ? kmem_cache_alloc_trace+0xd3/0x220
> [   17.683545]  tipc_node_create+0x2f0/0x790 [tipc]
> [   17.684956]  tipc_node_check_dest+0x72/0x680 [tipc]
> [   17.686706]  ? ___cache_free+0x31/0x350
> [   17.688008]  ? skb_release_data+0x128/0x140
> [   17.689431]  tipc_disc_rcv+0x479/0x510 [tipc]
> [   17.690904]  tipc_rcv+0x71c/0x730 [tipc]
> [   17.692219]  ? __netif_receive_skb_core+0xb7/0xf60
> [   17.693856]  tipc_l2_rcv_msg+0x5e/0x90 [tipc]
> [   17.695333]  __netif_receive_skb_list_core+0x20b/0x260
> [   17.697072]  netif_receive_skb_list_internal+0x1bf/0x2e0
> [   17.698870]  ? dev_gro_receive+0x4c2/0x680
> [   17.700255]  napi_complete_done+0x6f/0x180
> [   17.701657]  virtnet_poll+0x29c/0x42e [virtio_net]
> [   17.703262]  __napi_poll+0x2c/0x170
> [   17.704429]  net_rx_action+0x22f/0x280
> [   17.705706]  __do_softirq+0xfd/0x30a
> [   17.706921]  common_interrupt+0xa4/0xc0
> [   17.708206]  </IRQ>
> [   17.708922]  <TASK>
> [   17.709651]  asm_common_interrupt+0x1e/0x40
> [   17.711078] RIP: 0010:default_idle+0x18/0x20
>
> Signed-off-by: Hoang Le <hoa...@de...>
> ---
>   net/tipc/crypto.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
> index 81116312b753..9325479295b8 100644
> --- a/net/tipc/crypto.c
> +++ b/net/tipc/crypto.c
> @@ -524,7 +524,7 @@ static int tipc_aead_init(struct tipc_aead **aead, struct tipc_aead_key *ukey,
>   		return -EEXIST;
>   
>   	/* Allocate a new AEAD */
> -	tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
> +	tmp = kzalloc(sizeof(*tmp), GFP_ATOMIC);
>   	if (unlikely(!tmp))
>   		return -ENOMEM;
>   
> @@ -1463,7 +1463,7 @@ int tipc_crypto_start(struct tipc_crypto **crypto, struct net *net,
>   		return -EEXIST;
>   
>   	/* Allocate crypto */
> -	c = kzalloc(sizeof(*c), GFP_KERNEL);
> +	c = kzalloc(sizeof(*c), GFP_ATOMIC);
>   	if (!c)
>   		return -ENOMEM;
>   
> @@ -1477,7 +1477,7 @@ int tipc_crypto_start(struct tipc_crypto **crypto, struct net *net,
>   	}
>   
>   	/* Allocate statistic structure */
> -	c->stats = alloc_percpu(struct tipc_crypto_stats);
> +	c->stats = alloc_percpu_gfp(struct tipc_crypto_stats, GFP_ATOMIC);
>   	if (!c->stats) {
>   		if (c->wq)
>   			destroy_workqueue(c->wq);
> @@ -2450,7 +2450,7 @@ static void tipc_crypto_work_tx(struct work_struct *work)
>   	}
>   
>   	/* Lets duplicate it first */
> -	skey = kmemdup(aead->key, tipc_aead_key_size(aead->key), GFP_KERNEL);
> +	skey = kmemdup(aead->key, tipc_aead_key_size(aead->key), GFP_ATOMIC);
>   	rcu_read_unlock();
>   
>   	/* Now, generate new key, initiate & distribute it */
Acked-by: Jon Maloy <jm...@re...>

[tipc-discussion] [PATCH net-next] tipc: discard MSG_CRYPTO msgs when key_exchange_enabled is not set

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

When key_exchange is disabled, there is no reason to accept MSG_CRYPTO
msgs if it doesn't send MSG_CRYPTO msgs.

Signed-off-by: Xin Long <luc...@gm...>
Acked-by: Jon Maloy <jm...@re...>
---
 net/tipc/link.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 09ae8448f394..8d9e09f48f4c 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1298,7 +1298,8 @@ static bool tipc_data_input(struct tipc_link *l, struct sk_buff *skb,
 		return false;
 #ifdef CONFIG_TIPC_CRYPTO
 	case MSG_CRYPTO:
-		if (TIPC_SKB_CB(skb)->decrypted) {
+		if (sysctl_tipc_key_exchange_enabled &&
+		    TIPC_SKB_CB(skb)->decrypted) {
 			tipc_crypto_msg_rcv(l->net, skb);
 			return true;
 		}
-- 
2.27.0

[tipc-discussion] [net-next] Revert "tipc: use consistent GFP flags"

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

This reverts commit 86c3a3e964d910a62eeb277d60b2a60ebefa9feb.

The tipc_aead_init() function can be calling from an interrupt routine.
This allocation might sleep with GFP_KERNEL flag, hence the following BUG
is reported.

[   17.657509] BUG: sleeping function called from invalid context at include/linux/sched/mm.h:230
[   17.660916] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0, name: swapper/3
[   17.664093] preempt_count: 302, expected: 0
[   17.665619] RCU nest depth: 2, expected: 0
[   17.667163] Preemption disabled at:
[   17.667165] [<0000000000000000>] 0x0
[   17.669753] CPU: 3 PID: 0 Comm: swapper/3 Kdump: loaded Tainted: G        W         5.16.0-rc4+ #1
[   17.673006] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
[   17.675540] Call Trace:
[   17.676285]  <IRQ>
[   17.676913]  dump_stack_lvl+0x34/0x44
[   17.678033]  __might_resched.cold+0xd6/0x10f
[   17.679311]  kmem_cache_alloc_trace+0x14d/0x220
[   17.680663]  tipc_crypto_start+0x4a/0x2b0 [tipc]
[   17.682146]  ? kmem_cache_alloc_trace+0xd3/0x220
[   17.683545]  tipc_node_create+0x2f0/0x790 [tipc]
[   17.684956]  tipc_node_check_dest+0x72/0x680 [tipc]
[   17.686706]  ? ___cache_free+0x31/0x350
[   17.688008]  ? skb_release_data+0x128/0x140
[   17.689431]  tipc_disc_rcv+0x479/0x510 [tipc]
[   17.690904]  tipc_rcv+0x71c/0x730 [tipc]
[   17.692219]  ? __netif_receive_skb_core+0xb7/0xf60
[   17.693856]  tipc_l2_rcv_msg+0x5e/0x90 [tipc]
[   17.695333]  __netif_receive_skb_list_core+0x20b/0x260
[   17.697072]  netif_receive_skb_list_internal+0x1bf/0x2e0
[   17.698870]  ? dev_gro_receive+0x4c2/0x680
[   17.700255]  napi_complete_done+0x6f/0x180
[   17.701657]  virtnet_poll+0x29c/0x42e [virtio_net]
[   17.703262]  __napi_poll+0x2c/0x170
[   17.704429]  net_rx_action+0x22f/0x280
[   17.705706]  __do_softirq+0xfd/0x30a
[   17.706921]  common_interrupt+0xa4/0xc0
[   17.708206]  </IRQ>
[   17.708922]  <TASK>
[   17.709651]  asm_common_interrupt+0x1e/0x40
[   17.711078] RIP: 0010:default_idle+0x18/0x20

Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/crypto.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index 81116312b753..9325479295b8 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -524,7 +524,7 @@ static int tipc_aead_init(struct tipc_aead **aead, struct tipc_aead_key *ukey,
 		return -EEXIST;
 
 	/* Allocate a new AEAD */
-	tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
+	tmp = kzalloc(sizeof(*tmp), GFP_ATOMIC);
 	if (unlikely(!tmp))
 		return -ENOMEM;
 
@@ -1463,7 +1463,7 @@ int tipc_crypto_start(struct tipc_crypto **crypto, struct net *net,
 		return -EEXIST;
 
 	/* Allocate crypto */
-	c = kzalloc(sizeof(*c), GFP_KERNEL);
+	c = kzalloc(sizeof(*c), GFP_ATOMIC);
 	if (!c)
 		return -ENOMEM;
 
@@ -1477,7 +1477,7 @@ int tipc_crypto_start(struct tipc_crypto **crypto, struct net *net,
 	}
 
 	/* Allocate statistic structure */
-	c->stats = alloc_percpu(struct tipc_crypto_stats);
+	c->stats = alloc_percpu_gfp(struct tipc_crypto_stats, GFP_ATOMIC);
 	if (!c->stats) {
 		if (c->wq)
 			destroy_workqueue(c->wq);
@@ -2450,7 +2450,7 @@ static void tipc_crypto_work_tx(struct work_struct *work)
 	}
 
 	/* Lets duplicate it first */
-	skey = kmemdup(aead->key, tipc_aead_key_size(aead->key), GFP_KERNEL);
+	skey = kmemdup(aead->key, tipc_aead_key_size(aead->key), GFP_ATOMIC);
 	rcu_read_unlock();
 
 	/* Now, generate new key, initiate & distribute it */
-- 
2.30.2

Re: [tipc-discussion] [PATCH net-next] tipc: discard MSG_CRYPTO msgs when key_exchange_enabled is not set

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

On 11/24/21 15:28, Xin Long wrote:
> When key_exchange is disabled, there is no reason to accept MSG_CRYPTO
> msgs if it doesn't send MSG_CRYPTO msgs.
>
> Signed-off-by: Xin Long <luc...@gm...>
> ---
>   net/tipc/link.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/tipc/link.c b/net/tipc/link.c
> index 09ae8448f394..8d9e09f48f4c 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -1298,7 +1298,8 @@ static bool tipc_data_input(struct tipc_link *l, struct sk_buff *skb,
>   		return false;
>   #ifdef CONFIG_TIPC_CRYPTO
>   	case MSG_CRYPTO:
> -		if (TIPC_SKB_CB(skb)->decrypted) {
> +		if (sysctl_tipc_key_exchange_enabled &&
> +		    TIPC_SKB_CB(skb)->decrypted) {
>   			tipc_crypto_msg_rcv(l->net, skb);
>   			return true;
>   		}
Acked-by: Jon Maloy <jm...@re...>

[tipc-discussion] [PATCH net-next] tipc: discard MSG_CRYPTO msgs when key_exchange_enabled is not set

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

When key_exchange is disabled, there is no reason to accept MSG_CRYPTO
msgs if it doesn't send MSG_CRYPTO msgs.

Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/link.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 09ae8448f394..8d9e09f48f4c 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1298,7 +1298,8 @@ static bool tipc_data_input(struct tipc_link *l, struct sk_buff *skb,
 		return false;
 #ifdef CONFIG_TIPC_CRYPTO
 	case MSG_CRYPTO:
-		if (TIPC_SKB_CB(skb)->decrypted) {
+		if (sysctl_tipc_key_exchange_enabled &&
+		    TIPC_SKB_CB(skb)->decrypted) {
 			tipc_crypto_msg_rcv(l->net, skb);
 			return true;
 		}
-- 
2.27.0

[tipc-discussion] [PATCH net-next] tipc: delete the unlikely branch in tipc_aead_encrypt

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

When a skb comes to tipc_aead_encrypt(), it's always linear. The
unlikely check 'skb_cloned(skb) && tailen <= skb_tailroom(skb)'
can completely be taken care of in skb_cow_data() by the code
in branch "if (!skb_has_frag_list())".

Also, remove the 'TODO:' annotation, as the pages in skbs are not
writable, see more on commit 3cf4375a0904 ("tipc: do not write
skb_shinfo frags when doing decrytion").

Signed-off-by: Xin Long <luc...@gm...>
Acked-by: Jon Maloy <jm...@re...>
---
 net/tipc/crypto.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index b4d9419a015b..81116312b753 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -761,21 +761,10 @@ static int tipc_aead_encrypt(struct tipc_aead *aead, struct sk_buff *skb,
 			 skb_tailroom(skb), tailen);
 	}
 
-	if (unlikely(!skb_cloned(skb) && tailen <= skb_tailroom(skb))) {
-		nsg = 1;
-		trailer = skb;
-	} else {
-		/* TODO: We could avoid skb_cow_data() if skb has no frag_list
-		 * e.g. by skb_fill_page_desc() to add another page to the skb
-		 * with the wanted tailen... However, page skbs look not often,
-		 * so take it easy now!
-		 * Cloned skbs e.g. from link_xmit() seems no choice though :(
-		 */
-		nsg = skb_cow_data(skb, tailen, &trailer);
-		if (unlikely(nsg < 0)) {
-			pr_err("TX: skb_cow_data() returned %d\n", nsg);
-			return nsg;
-		}
+	nsg = skb_cow_data(skb, tailen, &trailer);
+	if (unlikely(nsg < 0)) {
+		pr_err("TX: skb_cow_data() returned %d\n", nsg);
+		return nsg;
 	}
 
 	pskb_put(skb, trailer, tailen);
-- 
2.27.0

Re: [tipc-discussion] [PATCH net-next] tipc: delete the unlikely branch in tipc_aead_encrypt

[Jon M. <ma...@do...>]

 

    On Sunday, November 21, 2021, 12:32:19 PM EST, Xin Long <luc...@gm...> wrote:  
 
 When a skb comes to tipc_aead_encrypt(), it's always linear. The
unlikely check 'skb_cloned(skb) && tailen <= skb_tailroom(skb)'
can completely be taken care of in skb_cow_data() by the code
in branch "if (!skb_has_frag_list())".

Also, remove the 'TODO:' annotation, as the pages in skbs are not
writable, see more on commit 3cf4375a0904 ("tipc: do not write
skb_shinfo frags when doing decrytion").

Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/crypto.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index e701651f6533..c5eefe4a8c4d 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -757,21 +757,10 @@ static int tipc_aead_encrypt(struct tipc_aead *aead, struct sk_buff *skb,
             skb_tailroom(skb), tailen);
     }
 
-    if (unlikely(!skb_cloned(skb) && tailen <= skb_tailroom(skb))) {
-        nsg = 1;
-        trailer = skb;
-    } else {
-        /* TODO: We could avoid skb_cow_data() if skb has no frag_list
-        * e.g. by skb_fill_page_desc() to add another page to the skb
-        * with the wanted tailen... However, page skbs look not often,
-        * so take it easy now!
-        * Cloned skbs e.g. from link_xmit() seems no choice though :(
-        */
-        nsg = skb_cow_data(skb, tailen, &trailer);
-        if (unlikely(nsg < 0)) {
-            pr_err("TX: skb_cow_data() returned %d\n", nsg);
-            return nsg;
-        }
+    nsg = skb_cow_data(skb, tailen, &trailer);
+    if (unlikely(nsg < 0)) {
+        pr_err("TX: skb_cow_data() returned %d\n", nsg);
+        return nsg;
     }
 
     pskb_put(skb, trailer, tailen);
-- 
2.27.0

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

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

[tipc-discussion] [PATCH net-next] tipc: delete the unlikely branch in tipc_aead_encrypt

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

When a skb comes to tipc_aead_encrypt(), it's always linear. The
unlikely check 'skb_cloned(skb) && tailen <= skb_tailroom(skb)'
can completely be taken care of in skb_cow_data() by the code
in branch "if (!skb_has_frag_list())".

Also, remove the 'TODO:' annotation, as the pages in skbs are not
writable, see more on commit 3cf4375a0904 ("tipc: do not write
skb_shinfo frags when doing decrytion").

Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/crypto.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index e701651f6533..c5eefe4a8c4d 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -757,21 +757,10 @@ static int tipc_aead_encrypt(struct tipc_aead *aead, struct sk_buff *skb,
 			 skb_tailroom(skb), tailen);
 	}
 
-	if (unlikely(!skb_cloned(skb) && tailen <= skb_tailroom(skb))) {
-		nsg = 1;
-		trailer = skb;
-	} else {
-		/* TODO: We could avoid skb_cow_data() if skb has no frag_list
-		 * e.g. by skb_fill_page_desc() to add another page to the skb
-		 * with the wanted tailen... However, page skbs look not often,
-		 * so take it easy now!
-		 * Cloned skbs e.g. from link_xmit() seems no choice though :(
-		 */
-		nsg = skb_cow_data(skb, tailen, &trailer);
-		if (unlikely(nsg < 0)) {
-			pr_err("TX: skb_cow_data() returned %d\n", nsg);
-			return nsg;
-		}
+	nsg = skb_cow_data(skb, tailen, &trailer);
+	if (unlikely(nsg < 0)) {
+		pr_err("TX: skb_cow_data() returned %d\n", nsg);
+		return nsg;
 	}
 
 	pskb_put(skb, trailer, tailen);
-- 
2.27.0

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

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

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

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

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

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

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

-----Original Message-----
From: Tadeusz Struk <tad...@li...> 
Sent: Tuesday, November 16, 2021 12:02 AM
To: da...@da...
Cc: Tadeusz Struk <tad...@li...>; Jon Maloy <jm...@re...>; Xue, Ying <Yin...@wi...>; Jakub Kicinski <ku...@ke...>; ne...@vg...; tip...@li...; lin...@vg...; st...@vg...; Dmitry Vyukov <dv...@go...>
Subject: [PATCH v2] tipc: check for null after calling kmemdup

kmemdup can return a null pointer so need to check for it, otherwise the null key will be dereferenced later in tipc_crypto_key_xmit as can be seen in the trace [1].

Cc: Jon Maloy <jm...@re...>
Cc: Ying Xue <yin...@wi...>
Cc: "David S. Miller" <da...@da...>
Cc: Jakub Kicinski <ku...@ke...>
Cc: ne...@vg...
Cc: tip...@li...
Cc: lin...@vg...
Cc: st...@vg... # 5.15, 5.14, 5.10

[1] https://syzkaller.appspot.com/bug?id=bca180abb29567b189efdbdb34cbf7ba851c2a58

Reported-by: Dmitry Vyukov <dv...@go...>
Signed-off-by: Tadeusz Struk <tad...@li...>
---
Changed in v2:
- use tipc_aead_free() to free all crytpo tfm instances
  that might have been allocated before the fail.
---
 net/tipc/crypto.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c index dc60c32bb70d..d293614d5fc6 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -597,6 +597,10 @@ static int tipc_aead_init(struct tipc_aead **aead, struct tipc_aead_key *ukey,
 	tmp->cloned = NULL;
 	tmp->authsize = TIPC_AES_GCM_TAG_SIZE;
 	tmp->key = kmemdup(ukey, tipc_aead_key_size(ukey), GFP_KERNEL);
+	if (!tmp->key) {
+		tipc_aead_free(&tmp->rcu);
+		return -ENOMEM;
+	}
 	memcpy(&tmp->salt, ukey->key + keylen, TIPC_AES_GCM_SALT_SIZE);
 	atomic_set(&tmp->users, 0);
 	atomic64_set(&tmp->seqno, 0);
--
2.33.1