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

Re: [tipc-discussion] [net v1 2/3] tipc: fix wrong socket reference counter after tipc_sk_timeout() returns

[Ying X. <yin...@wi...>]

On 8/13/19 6:01 PM, Tung Nguyen wrote:
> When tipc_sk_timeout() is executed but user space is grabbing
> ownership, this function rearms itself and returns. However, the
> socket reference counter is not reduced. This causes potential
> unexpected behavior.
> 
> This commit fixes it by calling sock_put() before tipc_sk_timeout()
> returns in the above-mentioned case.
> 
> Fixes: afe8792fec69 ("tipc: refactor function tipc_sk_timeout()")
> Signed-off-by: Tung Nguyen <tun...@de...>

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

> ---
>  net/tipc/socket.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index dcb8b6082757..9fd9a5727786 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -2683,6 +2683,7 @@ static void tipc_sk_timeout(struct timer_list *t)
>  	if (sock_owned_by_user(sk)) {
>  		sk_reset_timer(sk, &sk->sk_timer, jiffies + HZ / 20);
>  		bh_unlock_sock(sk);
> +		sock_put(sk);
>  		return;
>  	}
>  
> 

Re: [tipc-discussion] [net v1 1/3] tipc: fix potential memory leak in __tipc_sendmsg()

[Ying X. <yin...@wi...>]

On 8/13/19 6:01 PM, Tung Nguyen wrote:
> When initiating a connection message to a server side, the connection
> message is cloned and added to the socket write queue. However, if the
> cloning is failed, only the socket write queue is purged. It causes
> memory leak because the original connection message is not freed.
> 
> This commit fixes it by purging the list of connection message when
> it cannot be cloned.
> 
> Fixes: 6787927475e5 ("tipc: buffer overflow handling in listener socket")
> Reported-by: Hoang Le <hoa...@de...>
> Signed-off-by: Tung Nguyen <tun...@de...>

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

> ---
>  net/tipc/socket.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 83ae41d7e554..dcb8b6082757 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -1392,8 +1392,10 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
>  	rc = tipc_msg_build(hdr, m, 0, dlen, mtu, &pkts);
>  	if (unlikely(rc != dlen))
>  		return rc;
> -	if (unlikely(syn && !tipc_msg_skb_clone(&pkts, &sk->sk_write_queue)))
> +	if (unlikely(syn && !tipc_msg_skb_clone(&pkts, &sk->sk_write_queue))) {
> +		__skb_queue_purge(&pkts);
>  		return -ENOMEM;
> +	}
>  
>  	trace_tipc_sk_sendmsg(sk, skb_peek(&pkts), TIPC_DUMP_SK_SNDQ, " ");
>  	rc = tipc_node_xmit(net, &pkts, dnode, tsk->portid);
> 

[tipc-discussion] [net v1 2/3] tipc: fix wrong socket reference counter after tipc_sk_timeout() returns

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

When tipc_sk_timeout() is executed but user space is grabbing
ownership, this function rearms itself and returns. However, the
socket reference counter is not reduced. This causes potential
unexpected behavior.

This commit fixes it by calling sock_put() before tipc_sk_timeout()
returns in the above-mentioned case.

Fixes: afe8792fec69 ("tipc: refactor function tipc_sk_timeout()")
Signed-off-by: Tung Nguyen <tun...@de...>
---
 net/tipc/socket.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index dcb8b6082757..9fd9a5727786 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2683,6 +2683,7 @@ static void tipc_sk_timeout(struct timer_list *t)
 	if (sock_owned_by_user(sk)) {
 		sk_reset_timer(sk, &sk->sk_timer, jiffies + HZ / 20);
 		bh_unlock_sock(sk);
+		sock_put(sk);
 		return;
 	}
 
-- 
2.17.1

[tipc-discussion] [net v1 3/3] tipc: fix connection failure under link congestion

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

When initiating a connection message under link congestion,
function __tipc_sendmsg() is used to send the connection message to
a listening socket. Function tipc_wait_for_cond() is called to wait
until the link is not congested. However, it calls tipc_sk_sock_err()
for sanity check and this function returns -ENOTCONN immediately
because the socket state is not ESTABLISHED.

This commit fixes this issue by moving the sanity check for
connection-oriented socket from tipc_sk_sock_err() to
__tipc_sendstream().

Fixes: 8c44e1af16b2 ("tipc: unify tipc_wait_for_sndpkt() and tipc_wait_for_sndmsg() functions)
Signed-off-by: Tung Nguyen <tun...@de...>
---
 net/tipc/socket.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 9fd9a5727786..0ce441fd126c 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -363,12 +363,9 @@ static int tipc_sk_sock_err(struct socket *sock, long *timeout)
 
 	if (err)
 		return err;
-	if (typ == SOCK_STREAM || typ == SOCK_SEQPACKET) {
-		if (sk->sk_state == TIPC_DISCONNECTING)
-			return -EPIPE;
-		else if (!tipc_sk_connected(sk))
-			return -ENOTCONN;
-	}
+	if ((typ == SOCK_STREAM || typ == SOCK_SEQPACKET) &&
+	    (sk->sk_state == TIPC_DISCONNECTING))
+		return -EPIPE;
 	if (!*timeout)
 		return -EAGAIN;
 	if (signal_pending(current))
@@ -1462,6 +1459,13 @@ static int __tipc_sendstream(struct socket *sock, struct msghdr *m, size_t dlen)
 		return rc;
 	}
 
+	if (!tipc_sk_connected(sk)) {
+		if (sk->sk_state == TIPC_DISCONNECTING)
+			return -EPIPE;
+		else
+			return -ENOTCONN;
+	}
+
 	do {
 		rc = tipc_wait_for_cond(sock, &timeout,
 					(!tsk->cong_link_cnt &&
-- 
2.17.1

[tipc-discussion] [net v1 0/3] Fix some bugs at socket layer

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

This series fixes some bugs at socket layer.

Tung Nguyen (3):
  tipc: fix potential memory leak in __tipc_sendmsg()
  tipc: fix wrong socket reference counter after tipc_sk_timeout()
    returns
  tipc: fix connection failure under link congestion

 net/tipc/socket.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

-- 
2.17.1

[tipc-discussion] [net v1 1/3] tipc: fix potential memory leak in __tipc_sendmsg()

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

When initiating a connection message to a server side, the connection
message is cloned and added to the socket write queue. However, if the
cloning is failed, only the socket write queue is purged. It causes
memory leak because the original connection message is not freed.

This commit fixes it by purging the list of connection message when
it cannot be cloned.

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

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 83ae41d7e554..dcb8b6082757 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1392,8 +1392,10 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 	rc = tipc_msg_build(hdr, m, 0, dlen, mtu, &pkts);
 	if (unlikely(rc != dlen))
 		return rc;
-	if (unlikely(syn && !tipc_msg_skb_clone(&pkts, &sk->sk_write_queue)))
+	if (unlikely(syn && !tipc_msg_skb_clone(&pkts, &sk->sk_write_queue))) {
+		__skb_queue_purge(&pkts);
 		return -ENOMEM;
+	}
 
 	trace_tipc_sk_sendmsg(sk, skb_peek(&pkts), TIPC_DUMP_SK_SNDQ, " ");
 	rc = tipc_node_xmit(net, &pkts, dnode, tsk->portid);
-- 
2.17.1

Re: [tipc-discussion] [PATCH v2 0/3] Fix three issues found by syzbot

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

From: Ying Xue <yin...@wi...>
Date: Mon, 12 Aug 2019 15:32:39 +0800

> Ying Xue (3):
>   tipc: fix memory leak issue
>   tipc: fix memory leak issue

Please make the subject lines for these two patches unique.  Perhaps
mention what part of the tipc code has the memory leak you are fixing.

Thanks.

[tipc-discussion] [PATCH v2 0/3] Fix three issues found by syzbot

[Ying X. <yin...@wi...>]

In this series, try to fix two memory leak issues and another issue of
calling smp_processor_id() in preemptible context.

Changes since v1:
 - Fix "Reported-by:" missing in patch #3, which was reported by Jakub
   Kicinski

Ying Xue (3):
  tipc: fix memory leak issue
  tipc: fix memory leak issue
  tipc: fix issue of calling smp_processor_id() in preemptible

 net/tipc/group.c     | 22 +++++++++++++---------
 net/tipc/node.c      |  7 +++++--
 net/tipc/udp_media.c | 12 +++++++++---
 3 files changed, 27 insertions(+), 14 deletions(-)

-- 
2.7.4

[tipc-discussion] [PATCH v2 1/3] tipc: fix memory leak issue

[Ying X. <yin...@wi...>]

syzbot found the following memory leak:

[   68.602482][ T7130] kmemleak: 2 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
BUG: memory leak
unreferenced object 0xffff88810df83c00 (size 512):
  comm "softirq", pid 0, jiffies 4294942354 (age 19.830s)
  hex dump (first 32 bytes):
    38 1a 0d 0f 81 88 ff ff 38 1a 0d 0f 81 88 ff ff  8.......8.......
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<000000009375ee42>] kmem_cache_alloc_node+0x153/0x2a0
    [<000000004c563922>] __alloc_skb+0x6e/0x210
    [<00000000ec87bfa1>] tipc_buf_acquire+0x2f/0x80
    [<00000000d151ef84>] tipc_msg_create+0x37/0xe0
    [<000000008bb437b0>] tipc_group_create_event+0xb3/0x1b0
    [<00000000947b1d0f>] tipc_group_proto_rcv+0x569/0x640
    [<00000000b75ab039>] tipc_sk_filter_rcv+0x9ac/0xf20
    [<000000000dab7a6c>] tipc_sk_rcv+0x494/0x8a0
    [<00000000023a7ddd>] tipc_node_xmit+0x196/0x1f0
    [<00000000337dd9eb>] tipc_node_distr_xmit+0x7d/0x120
    [<00000000b6375182>] tipc_group_delete+0xe6/0x130
    [<000000000361ba2b>] tipc_sk_leave+0x57/0xb0
    [<000000009df90505>] tipc_release+0x7b/0x5e0
    [<000000009f3189da>] __sock_release+0x4b/0xe0
    [<00000000d3568ee0>] sock_close+0x1b/0x30
    [<00000000266a6215>] __fput+0xed/0x300

Reported-by: syz...@sy...
Signed-off-by: Hillf Danton <hd...@si...>
Signed-off-by: Ying Xue <yin...@wi...>
---
 net/tipc/node.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index 7ca0190..d1852fc 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -1469,10 +1469,13 @@ int tipc_node_xmit(struct net *net, struct sk_buff_head *list,
 	spin_unlock_bh(&le->lock);
 	tipc_node_read_unlock(n);
 
-	if (unlikely(rc == -ENOBUFS))
+	if (unlikely(rc == -ENOBUFS)) {
 		tipc_node_link_down(n, bearer_id, false);
-	else
+		skb_queue_purge(list);
+		skb_queue_purge(&xmitq);
+	} else {
 		tipc_bearer_xmit(net, bearer_id, &xmitq, &le->maddr);
+	}
 
 	tipc_node_put(n);
 
-- 
2.7.4

[tipc-discussion] [PATCH v2 3/3] tipc: fix issue of calling smp_processor_id() in preemptible

[Ying X. <yin...@wi...>]

syzbot found the following issue:

[   81.119772][ T8612] BUG: using smp_processor_id() in preemptible [00000000] code: syz-executor834/8612
[   81.136212][ T8612] caller is dst_cache_get+0x3d/0xb0
[   81.141450][ T8612] CPU: 0 PID: 8612 Comm: syz-executor834 Not tainted 5.2.0-rc6+ #48
[   81.149435][ T8612] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
[   81.159480][ T8612] Call Trace:
[   81.162789][ T8612]  dump_stack+0x172/0x1f0
[   81.167123][ T8612]  debug_smp_processor_id+0x251/0x280
[   81.172479][ T8612]  dst_cache_get+0x3d/0xb0
[   81.176928][ T8612]  tipc_udp_xmit.isra.0+0xc4/0xb80
[   81.182046][ T8612]  ? kasan_kmalloc+0x9/0x10
[   81.186531][ T8612]  ? tipc_udp_addr2str+0x170/0x170
[   81.191641][ T8612]  ? __copy_skb_header+0x2e8/0x560
[   81.196750][ T8612]  ? __skb_checksum_complete+0x3f0/0x3f0
[   81.202364][ T8612]  ? netdev_alloc_frag+0x1b0/0x1b0
[   81.207452][ T8612]  ? skb_copy_header+0x21/0x2b0
[   81.212282][ T8612]  ? __pskb_copy_fclone+0x516/0xc90
[   81.217470][ T8612]  tipc_udp_send_msg+0x29a/0x4b0
[   81.222400][ T8612]  tipc_bearer_xmit_skb+0x16c/0x360
[   81.227585][ T8612]  tipc_enable_bearer+0xabe/0xd20
[   81.232606][ T8612]  ? __nla_validate_parse+0x2d0/0x1ee0
[   81.238048][ T8612]  ? tipc_bearer_xmit_skb+0x360/0x360
[   81.243401][ T8612]  ? nla_memcpy+0xb0/0xb0
[   81.247710][ T8612]  ? nla_memcpy+0xb0/0xb0
[   81.252020][ T8612]  ? __nla_parse+0x43/0x60
[   81.256417][ T8612]  __tipc_nl_bearer_enable+0x2de/0x3a0
[   81.261856][ T8612]  ? __tipc_nl_bearer_enable+0x2de/0x3a0
[   81.267467][ T8612]  ? tipc_nl_bearer_disable+0x40/0x40
[   81.272848][ T8612]  ? unwind_get_return_address+0x58/0xa0
[   81.278501][ T8612]  ? lock_acquire+0x16f/0x3f0
[   81.283190][ T8612]  tipc_nl_bearer_enable+0x23/0x40
[   81.288300][ T8612]  genl_family_rcv_msg+0x74b/0xf90
[   81.293404][ T8612]  ? genl_unregister_family+0x790/0x790
[   81.298935][ T8612]  ? __lock_acquire+0x54f/0x5490
[   81.303852][ T8612]  ? __netlink_lookup+0x3fa/0x7b0
[   81.308865][ T8612]  genl_rcv_msg+0xca/0x16c
[   81.313266][ T8612]  netlink_rcv_skb+0x177/0x450
[   81.318043][ T8612]  ? genl_family_rcv_msg+0xf90/0xf90
[   81.323311][ T8612]  ? netlink_ack+0xb50/0xb50
[   81.327906][ T8612]  ? lock_acquire+0x16f/0x3f0
[   81.332589][ T8612]  ? kasan_check_write+0x14/0x20
[   81.337511][ T8612]  genl_rcv+0x29/0x40
[   81.341485][ T8612]  netlink_unicast+0x531/0x710
[   81.346268][ T8612]  ? netlink_attachskb+0x770/0x770
[   81.351374][ T8612]  ? _copy_from_iter_full+0x25d/0x8c0
[   81.356765][ T8612]  ? __sanitizer_cov_trace_cmp8+0x18/0x20
[   81.362479][ T8612]  ? __check_object_size+0x3d/0x42f
[   81.367667][ T8612]  netlink_sendmsg+0x8ae/0xd70
[   81.372415][ T8612]  ? netlink_unicast+0x710/0x710
[   81.377520][ T8612]  ? aa_sock_msg_perm.isra.0+0xba/0x170
[   81.383051][ T8612]  ? apparmor_socket_sendmsg+0x2a/0x30
[   81.388530][ T8612]  ? __sanitizer_cov_trace_const_cmp4+0x16/0x20
[   81.394775][ T8612]  ? security_socket_sendmsg+0x8d/0xc0
[   81.400240][ T8612]  ? netlink_unicast+0x710/0x710
[   81.405161][ T8612]  sock_sendmsg+0xd7/0x130
[   81.409561][ T8612]  ___sys_sendmsg+0x803/0x920
[   81.414220][ T8612]  ? copy_msghdr_from_user+0x430/0x430
[   81.419667][ T8612]  ? _raw_spin_unlock_irqrestore+0x6b/0xe0
[   81.425461][ T8612]  ? debug_object_active_state+0x25d/0x380
[   81.431255][ T8612]  ? __lock_acquire+0x54f/0x5490
[   81.436174][ T8612]  ? kasan_check_read+0x11/0x20
[   81.441208][ T8612]  ? _raw_spin_unlock_irqrestore+0xa4/0xe0
[   81.447008][ T8612]  ? mark_held_locks+0xf0/0xf0
[   81.451768][ T8612]  ? __call_rcu.constprop.0+0x28b/0x720
[   81.457298][ T8612]  ? call_rcu+0xb/0x10
[   81.461353][ T8612]  ? __sanitizer_cov_trace_const_cmp4+0x16/0x20
[   81.467589][ T8612]  ? __fget_light+0x1a9/0x230
[   81.472249][ T8612]  ? __fdget+0x1b/0x20
[   81.476301][ T8612]  ? __sanitizer_cov_trace_const_cmp8+0x18/0x20
[   81.482545][ T8612]  __sys_sendmsg+0x105/0x1d0
[   81.487115][ T8612]  ? __ia32_sys_shutdown+0x80/0x80
[   81.492208][ T8612]  ? blkcg_maybe_throttle_current+0x5e2/0xfb0
[   81.498272][ T8612]  ? trace_hardirqs_on_thunk+0x1a/0x1c
[   81.503726][ T8612]  ? do_syscall_64+0x26/0x680
[   81.508385][ T8612]  ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
[   81.514444][ T8612]  ? do_syscall_64+0x26/0x680
[   81.519110][ T8612]  __x64_sys_sendmsg+0x78/0xb0
[   81.523862][ T8612]  do_syscall_64+0xfd/0x680
[   81.528352][ T8612]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[   81.534234][ T8612] RIP: 0033:0x444679
[   81.538114][ T8612] Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 1b d8 fb ff c3 66 2e 0f 1f 84 00 00 00 00
[   81.557709][ T8612] RSP: 002b:00007fff0201a8b8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[   81.566147][ T8612] RAX: ffffffffffffffda RBX: 00000000004002e0 RCX: 0000000000444679
[   81.574108][ T8612] RDX: 0000000000000000 RSI: 0000000020000580 RDI: 0000000000000003
[   81.582152][ T8612] RBP: 00000000006cf018 R08: 0000000000000001 R09: 00000000004002e0
[   81.590113][ T8612] R10: 0000000000000008 R11: 0000000000000246 R12: 0000000000402320
[   81.598089][ T8612] R13: 00000000004023b0 R14: 0000000000000000 R15: 0000000000

In commit e9c1a793210f ("tipc: add dst_cache support for udp media")
dst_cache_get() was introduced to be called in tipc_udp_xmit(). But
smp_processor_id() called by dst_cache_get() cannot be invoked in
preemptible context, as a result, the complaint above was reported.

Fixes: e9c1a793210f ("tipc: add dst_cache support for udp media")
Reported-by: syz...@sy...
Signed-off-by: Hillf Danton <hd...@si...>
Signed-off-by: Ying Xue <yin...@wi...>
---
 net/tipc/udp_media.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index 287df687..ca3ae2e 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c
@@ -224,6 +224,8 @@ static int tipc_udp_send_msg(struct net *net, struct sk_buff *skb,
 	struct udp_bearer *ub;
 	int err = 0;
 
+	local_bh_disable();
+
 	if (skb_headroom(skb) < UDP_MIN_HEADROOM) {
 		err = pskb_expand_head(skb, UDP_MIN_HEADROOM, 0, GFP_ATOMIC);
 		if (err)
@@ -237,9 +239,12 @@ static int tipc_udp_send_msg(struct net *net, struct sk_buff *skb,
 		goto out;
 	}
 
-	if (addr->broadcast != TIPC_REPLICAST_SUPPORT)
-		return tipc_udp_xmit(net, skb, ub, src, dst,
-				     &ub->rcast.dst_cache);
+	if (addr->broadcast != TIPC_REPLICAST_SUPPORT) {
+		err = tipc_udp_xmit(net, skb, ub, src, dst,
+				    &ub->rcast.dst_cache);
+		local_bh_enable();
+		return err;
+	}
 
 	/* Replicast, send an skb to each configured IP address */
 	list_for_each_entry_rcu(rcast, &ub->rcast.list, list) {
@@ -259,6 +264,7 @@ static int tipc_udp_send_msg(struct net *net, struct sk_buff *skb,
 	err = 0;
 out:
 	kfree_skb(skb);
+	local_bh_enable();
 	return err;
 }
 
-- 
2.7.4

[tipc-discussion] [PATCH v2 2/3] tipc: fix memory leak issue

[Ying X. <yin...@wi...>]

syzbot found the following memory leak issue:

[   72.286706][ T7064] kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
BUG: memory leak
unreferenced object 0xffff888122bca200 (size 128):
  comm "syz-executor232", pid 7065, jiffies 4294943817 (age 8.880s)
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 18 a2 bc 22 81 88 ff ff  ..........."....
  backtrace:
    [<000000005bada299>] kmem_cache_alloc_trace+0x145/0x2c0
    [<00000000e7bcdc9f>] tipc_group_create_member+0x3c/0x190
    [<0000000005f56f40>] tipc_group_add_member+0x34/0x40
    [<0000000044406683>] tipc_nametbl_build_group+0x9b/0xf0
    [<000000009f71e803>] tipc_setsockopt+0x170/0x490
    [<000000007f61cbc2>] __sys_setsockopt+0x10f/0x220
    [<00000000cc630372>] __x64_sys_setsockopt+0x26/0x30
    [<00000000ec30be33>] do_syscall_64+0x76/0x1a0
    [<00000000271be3e6>] entry_SYSCALL_64_after_hwframe+0x44/0xa9

Reported-by: syz...@sy...
Signed-off-by: Hillf Danton <hd...@si...>
Signed-off-by: Ying Xue <yin...@wi...>
---
 net/tipc/group.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/net/tipc/group.c b/net/tipc/group.c
index 5f98d38..cbc540a 100644
--- a/net/tipc/group.c
+++ b/net/tipc/group.c
@@ -273,8 +273,8 @@ static struct tipc_member *tipc_group_find_node(struct tipc_group *grp,
 	return NULL;
 }
 
-static void tipc_group_add_to_tree(struct tipc_group *grp,
-				   struct tipc_member *m)
+struct tipc_member *tipc_group_add_to_tree(struct tipc_group *grp,
+					   struct tipc_member *m)
 {
 	u64 nkey, key = (u64)m->node << 32 | m->port;
 	struct rb_node **n, *parent = NULL;
@@ -282,7 +282,6 @@ static void tipc_group_add_to_tree(struct tipc_group *grp,
 
 	n = &grp->members.rb_node;
 	while (*n) {
-		tmp = container_of(*n, struct tipc_member, tree_node);
 		parent = *n;
 		tmp = container_of(parent, struct tipc_member, tree_node);
 		nkey = (u64)tmp->node << 32 | tmp->port;
@@ -291,17 +290,18 @@ static void tipc_group_add_to_tree(struct tipc_group *grp,
 		else if (key > nkey)
 			n = &(*n)->rb_right;
 		else
-			return;
+			return tmp;
 	}
 	rb_link_node(&m->tree_node, parent, n);
 	rb_insert_color(&m->tree_node, &grp->members);
+	return m;
 }
 
 static struct tipc_member *tipc_group_create_member(struct tipc_group *grp,
 						    u32 node, u32 port,
 						    u32 instance, int state)
 {
-	struct tipc_member *m;
+	struct tipc_member *m, *n;
 
 	m = kzalloc(sizeof(*m), GFP_ATOMIC);
 	if (!m)
@@ -315,10 +315,14 @@ static struct tipc_member *tipc_group_create_member(struct tipc_group *grp,
 	m->instance = instance;
 	m->bc_acked = grp->bc_snd_nxt - 1;
 	grp->member_cnt++;
-	tipc_group_add_to_tree(grp, m);
-	tipc_nlist_add(&grp->dests, m->node);
-	m->state = state;
-	return m;
+	n = tipc_group_add_to_tree(grp, m);
+	if (n == m) {
+		tipc_nlist_add(&grp->dests, m->node);
+		m->state = state;
+	} else {
+		kfree(m);
+	}
+	return n;
 }
 
 void tipc_group_add_member(struct tipc_group *grp, u32 node,
-- 
2.7.4

Re: [tipc-discussion] [PATCH v3] tipc: initialise addr_trail_end when setting node addresses

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

From: Chris Packham <chr...@al...>
Date: Mon, 12 Aug 2019 08:18:25 +1200

> We set the field 'addr_trial_end' to 'jiffies', instead of the current
> value 0, at the moment the node address is initialized. This guarantees
> we don't inadvertently enter an address trial period when the node
> address is explicitly set by the user.
> 
> Signed-off-by: Chris Packham <chr...@al...>
> Acked-by: Jon Maloy <jon...@er...>

Applied.

Re: [tipc-discussion] [PATCH v2] tipc: initialise addr_trail_end when setting node addresses

[Jon M. <jon...@er...>]

I would re-phrase this a little:
We set the field 'addr_trial_end' to 'jiffies', instead of the current value 0,  at the moment the node address is initialized. 
This guarantees we don't inadvertently enter an address trial period when the node address is explicitly set by the user.

Acked-by: Jon Maloy <jon...@er...>


> -----Original Message-----
> From: net...@vg... <net...@vg...> On
> Behalf Of Chris Packham
> Sent: 8-Aug-19 20:55
> To: Jon Maloy <jon...@er...>; yin...@wi...;
> da...@da...
> Cc: ne...@vg...; tip...@li...; linux-
> ke...@vg...; Chris Packham <chr...@al...>
> Subject: [PATCH v2] tipc: initialise addr_trail_end when setting node addresses
> 
> Ensure addr_trail_end is set to jiffies when configuring the node address. This
> ensures that we don't treat the initial value of 0 as being a wrapped. This isn't a
> problem when using auto-generated node addresses because the
> addr_trail_end is updated for the duplicate address detection phase.
> 
> Signed-off-by: Chris Packham <chr...@al...>
> ---
> Changes in v2:
> - move setting to tipc_set_node_addr() as suggested
> - reword commit message
> 
>  net/tipc/addr.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/tipc/addr.c b/net/tipc/addr.c index
> b88d48d00913..0f1eaed1bd1b 100644
> --- a/net/tipc/addr.c
> +++ b/net/tipc/addr.c
> @@ -75,6 +75,7 @@ void tipc_set_node_addr(struct net *net, u32 addr)
>  		tipc_set_node_id(net, node_id);
>  	}
>  	tn->trial_addr = addr;
> +	tn->addr_trial_end = jiffies;
>  	pr_info("32-bit node address hash set to %x\n", addr);  }
> 
> --
> 2.22.0

[tipc-discussion] [PATCH 1/3] tipc: fix memory leak issue

[Ying X. <yin...@wi...>]

syzbot found the following memory leak:

[   68.602482][ T7130] kmemleak: 2 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
BUG: memory leak
unreferenced object 0xffff88810df83c00 (size 512):
  comm "softirq", pid 0, jiffies 4294942354 (age 19.830s)
  hex dump (first 32 bytes):
    38 1a 0d 0f 81 88 ff ff 38 1a 0d 0f 81 88 ff ff  8.......8.......
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<000000009375ee42>] kmem_cache_alloc_node+0x153/0x2a0
    [<000000004c563922>] __alloc_skb+0x6e/0x210
    [<00000000ec87bfa1>] tipc_buf_acquire+0x2f/0x80
    [<00000000d151ef84>] tipc_msg_create+0x37/0xe0
    [<000000008bb437b0>] tipc_group_create_event+0xb3/0x1b0
    [<00000000947b1d0f>] tipc_group_proto_rcv+0x569/0x640
    [<00000000b75ab039>] tipc_sk_filter_rcv+0x9ac/0xf20
    [<000000000dab7a6c>] tipc_sk_rcv+0x494/0x8a0
    [<00000000023a7ddd>] tipc_node_xmit+0x196/0x1f0
    [<00000000337dd9eb>] tipc_node_distr_xmit+0x7d/0x120
    [<00000000b6375182>] tipc_group_delete+0xe6/0x130
    [<000000000361ba2b>] tipc_sk_leave+0x57/0xb0
    [<000000009df90505>] tipc_release+0x7b/0x5e0
    [<000000009f3189da>] __sock_release+0x4b/0xe0
    [<00000000d3568ee0>] sock_close+0x1b/0x30
    [<00000000266a6215>] __fput+0xed/0x300

Reported-by: syz...@sy...
Signed-off-by: Hillf Danton <hd...@si...>
Signed-off-by: Ying Xue <yin...@wi...>
---
 net/tipc/node.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index 7ca0190..d1852fc 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -1469,10 +1469,13 @@ int tipc_node_xmit(struct net *net, struct sk_buff_head *list,
 	spin_unlock_bh(&le->lock);
 	tipc_node_read_unlock(n);
 
-	if (unlikely(rc == -ENOBUFS))
+	if (unlikely(rc == -ENOBUFS)) {
 		tipc_node_link_down(n, bearer_id, false);
-	else
+		skb_queue_purge(list);
+		skb_queue_purge(&xmitq);
+	} else {
 		tipc_bearer_xmit(net, bearer_id, &xmitq, &le->maddr);
+	}
 
 	tipc_node_put(n);
 
-- 
2.7.4

[tipc-discussion] [PATCH 0/3] Fix three issues found by syzbot

[Ying X. <yin...@wi...>]

In this series, try to fix two memory leak issues and another issue of
calling smp_processor_id() in preemptible context.

Ying Xue (3):
  tipc: fix memory leak issue
  tipc: fix memory leak issue
  tipc: fix issue of calling smp_processor_id() in preemptible

 net/tipc/group.c     | 22 +++++++++++++---------
 net/tipc/node.c      |  7 +++++--
 net/tipc/udp_media.c | 12 +++++++++---
 3 files changed, 27 insertions(+), 14 deletions(-)

-- 
2.7.4

[tipc-discussion] [PATCH 3/3] tipc: fix issue of calling smp_processor_id() in preemptible

[Ying X. <yin...@wi...>]

syzbot found the following issue:

[   81.119772][ T8612] BUG: using smp_processor_id() in preemptible [00000000] code: syz-executor834/8612
[   81.136212][ T8612] caller is dst_cache_get+0x3d/0xb0
[   81.141450][ T8612] CPU: 0 PID: 8612 Comm: syz-executor834 Not tainted 5.2.0-rc6+ #48
[   81.149435][ T8612] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
[   81.159480][ T8612] Call Trace:
[   81.162789][ T8612]  dump_stack+0x172/0x1f0
[   81.167123][ T8612]  debug_smp_processor_id+0x251/0x280
[   81.172479][ T8612]  dst_cache_get+0x3d/0xb0
[   81.176928][ T8612]  tipc_udp_xmit.isra.0+0xc4/0xb80
[   81.182046][ T8612]  ? kasan_kmalloc+0x9/0x10
[   81.186531][ T8612]  ? tipc_udp_addr2str+0x170/0x170
[   81.191641][ T8612]  ? __copy_skb_header+0x2e8/0x560
[   81.196750][ T8612]  ? __skb_checksum_complete+0x3f0/0x3f0
[   81.202364][ T8612]  ? netdev_alloc_frag+0x1b0/0x1b0
[   81.207452][ T8612]  ? skb_copy_header+0x21/0x2b0
[   81.212282][ T8612]  ? __pskb_copy_fclone+0x516/0xc90
[   81.217470][ T8612]  tipc_udp_send_msg+0x29a/0x4b0
[   81.222400][ T8612]  tipc_bearer_xmit_skb+0x16c/0x360
[   81.227585][ T8612]  tipc_enable_bearer+0xabe/0xd20
[   81.232606][ T8612]  ? __nla_validate_parse+0x2d0/0x1ee0
[   81.238048][ T8612]  ? tipc_bearer_xmit_skb+0x360/0x360
[   81.243401][ T8612]  ? nla_memcpy+0xb0/0xb0
[   81.247710][ T8612]  ? nla_memcpy+0xb0/0xb0
[   81.252020][ T8612]  ? __nla_parse+0x43/0x60
[   81.256417][ T8612]  __tipc_nl_bearer_enable+0x2de/0x3a0
[   81.261856][ T8612]  ? __tipc_nl_bearer_enable+0x2de/0x3a0
[   81.267467][ T8612]  ? tipc_nl_bearer_disable+0x40/0x40
[   81.272848][ T8612]  ? unwind_get_return_address+0x58/0xa0
[   81.278501][ T8612]  ? lock_acquire+0x16f/0x3f0
[   81.283190][ T8612]  tipc_nl_bearer_enable+0x23/0x40
[   81.288300][ T8612]  genl_family_rcv_msg+0x74b/0xf90
[   81.293404][ T8612]  ? genl_unregister_family+0x790/0x790
[   81.298935][ T8612]  ? __lock_acquire+0x54f/0x5490
[   81.303852][ T8612]  ? __netlink_lookup+0x3fa/0x7b0
[   81.308865][ T8612]  genl_rcv_msg+0xca/0x16c
[   81.313266][ T8612]  netlink_rcv_skb+0x177/0x450
[   81.318043][ T8612]  ? genl_family_rcv_msg+0xf90/0xf90
[   81.323311][ T8612]  ? netlink_ack+0xb50/0xb50
[   81.327906][ T8612]  ? lock_acquire+0x16f/0x3f0
[   81.332589][ T8612]  ? kasan_check_write+0x14/0x20
[   81.337511][ T8612]  genl_rcv+0x29/0x40
[   81.341485][ T8612]  netlink_unicast+0x531/0x710
[   81.346268][ T8612]  ? netlink_attachskb+0x770/0x770
[   81.351374][ T8612]  ? _copy_from_iter_full+0x25d/0x8c0
[   81.356765][ T8612]  ? __sanitizer_cov_trace_cmp8+0x18/0x20
[   81.362479][ T8612]  ? __check_object_size+0x3d/0x42f
[   81.367667][ T8612]  netlink_sendmsg+0x8ae/0xd70
[   81.372415][ T8612]  ? netlink_unicast+0x710/0x710
[   81.377520][ T8612]  ? aa_sock_msg_perm.isra.0+0xba/0x170
[   81.383051][ T8612]  ? apparmor_socket_sendmsg+0x2a/0x30
[   81.388530][ T8612]  ? __sanitizer_cov_trace_const_cmp4+0x16/0x20
[   81.394775][ T8612]  ? security_socket_sendmsg+0x8d/0xc0
[   81.400240][ T8612]  ? netlink_unicast+0x710/0x710
[   81.405161][ T8612]  sock_sendmsg+0xd7/0x130
[   81.409561][ T8612]  ___sys_sendmsg+0x803/0x920
[   81.414220][ T8612]  ? copy_msghdr_from_user+0x430/0x430
[   81.419667][ T8612]  ? _raw_spin_unlock_irqrestore+0x6b/0xe0
[   81.425461][ T8612]  ? debug_object_active_state+0x25d/0x380
[   81.431255][ T8612]  ? __lock_acquire+0x54f/0x5490
[   81.436174][ T8612]  ? kasan_check_read+0x11/0x20
[   81.441208][ T8612]  ? _raw_spin_unlock_irqrestore+0xa4/0xe0
[   81.447008][ T8612]  ? mark_held_locks+0xf0/0xf0
[   81.451768][ T8612]  ? __call_rcu.constprop.0+0x28b/0x720
[   81.457298][ T8612]  ? call_rcu+0xb/0x10
[   81.461353][ T8612]  ? __sanitizer_cov_trace_const_cmp4+0x16/0x20
[   81.467589][ T8612]  ? __fget_light+0x1a9/0x230
[   81.472249][ T8612]  ? __fdget+0x1b/0x20
[   81.476301][ T8612]  ? __sanitizer_cov_trace_const_cmp8+0x18/0x20
[   81.482545][ T8612]  __sys_sendmsg+0x105/0x1d0
[   81.487115][ T8612]  ? __ia32_sys_shutdown+0x80/0x80
[   81.492208][ T8612]  ? blkcg_maybe_throttle_current+0x5e2/0xfb0
[   81.498272][ T8612]  ? trace_hardirqs_on_thunk+0x1a/0x1c
[   81.503726][ T8612]  ? do_syscall_64+0x26/0x680
[   81.508385][ T8612]  ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
[   81.514444][ T8612]  ? do_syscall_64+0x26/0x680
[   81.519110][ T8612]  __x64_sys_sendmsg+0x78/0xb0
[   81.523862][ T8612]  do_syscall_64+0xfd/0x680
[   81.528352][ T8612]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[   81.534234][ T8612] RIP: 0033:0x444679
[   81.538114][ T8612] Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 1b d8 fb ff c3 66 2e 0f 1f 84 00 00 00 00
[   81.557709][ T8612] RSP: 002b:00007fff0201a8b8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[   81.566147][ T8612] RAX: ffffffffffffffda RBX: 00000000004002e0 RCX: 0000000000444679
[   81.574108][ T8612] RDX: 0000000000000000 RSI: 0000000020000580 RDI: 0000000000000003
[   81.582152][ T8612] RBP: 00000000006cf018 R08: 0000000000000001 R09: 00000000004002e0
[   81.590113][ T8612] R10: 0000000000000008 R11: 0000000000000246 R12: 0000000000402320
[   81.598089][ T8612] R13: 00000000004023b0 R14: 0000000000000000 R15: 0000000000

In commit e9c1a793210f ("tipc: add dst_cache support for udp media")
dst_cache_get() was introduced to be called in tipc_udp_xmit(). But
smp_processor_id() called by dst_cache_get() cannot be invoked in
preemptible context, as a result, the complaint above was reported.

Fixes: e9c1a793210f ("tipc: add dst_cache support for udp media")
syz...@sy...
Signed-off-by: Hillf Danton <hd...@si...>
Signed-off-by: Ying Xue <yin...@wi...>
---
 net/tipc/udp_media.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index 287df687..ca3ae2e 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c
@@ -224,6 +224,8 @@ static int tipc_udp_send_msg(struct net *net, struct sk_buff *skb,
 	struct udp_bearer *ub;
 	int err = 0;
 
+	local_bh_disable();
+
 	if (skb_headroom(skb) < UDP_MIN_HEADROOM) {
 		err = pskb_expand_head(skb, UDP_MIN_HEADROOM, 0, GFP_ATOMIC);
 		if (err)
@@ -237,9 +239,12 @@ static int tipc_udp_send_msg(struct net *net, struct sk_buff *skb,
 		goto out;
 	}
 
-	if (addr->broadcast != TIPC_REPLICAST_SUPPORT)
-		return tipc_udp_xmit(net, skb, ub, src, dst,
-				     &ub->rcast.dst_cache);
+	if (addr->broadcast != TIPC_REPLICAST_SUPPORT) {
+		err = tipc_udp_xmit(net, skb, ub, src, dst,
+				    &ub->rcast.dst_cache);
+		local_bh_enable();
+		return err;
+	}
 
 	/* Replicast, send an skb to each configured IP address */
 	list_for_each_entry_rcu(rcast, &ub->rcast.list, list) {
@@ -259,6 +264,7 @@ static int tipc_udp_send_msg(struct net *net, struct sk_buff *skb,
 	err = 0;
 out:
 	kfree_skb(skb);
+	local_bh_enable();
 	return err;
 }
 
-- 
2.7.4

[tipc-discussion] [PATCH 2/3] tipc: fix memory leak issue

[Ying X. <yin...@wi...>]

syzbot found the following memory leak issue:

[   72.286706][ T7064] kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
BUG: memory leak
unreferenced object 0xffff888122bca200 (size 128):
  comm "syz-executor232", pid 7065, jiffies 4294943817 (age 8.880s)
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 18 a2 bc 22 81 88 ff ff  ..........."....
  backtrace:
    [<000000005bada299>] kmem_cache_alloc_trace+0x145/0x2c0
    [<00000000e7bcdc9f>] tipc_group_create_member+0x3c/0x190
    [<0000000005f56f40>] tipc_group_add_member+0x34/0x40
    [<0000000044406683>] tipc_nametbl_build_group+0x9b/0xf0
    [<000000009f71e803>] tipc_setsockopt+0x170/0x490
    [<000000007f61cbc2>] __sys_setsockopt+0x10f/0x220
    [<00000000cc630372>] __x64_sys_setsockopt+0x26/0x30
    [<00000000ec30be33>] do_syscall_64+0x76/0x1a0
    [<00000000271be3e6>] entry_SYSCALL_64_after_hwframe+0x44/0xa9

Reported-by: syz...@sy...
Signed-off-by: Hillf Danton <hd...@si...>
Signed-off-by: Ying Xue <yin...@wi...>
---
 net/tipc/group.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/net/tipc/group.c b/net/tipc/group.c
index 5f98d38..cbc540a 100644
--- a/net/tipc/group.c
+++ b/net/tipc/group.c
@@ -273,8 +273,8 @@ static struct tipc_member *tipc_group_find_node(struct tipc_group *grp,
 	return NULL;
 }
 
-static void tipc_group_add_to_tree(struct tipc_group *grp,
-				   struct tipc_member *m)
+struct tipc_member *tipc_group_add_to_tree(struct tipc_group *grp,
+					   struct tipc_member *m)
 {
 	u64 nkey, key = (u64)m->node << 32 | m->port;
 	struct rb_node **n, *parent = NULL;
@@ -282,7 +282,6 @@ static void tipc_group_add_to_tree(struct tipc_group *grp,
 
 	n = &grp->members.rb_node;
 	while (*n) {
-		tmp = container_of(*n, struct tipc_member, tree_node);
 		parent = *n;
 		tmp = container_of(parent, struct tipc_member, tree_node);
 		nkey = (u64)tmp->node << 32 | tmp->port;
@@ -291,17 +290,18 @@ static void tipc_group_add_to_tree(struct tipc_group *grp,
 		else if (key > nkey)
 			n = &(*n)->rb_right;
 		else
-			return;
+			return tmp;
 	}
 	rb_link_node(&m->tree_node, parent, n);
 	rb_insert_color(&m->tree_node, &grp->members);
+	return m;
 }
 
 static struct tipc_member *tipc_group_create_member(struct tipc_group *grp,
 						    u32 node, u32 port,
 						    u32 instance, int state)
 {
-	struct tipc_member *m;
+	struct tipc_member *m, *n;
 
 	m = kzalloc(sizeof(*m), GFP_ATOMIC);
 	if (!m)
@@ -315,10 +315,14 @@ static struct tipc_member *tipc_group_create_member(struct tipc_group *grp,
 	m->instance = instance;
 	m->bc_acked = grp->bc_snd_nxt - 1;
 	grp->member_cnt++;
-	tipc_group_add_to_tree(grp, m);
-	tipc_nlist_add(&grp->dests, m->node);
-	m->state = state;
-	return m;
+	n = tipc_group_add_to_tree(grp, m);
+	if (n == m) {
+		tipc_nlist_add(&grp->dests, m->node);
+		m->state = state;
+	} else {
+		kfree(m);
+	}
+	return n;
 }
 
 void tipc_group_add_member(struct tipc_group *grp, u32 node,
-- 
2.7.4

Re: [tipc-discussion] [net-next v3] tipc: add loopback device tracking

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

From: joh...@de...
Date: Wed,  7 Aug 2019 12:52:29 +1000

> From: John Rutherford <joh...@de...>
> 
> Since node internal messages are passed directly to the socket, it is not
> possible to observe those messages via tcpdump or wireshark.
> 
> We now remedy this by making it possible to clone such messages and send
> the clones to the loopback interface.  The clones are dropped at reception
> and have no functional role except making the traffic visible.
> 
> The feature is enabled if network taps are active for the loopback device.
> pcap filtering restrictions require the messages to be presented to the
> receiving side of the loopback device.
> 
> v3 - Function dev_nit_active used to check for network taps.
>    - Procedure netif_rx_ni used to send cloned messages to loopback device.
> 
> Signed-off-by: John Rutherford <joh...@de...>
> Acked-by: Jon Maloy <jon...@er...>
> Acked-by: Ying Xue <yin...@wi...>

Applied, thank you.

Re: [tipc-discussion] [PATCH] tipc: set addr_trail_end when using explicit node addresses

[Jon M. <jon...@er...>]

You should rather set this one unconditionally in tipc_set_node_addr().
The problems is not about the state machine, but that jiffies is close to the wrap-around time, so that it is perceived as being before the time "0".

BR
///jon


> -----Original Message-----
> From: net...@vg... <net...@vg...> On
> Behalf Of Chris Packham
> Sent: 7-Aug-19 00:56
> To: Jon Maloy <jon...@er...>; yin...@wi...;
> da...@da...
> Cc: ne...@vg...; tip...@li...; linux-
> ke...@vg...; Chris Packham <chr...@al...>
> Subject: [PATCH] tipc: set addr_trail_end when using explicit node addresses
> 
> When tipc uses auto-generated node addresses it goes through a duplicate
> address detection phase to ensure the address is unique.
> 
> When using explicitly configured node names the DAD phase is skipped.
> However addr_trail_end was being left set to 0 which causes parts of the tipc
> state machine to assume that the address is not yet valid and unnecessarily
> delays the discovery phase. By setting addr_trail_end to jiffies when using
> explicit addresses we ensure that we move straight to discovery.
> 
> Signed-off-by: Chris Packham <chr...@al...>
> ---
>  net/tipc/discover.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/tipc/discover.c b/net/tipc/discover.c index
> c138d68e8a69..f83bfe8c9443 100644
> --- a/net/tipc/discover.c
> +++ b/net/tipc/discover.c
> @@ -361,6 +361,8 @@ int tipc_disc_create(struct net *net, struct
> tipc_bearer *b,
>  	if (!tipc_own_addr(net)) {
>  		tn->addr_trial_end = jiffies + msecs_to_jiffies(1000);
>  		msg_set_type(buf_msg(d->skb), DSC_TRIAL_MSG);
> +	} else {
> +		tn->addr_trial_end = jiffies;
>  	}
>  	memcpy(&d->dest, dest, sizeof(*dest));
>  	d->net = net;
> --
> 2.22.0

Re: [tipc-discussion] Slowness forming TIPC cluster with explicit node addresses

[Jon M. <jon...@er...>]

> -----Original Message-----
> From: Chris Packham <Chr...@al...>
> Sent: 4-Aug-19 19:05
> To: Jon Maloy <jon...@er...>; tipc-
> dis...@li...
> Cc: ne...@vg...; lin...@vg...
> Subject: Re: Slowness forming TIPC cluster with explicit node addresses
> 
> On Sun, 2019-08-04 at 21:53 +0000, Jon Maloy wrote:
> >
> > >
> > > -----Original Message-----
> > > From: net...@vg... <net...@vg...>
> On
> > > Behalf Of Chris Packham
> > > Sent: 2-Aug-19 01:11
> > > To: Jon Maloy <jon...@er...>; tipc-
> > > dis...@li...
> > > Cc: ne...@vg...; lin...@vg...
> > > Subject: Re: Slowness forming TIPC cluster with explicit node
> > > addresses
> > >
> > > On Mon, 2019-07-29 at 09:04 +1200, Chris Packham wrote:
> > > >
> > > > On Fri, 2019-07-26 at 13:31 +0000, Jon Maloy wrote:
> > > > >
> > > > >
> > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > -----Original Message-----
> > > > > > From: net...@vg... <netdev-
> > > ow...@vg...>
> > > >
> > > > >
> > > > > >
> > > > > > On Behalf Of Chris Packham
> > > > > > Sent: 25-Jul-19 19:37
> > > > > > To: tip...@li...
> > > > > > Cc: ne...@vg...; lin...@vg...
> > > > > > Subject: Slowness forming TIPC cluster with explicit node
> > > > > > addresses
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > I'm having problems forming a TIPC cluster between 2 nodes.
> > > > > >
> > > > > > This is the basic steps I'm going through on each node.
> > > > > >
> > > > > > modprobe tipc
> > > > > > ip link set eth2 up
> > > > > > tipc node set addr 1.1.5 # or 1.1.6 tipc bearer enable media
> > > > > > eth dev eth0
> > > > > eth2, I assume...
> > > > >
> > > > Yes sorry I keep switching between between Ethernet ports for
> > > > testing
> > > > so I hand edited the email.
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > Then to confirm if the cluster is formed I use tipc link list
> > > > > >
> > > > > > [root@node-5 ~]# tipc link list
> > > > > > broadcast-link: up
> > > > > > ...
> > > > > >
> > > > > > Looking at tcpdump the two nodes are sending packets
> > > > > >
> > > > > > 22:30:05.782320 TIPC v2.0 1.1.5 > 0.0.0, headerlength 60
> > > > > > bytes,
> > > > > > MessageSize
> > > > > > 76 bytes, Neighbor Detection Protocol internal, messageType
> > > > > > Link
> > > > > > request
> > > > > > 22:30:05.863555 TIPC v2.0 1.1.6 > 0.0.0, headerlength 60
> > > > > > bytes,
> > > > > > MessageSize
> > > > > > 76 bytes, Neighbor Detection Protocol internal, messageType
> > > > > > Link
> > > > > > request
> > > > > >
> > > > > > Eventually (after a few minutes) the link does come up
> > > > > >
> > > > > > [root@node-6 ~]# tipc link list
> > > > > > broadcast-link: up
> > > > > > 1001006:eth2-1001005:eth2: up
> > > > > >
> > > > > > [root@node-5 ~]# tipc link list
> > > > > > broadcast-link: up
> > > > > > 1001005:eth2-1001006:eth2: up
> > > > > >
> > > > > > When I remove the "tipc node set addr" things seem to kick
> > > > > > into
> > > > > > life straight away
> > > > > >
> > > > > > [root@node-5 ~]# tipc link list
> > > > > > broadcast-link: up
> > > > > > 0050b61bd2aa:eth2-0050b61e6dfa:eth2: up
> > > > > >
> > > > > > So there appears to be some difference in behaviour between
> > > > > > having
> > > > > > an explicit node address and using the default. Unfortunately
> > > > > > our
> > > > > > application relies on setting the node addresses.
> > > > > I do this many times a day, without any problems. If there
> > > > > would be
> > > > > any time difference, I would expect the 'auto configurable'
> > > > > version
> > > > > to be slower, because it involves a DAD step.
> > > > > Are you sure you don't have any other nodes running in your
> > > > > system?
> > > > >
> > > > > ///jon
> > > > >
> > > > Nope the two nodes are connected back to back. Does the number of
> > > > Ethernet interfaces make a difference? As you can see I've got 3
> > > > on
> > > > each node. One is completely disconnected, one is for booting
> > > > over
> > > > TFTP
> > > >  (only used by U-boot) and the other is the USB Ethernet I'm
> > > > using for
> > > > testing.
> > > >
> > > So I can still reproduce this on nodes that only have one network
> > > interface and
> > > are the only things connected.
> > >
> > > I did find one thing that helps
> > >
> > > diff --git a/net/tipc/discover.c b/net/tipc/discover.c index
> > > c138d68e8a69..49921dad404a 100644
> > > --- a/net/tipc/discover.c
> > > +++ b/net/tipc/discover.c
> > > @@ -358,10 +358,10 @@ int tipc_disc_create(struct net *net, struct
> > > tipc_bearer *b,
> > >         tipc_disc_init_msg(net, d->skb, DSC_REQ_MSG, b);
> > >
> > >         /* Do we need an address trial period first ? */
> > > -       if (!tipc_own_addr(net)) {
> > > +//     if (!tipc_own_addr(net)) {
> > >                 tn->addr_trial_end = jiffies +
> > > msecs_to_jiffies(1000);
> > >                 msg_set_type(buf_msg(d->skb), DSC_TRIAL_MSG);
> > > -       }
> > > +//     }
> > >         memcpy(&d->dest, dest, sizeof(*dest));
> > >         d->net = net;
> > >         d->bearer_id = b->identity;
> > >
> > > I think because with pre-configured addresses the duplicate address
> > > detection
> > > is skipped the shorter init phase is skipped. Would is make sense
> > > to
> > > unconditionally do the trial step? Or is there some better way to
> > > get things to
> > > transition with pre-assigned addresses.
> >
> > I am on vacation until the end of next-week, so I can't give you any
> > good analysis right now.
> 
> Thanks for taking the time to respond.
> 
> > To do the trial step doesn’t make much sense to me, -it would only
> > delay the setup unnecessarily (but with only 1 second).
> > Can you check the initial value of addr_trial_end when there a pre-
> > configured address?
> 
> I had the same thought. For both my devices 'addr_trial_end = 0' so I
> think tipc_disc_addr_trial_msg should end up with trial == false

I suggest you try initializing it to jiffies and see what happens.

///jon

> 
> >
> > ///jon
> >

[tipc-discussion] [net-next v3] tipc: add loopback device tracking

[<joh...@de...>]

From: John Rutherford <joh...@de...>

Since node internal messages are passed directly to the socket, it is not
possible to observe those messages via tcpdump or wireshark.

We now remedy this by making it possible to clone such messages and send
the clones to the loopback interface.  The clones are dropped at reception
and have no functional role except making the traffic visible.

The feature is enabled if network taps are active for the loopback device.
pcap filtering restrictions require the messages to be presented to the
receiving side of the loopback device.

v3 - Function dev_nit_active used to check for network taps.
   - Procedure netif_rx_ni used to send cloned messages to loopback device.

Signed-off-by: John Rutherford <joh...@de...>
Acked-by: Jon Maloy <jon...@er...>
Acked-by: Ying Xue <yin...@wi...>
---
 net/tipc/bcast.c  |  4 +++-
 net/tipc/bearer.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 net/tipc/bearer.h | 10 +++++++++
 net/tipc/core.c   |  5 +++++
 net/tipc/core.h   |  3 +++
 net/tipc/node.c   |  1 +
 net/tipc/topsrv.c |  2 ++
 7 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
index 6c997d4..235331d 100644
--- a/net/tipc/bcast.c
+++ b/net/tipc/bcast.c
@@ -406,8 +406,10 @@ int tipc_mcast_xmit(struct net *net, struct sk_buff_head *pkts,
 			rc = tipc_bcast_xmit(net, pkts, cong_link_cnt);
 	}
 
-	if (dests->local)
+	if (dests->local) {
+		tipc_loopback_trace(net, &localq);
 		tipc_sk_mcast_rcv(net, &localq, &inputq);
+	}
 exit:
 	/* This queue should normally be empty by now */
 	__skb_queue_purge(pkts);
diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 2bed658..93c9616 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -389,6 +389,11 @@ int tipc_enable_l2_media(struct net *net, struct tipc_bearer *b,
 		dev_put(dev);
 		return -EINVAL;
 	}
+	if (dev == net->loopback_dev) {
+		dev_put(dev);
+		pr_info("Enabling <%s> not permitted\n", b->name);
+		return -EINVAL;
+	}
 
 	/* Autoconfigure own node identity if needed */
 	if (!tipc_own_id(net) && hwaddr_len <= NODE_ID_LEN) {
@@ -674,6 +679,65 @@ void tipc_bearer_stop(struct net *net)
 	}
 }
 
+void tipc_clone_to_loopback(struct net *net, struct sk_buff_head *pkts)
+{
+	struct net_device *dev = net->loopback_dev;
+	struct sk_buff *skb, *_skb;
+	int exp;
+
+	skb_queue_walk(pkts, _skb) {
+		skb = pskb_copy(_skb, GFP_ATOMIC);
+		if (!skb)
+			continue;
+
+		exp = SKB_DATA_ALIGN(dev->hard_header_len - skb_headroom(skb));
+		if (exp > 0 && pskb_expand_head(skb, exp, 0, GFP_ATOMIC)) {
+			kfree_skb(skb);
+			continue;
+		}
+
+		skb_reset_network_header(skb);
+		dev_hard_header(skb, dev, ETH_P_TIPC, dev->dev_addr,
+				dev->dev_addr, skb->len);
+		skb->dev = dev;
+		skb->pkt_type = PACKET_HOST;
+		skb->ip_summed = CHECKSUM_UNNECESSARY;
+		skb->protocol = eth_type_trans(skb, dev);
+		netif_rx_ni(skb);
+	}
+}
+
+static int tipc_loopback_rcv_pkt(struct sk_buff *skb, struct net_device *dev,
+				 struct packet_type *pt, struct net_device *od)
+{
+	consume_skb(skb);
+	return NET_RX_SUCCESS;
+}
+
+int tipc_attach_loopback(struct net *net)
+{
+	struct net_device *dev = net->loopback_dev;
+	struct tipc_net *tn = tipc_net(net);
+
+	if (!dev)
+		return -ENODEV;
+
+	dev_hold(dev);
+	tn->loopback_pt.dev = dev;
+	tn->loopback_pt.type = htons(ETH_P_TIPC);
+	tn->loopback_pt.func = tipc_loopback_rcv_pkt;
+	dev_add_pack(&tn->loopback_pt);
+	return 0;
+}
+
+void tipc_detach_loopback(struct net *net)
+{
+	struct tipc_net *tn = tipc_net(net);
+
+	dev_remove_pack(&tn->loopback_pt);
+	dev_put(net->loopback_dev);
+}
+
 /* Caller should hold rtnl_lock to protect the bearer */
 static int __tipc_nl_add_bearer(struct tipc_nl_msg *msg,
 				struct tipc_bearer *bearer, int nlflags)
diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
index 7f4c569..ea0f3c4 100644
--- a/net/tipc/bearer.h
+++ b/net/tipc/bearer.h
@@ -232,6 +232,16 @@ void tipc_bearer_xmit(struct net *net, u32 bearer_id,
 		      struct tipc_media_addr *dst);
 void tipc_bearer_bc_xmit(struct net *net, u32 bearer_id,
 			 struct sk_buff_head *xmitq);
+void tipc_clone_to_loopback(struct net *net, struct sk_buff_head *pkts);
+int tipc_attach_loopback(struct net *net);
+void tipc_detach_loopback(struct net *net);
+
+static inline void tipc_loopback_trace(struct net *net,
+				       struct sk_buff_head *pkts)
+{
+	if (unlikely(dev_nit_active(net->loopback_dev)))
+		tipc_clone_to_loopback(net, pkts);
+}
 
 /* check if device MTU is too low for tipc headers */
 static inline bool tipc_mtu_bad(struct net_device *dev, unsigned int reserve)
diff --git a/net/tipc/core.c b/net/tipc/core.c
index c837072..23cb379 100644
--- a/net/tipc/core.c
+++ b/net/tipc/core.c
@@ -82,6 +82,10 @@ static int __net_init tipc_init_net(struct net *net)
 	if (err)
 		goto out_bclink;
 
+	err = tipc_attach_loopback(net);
+	if (err)
+		goto out_bclink;
+
 	return 0;
 
 out_bclink:
@@ -94,6 +98,7 @@ static int __net_init tipc_init_net(struct net *net)
 
 static void __net_exit tipc_exit_net(struct net *net)
 {
+	tipc_detach_loopback(net);
 	tipc_net_stop(net);
 	tipc_bcast_stop(net);
 	tipc_nametbl_stop(net);
diff --git a/net/tipc/core.h b/net/tipc/core.h
index 7a68e1b..60d8295 100644
--- a/net/tipc/core.h
+++ b/net/tipc/core.h
@@ -125,6 +125,9 @@ struct tipc_net {
 
 	/* Cluster capabilities */
 	u16 capabilities;
+
+	/* Tracing of node internal messages */
+	struct packet_type loopback_pt;
 };
 
 static inline struct tipc_net *tipc_net(struct net *net)
diff --git a/net/tipc/node.c b/net/tipc/node.c
index 550581d..16d251b 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -1443,6 +1443,7 @@ int tipc_node_xmit(struct net *net, struct sk_buff_head *list,
 	int rc;
 
 	if (in_own_node(net, dnode)) {
+		tipc_loopback_trace(net, list);
 		tipc_sk_rcv(net, list);
 		return 0;
 	}
diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index f345662..e3a6ba1 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -40,6 +40,7 @@
 #include "socket.h"
 #include "addr.h"
 #include "msg.h"
+#include "bearer.h"
 #include <net/sock.h>
 #include <linux/module.h>
 
@@ -608,6 +609,7 @@ static void tipc_topsrv_kern_evt(struct net *net, struct tipc_event *evt)
 	memcpy(msg_data(buf_msg(skb)), evt, sizeof(*evt));
 	skb_queue_head_init(&evtq);
 	__skb_queue_tail(&evtq, skb);
+	tipc_loopback_trace(net, &evtq);
 	tipc_sk_rcv(net, &evtq);
 }
 
-- 
2.11.0

Re: [tipc-discussion] Slowness forming TIPC cluster with explicit node addresses

[Jon M. <jon...@er...>]

> -----Original Message-----
> From: net...@vg... <net...@vg...> On
> Behalf Of Chris Packham
> Sent: 2-Aug-19 01:11
> To: Jon Maloy <jon...@er...>; tipc-
> dis...@li...
> Cc: ne...@vg...; lin...@vg...
> Subject: Re: Slowness forming TIPC cluster with explicit node addresses
> 
> On Mon, 2019-07-29 at 09:04 +1200, Chris Packham wrote:
> > On Fri, 2019-07-26 at 13:31 +0000, Jon Maloy wrote:
> > >
> > >
> > > >
> > > >
> > > > -----Original Message-----
> > > > From: net...@vg... <netdev-
> ow...@vg...>
> > > > On Behalf Of Chris Packham
> > > > Sent: 25-Jul-19 19:37
> > > > To: tip...@li...
> > > > Cc: ne...@vg...; lin...@vg...
> > > > Subject: Slowness forming TIPC cluster with explicit node
> > > > addresses
> > > >
> > > > Hi,
> > > >
> > > > I'm having problems forming a TIPC cluster between 2 nodes.
> > > >
> > > > This is the basic steps I'm going through on each node.
> > > >
> > > > modprobe tipc
> > > > ip link set eth2 up
> > > > tipc node set addr 1.1.5 # or 1.1.6 tipc bearer enable media eth
> > > > dev eth0
> > > eth2, I assume...
> > >
> > Yes sorry I keep switching between between Ethernet ports for testing
> > so I hand edited the email.
> >
> > >
> > > >
> > > >
> > > >
> > > > Then to confirm if the cluster is formed I use tipc link list
> > > >
> > > > [root@node-5 ~]# tipc link list
> > > > broadcast-link: up
> > > > ...
> > > >
> > > > Looking at tcpdump the two nodes are sending packets
> > > >
> > > > 22:30:05.782320 TIPC v2.0 1.1.5 > 0.0.0, headerlength 60 bytes,
> > > > MessageSize
> > > > 76 bytes, Neighbor Detection Protocol internal, messageType Link
> > > > request
> > > > 22:30:05.863555 TIPC v2.0 1.1.6 > 0.0.0, headerlength 60 bytes,
> > > > MessageSize
> > > > 76 bytes, Neighbor Detection Protocol internal, messageType Link
> > > > request
> > > >
> > > > Eventually (after a few minutes) the link does come up
> > > >
> > > > [root@node-6 ~]# tipc link list
> > > > broadcast-link: up
> > > > 1001006:eth2-1001005:eth2: up
> > > >
> > > > [root@node-5 ~]# tipc link list
> > > > broadcast-link: up
> > > > 1001005:eth2-1001006:eth2: up
> > > >
> > > > When I remove the "tipc node set addr" things seem to kick into
> > > > life straight away
> > > >
> > > > [root@node-5 ~]# tipc link list
> > > > broadcast-link: up
> > > > 0050b61bd2aa:eth2-0050b61e6dfa:eth2: up
> > > >
> > > > So there appears to be some difference in behaviour between having
> > > > an explicit node address and using the default. Unfortunately our
> > > > application relies on setting the node addresses.
> > > I do this many times a day, without any problems. If there would be
> > > any time difference, I would expect the 'auto configurable' version
> > > to be slower, because it involves a DAD step.
> > > Are you sure you don't have any other nodes running in your system?
> > >
> > > ///jon
> > >
> > Nope the two nodes are connected back to back. Does the number of
> > Ethernet interfaces make a difference? As you can see I've got 3 on
> > each node. One is completely disconnected, one is for booting over
> > TFTP
> >  (only used by U-boot) and the other is the USB Ethernet I'm using for
> > testing.
> >
> 
> So I can still reproduce this on nodes that only have one network interface and
> are the only things connected.
> 
> I did find one thing that helps
> 
> diff --git a/net/tipc/discover.c b/net/tipc/discover.c index
> c138d68e8a69..49921dad404a 100644
> --- a/net/tipc/discover.c
> +++ b/net/tipc/discover.c
> @@ -358,10 +358,10 @@ int tipc_disc_create(struct net *net, struct
> tipc_bearer *b,
>         tipc_disc_init_msg(net, d->skb, DSC_REQ_MSG, b);
> 
>         /* Do we need an address trial period first ? */
> -       if (!tipc_own_addr(net)) {
> +//     if (!tipc_own_addr(net)) {
>                 tn->addr_trial_end = jiffies + msecs_to_jiffies(1000);
>                 msg_set_type(buf_msg(d->skb), DSC_TRIAL_MSG);
> -       }
> +//     }
>         memcpy(&d->dest, dest, sizeof(*dest));
>         d->net = net;
>         d->bearer_id = b->identity;
> 
> I think because with pre-configured addresses the duplicate address detection
> is skipped the shorter init phase is skipped. Would is make sense to
> unconditionally do the trial step? Or is there some better way to get things to
> transition with pre-assigned addresses.

I am on vacation until the end of next-week, so I can't give you any good analysis right now.
To do the trial step doesn’t make much sense to me, -it would only delay the setup unnecessarily (but with only 1 second).
Can you check the initial value of addr_trial_end when there a pre-configured address?

///jon

[tipc-discussion] [PATCH 2/3] tipc: fix memory leak issue

[Ying X. <yin...@wi...>]

syzbot found the following memory leak issue:

[   72.286706][ T7064] kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
BUG: memory leak
unreferenced object 0xffff888122bca200 (size 128):
  comm "syz-executor232", pid 7065, jiffies 4294943817 (age 8.880s)
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 18 a2 bc 22 81 88 ff ff  ..........."....
  backtrace:
    [<000000005bada299>] kmem_cache_alloc_trace+0x145/0x2c0
    [<00000000e7bcdc9f>] tipc_group_create_member+0x3c/0x190
    [<0000000005f56f40>] tipc_group_add_member+0x34/0x40
    [<0000000044406683>] tipc_nametbl_build_group+0x9b/0xf0
    [<000000009f71e803>] tipc_setsockopt+0x170/0x490
    [<000000007f61cbc2>] __sys_setsockopt+0x10f/0x220
    [<00000000cc630372>] __x64_sys_setsockopt+0x26/0x30
    [<00000000ec30be33>] do_syscall_64+0x76/0x1a0
    [<00000000271be3e6>] entry_SYSCALL_64_after_hwframe+0x44/0xa9

Reported-by: syz...@sy...
Signed-off-by: Hillf Danton <hd...@si...>
Signed-off-by: Ying Xue <yin...@wi...>
---
 net/tipc/group.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/net/tipc/group.c b/net/tipc/group.c
index 5f98d38..cbc540a 100644
--- a/net/tipc/group.c
+++ b/net/tipc/group.c
@@ -273,8 +273,8 @@ static struct tipc_member *tipc_group_find_node(struct tipc_group *grp,
 	return NULL;
 }
 
-static void tipc_group_add_to_tree(struct tipc_group *grp,
-				   struct tipc_member *m)
+struct tipc_member *tipc_group_add_to_tree(struct tipc_group *grp,
+					   struct tipc_member *m)
 {
 	u64 nkey, key = (u64)m->node << 32 | m->port;
 	struct rb_node **n, *parent = NULL;
@@ -282,7 +282,6 @@ static void tipc_group_add_to_tree(struct tipc_group *grp,
 
 	n = &grp->members.rb_node;
 	while (*n) {
-		tmp = container_of(*n, struct tipc_member, tree_node);
 		parent = *n;
 		tmp = container_of(parent, struct tipc_member, tree_node);
 		nkey = (u64)tmp->node << 32 | tmp->port;
@@ -291,17 +290,18 @@ static void tipc_group_add_to_tree(struct tipc_group *grp,
 		else if (key > nkey)
 			n = &(*n)->rb_right;
 		else
-			return;
+			return tmp;
 	}
 	rb_link_node(&m->tree_node, parent, n);
 	rb_insert_color(&m->tree_node, &grp->members);
+	return m;
 }
 
 static struct tipc_member *tipc_group_create_member(struct tipc_group *grp,
 						    u32 node, u32 port,
 						    u32 instance, int state)
 {
-	struct tipc_member *m;
+	struct tipc_member *m, *n;
 
 	m = kzalloc(sizeof(*m), GFP_ATOMIC);
 	if (!m)
@@ -315,10 +315,14 @@ static struct tipc_member *tipc_group_create_member(struct tipc_group *grp,
 	m->instance = instance;
 	m->bc_acked = grp->bc_snd_nxt - 1;
 	grp->member_cnt++;
-	tipc_group_add_to_tree(grp, m);
-	tipc_nlist_add(&grp->dests, m->node);
-	m->state = state;
-	return m;
+	n = tipc_group_add_to_tree(grp, m);
+	if (n == m) {
+		tipc_nlist_add(&grp->dests, m->node);
+		m->state = state;
+	} else {
+		kfree(m);
+	}
+	return n;
 }
 
 void tipc_group_add_member(struct tipc_group *grp, u32 node,
-- 
2.7.4

[tipc-discussion] [PATCH 1/3] tipc: fix memory leak issue

[Ying X. <yin...@wi...>]

syzbot found the following memory leak:

[   68.602482][ T7130] kmemleak: 2 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
BUG: memory leak
unreferenced object 0xffff88810df83c00 (size 512):
  comm "softirq", pid 0, jiffies 4294942354 (age 19.830s)
  hex dump (first 32 bytes):
    38 1a 0d 0f 81 88 ff ff 38 1a 0d 0f 81 88 ff ff  8.......8.......
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<000000009375ee42>] kmem_cache_alloc_node+0x153/0x2a0
    [<000000004c563922>] __alloc_skb+0x6e/0x210
    [<00000000ec87bfa1>] tipc_buf_acquire+0x2f/0x80
    [<00000000d151ef84>] tipc_msg_create+0x37/0xe0
    [<000000008bb437b0>] tipc_group_create_event+0xb3/0x1b0
    [<00000000947b1d0f>] tipc_group_proto_rcv+0x569/0x640
    [<00000000b75ab039>] tipc_sk_filter_rcv+0x9ac/0xf20
    [<000000000dab7a6c>] tipc_sk_rcv+0x494/0x8a0
    [<00000000023a7ddd>] tipc_node_xmit+0x196/0x1f0
    [<00000000337dd9eb>] tipc_node_distr_xmit+0x7d/0x120
    [<00000000b6375182>] tipc_group_delete+0xe6/0x130
    [<000000000361ba2b>] tipc_sk_leave+0x57/0xb0
    [<000000009df90505>] tipc_release+0x7b/0x5e0
    [<000000009f3189da>] __sock_release+0x4b/0xe0
    [<00000000d3568ee0>] sock_close+0x1b/0x30
    [<00000000266a6215>] __fput+0xed/0x300

Reported-by: syz...@sy...
Signed-off-by: Hillf Danton <hd...@si...>
Signed-off-by: Ying Xue <yin...@wi...>
---
 net/tipc/node.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index 7ca0190..d1852fc 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -1469,10 +1469,13 @@ int tipc_node_xmit(struct net *net, struct sk_buff_head *list,
 	spin_unlock_bh(&le->lock);
 	tipc_node_read_unlock(n);
 
-	if (unlikely(rc == -ENOBUFS))
+	if (unlikely(rc == -ENOBUFS)) {
 		tipc_node_link_down(n, bearer_id, false);
-	else
+		skb_queue_purge(list);
+		skb_queue_purge(&xmitq);
+	} else {
 		tipc_bearer_xmit(net, bearer_id, &xmitq, &le->maddr);
+	}
 
 	tipc_node_put(n);
 
-- 
2.7.4

[tipc-discussion] [PATCH 3/3] tipc: fix issue of calling smp_processor_id() in preemptible

[Ying X. <yin...@wi...>]

syzbot found the following issue:

[   81.119772][ T8612] BUG: using smp_processor_id() in preemptible [00000000] code: syz-executor834/8612
[   81.136212][ T8612] caller is dst_cache_get+0x3d/0xb0
[   81.141450][ T8612] CPU: 0 PID: 8612 Comm: syz-executor834 Not tainted 5.2.0-rc6+ #48
[   81.149435][ T8612] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
[   81.159480][ T8612] Call Trace:
[   81.162789][ T8612]  dump_stack+0x172/0x1f0
[   81.167123][ T8612]  debug_smp_processor_id+0x251/0x280
[   81.172479][ T8612]  dst_cache_get+0x3d/0xb0
[   81.176928][ T8612]  tipc_udp_xmit.isra.0+0xc4/0xb80
[   81.182046][ T8612]  ? kasan_kmalloc+0x9/0x10
[   81.186531][ T8612]  ? tipc_udp_addr2str+0x170/0x170
[   81.191641][ T8612]  ? __copy_skb_header+0x2e8/0x560
[   81.196750][ T8612]  ? __skb_checksum_complete+0x3f0/0x3f0
[   81.202364][ T8612]  ? netdev_alloc_frag+0x1b0/0x1b0
[   81.207452][ T8612]  ? skb_copy_header+0x21/0x2b0
[   81.212282][ T8612]  ? __pskb_copy_fclone+0x516/0xc90
[   81.217470][ T8612]  tipc_udp_send_msg+0x29a/0x4b0
[   81.222400][ T8612]  tipc_bearer_xmit_skb+0x16c/0x360
[   81.227585][ T8612]  tipc_enable_bearer+0xabe/0xd20
[   81.232606][ T8612]  ? __nla_validate_parse+0x2d0/0x1ee0
[   81.238048][ T8612]  ? tipc_bearer_xmit_skb+0x360/0x360
[   81.243401][ T8612]  ? nla_memcpy+0xb0/0xb0
[   81.247710][ T8612]  ? nla_memcpy+0xb0/0xb0
[   81.252020][ T8612]  ? __nla_parse+0x43/0x60
[   81.256417][ T8612]  __tipc_nl_bearer_enable+0x2de/0x3a0
[   81.261856][ T8612]  ? __tipc_nl_bearer_enable+0x2de/0x3a0
[   81.267467][ T8612]  ? tipc_nl_bearer_disable+0x40/0x40
[   81.272848][ T8612]  ? unwind_get_return_address+0x58/0xa0
[   81.278501][ T8612]  ? lock_acquire+0x16f/0x3f0
[   81.283190][ T8612]  tipc_nl_bearer_enable+0x23/0x40
[   81.288300][ T8612]  genl_family_rcv_msg+0x74b/0xf90
[   81.293404][ T8612]  ? genl_unregister_family+0x790/0x790
[   81.298935][ T8612]  ? __lock_acquire+0x54f/0x5490
[   81.303852][ T8612]  ? __netlink_lookup+0x3fa/0x7b0
[   81.308865][ T8612]  genl_rcv_msg+0xca/0x16c
[   81.313266][ T8612]  netlink_rcv_skb+0x177/0x450
[   81.318043][ T8612]  ? genl_family_rcv_msg+0xf90/0xf90
[   81.323311][ T8612]  ? netlink_ack+0xb50/0xb50
[   81.327906][ T8612]  ? lock_acquire+0x16f/0x3f0
[   81.332589][ T8612]  ? kasan_check_write+0x14/0x20
[   81.337511][ T8612]  genl_rcv+0x29/0x40
[   81.341485][ T8612]  netlink_unicast+0x531/0x710
[   81.346268][ T8612]  ? netlink_attachskb+0x770/0x770
[   81.351374][ T8612]  ? _copy_from_iter_full+0x25d/0x8c0
[   81.356765][ T8612]  ? __sanitizer_cov_trace_cmp8+0x18/0x20
[   81.362479][ T8612]  ? __check_object_size+0x3d/0x42f
[   81.367667][ T8612]  netlink_sendmsg+0x8ae/0xd70
[   81.372415][ T8612]  ? netlink_unicast+0x710/0x710
[   81.377520][ T8612]  ? aa_sock_msg_perm.isra.0+0xba/0x170
[   81.383051][ T8612]  ? apparmor_socket_sendmsg+0x2a/0x30
[   81.388530][ T8612]  ? __sanitizer_cov_trace_const_cmp4+0x16/0x20
[   81.394775][ T8612]  ? security_socket_sendmsg+0x8d/0xc0
[   81.400240][ T8612]  ? netlink_unicast+0x710/0x710
[   81.405161][ T8612]  sock_sendmsg+0xd7/0x130
[   81.409561][ T8612]  ___sys_sendmsg+0x803/0x920
[   81.414220][ T8612]  ? copy_msghdr_from_user+0x430/0x430
[   81.419667][ T8612]  ? _raw_spin_unlock_irqrestore+0x6b/0xe0
[   81.425461][ T8612]  ? debug_object_active_state+0x25d/0x380
[   81.431255][ T8612]  ? __lock_acquire+0x54f/0x5490
[   81.436174][ T8612]  ? kasan_check_read+0x11/0x20
[   81.441208][ T8612]  ? _raw_spin_unlock_irqrestore+0xa4/0xe0
[   81.447008][ T8612]  ? mark_held_locks+0xf0/0xf0
[   81.451768][ T8612]  ? __call_rcu.constprop.0+0x28b/0x720
[   81.457298][ T8612]  ? call_rcu+0xb/0x10
[   81.461353][ T8612]  ? __sanitizer_cov_trace_const_cmp4+0x16/0x20
[   81.467589][ T8612]  ? __fget_light+0x1a9/0x230
[   81.472249][ T8612]  ? __fdget+0x1b/0x20
[   81.476301][ T8612]  ? __sanitizer_cov_trace_const_cmp8+0x18/0x20
[   81.482545][ T8612]  __sys_sendmsg+0x105/0x1d0
[   81.487115][ T8612]  ? __ia32_sys_shutdown+0x80/0x80
[   81.492208][ T8612]  ? blkcg_maybe_throttle_current+0x5e2/0xfb0
[   81.498272][ T8612]  ? trace_hardirqs_on_thunk+0x1a/0x1c
[   81.503726][ T8612]  ? do_syscall_64+0x26/0x680
[   81.508385][ T8612]  ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
[   81.514444][ T8612]  ? do_syscall_64+0x26/0x680
[   81.519110][ T8612]  __x64_sys_sendmsg+0x78/0xb0
[   81.523862][ T8612]  do_syscall_64+0xfd/0x680
[   81.528352][ T8612]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[   81.534234][ T8612] RIP: 0033:0x444679
[   81.538114][ T8612] Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 1b d8 fb ff c3 66 2e 0f 1f 84 00 00 00 00
[   81.557709][ T8612] RSP: 002b:00007fff0201a8b8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[   81.566147][ T8612] RAX: ffffffffffffffda RBX: 00000000004002e0 RCX: 0000000000444679
[   81.574108][ T8612] RDX: 0000000000000000 RSI: 0000000020000580 RDI: 0000000000000003
[   81.582152][ T8612] RBP: 00000000006cf018 R08: 0000000000000001 R09: 00000000004002e0
[   81.590113][ T8612] R10: 0000000000000008 R11: 0000000000000246 R12: 0000000000402320
[   81.598089][ T8612] R13: 00000000004023b0 R14: 0000000000000000 R15: 0000000000

In commit e9c1a793210f ("tipc: add dst_cache support for udp media")
dst_cache_get() was introduced to be called in tipc_udp_xmit(). But
smp_processor_id() called by dst_cache_get() cannot be invoked in
preemptible context, as a result, the complaint above was reported.

Fixes: e9c1a793210f ("tipc: add dst_cache support for udp media")
syz...@sy...
Signed-off-by: Hillf Danton <hd...@si...>
Signed-off-by: Ying Xue <yin...@wi...>
---
 net/tipc/udp_media.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index 287df687..ca3ae2e 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c
@@ -224,6 +224,8 @@ static int tipc_udp_send_msg(struct net *net, struct sk_buff *skb,
 	struct udp_bearer *ub;
 	int err = 0;
 
+	local_bh_disable();
+
 	if (skb_headroom(skb) < UDP_MIN_HEADROOM) {
 		err = pskb_expand_head(skb, UDP_MIN_HEADROOM, 0, GFP_ATOMIC);
 		if (err)
@@ -237,9 +239,12 @@ static int tipc_udp_send_msg(struct net *net, struct sk_buff *skb,
 		goto out;
 	}
 
-	if (addr->broadcast != TIPC_REPLICAST_SUPPORT)
-		return tipc_udp_xmit(net, skb, ub, src, dst,
-				     &ub->rcast.dst_cache);
+	if (addr->broadcast != TIPC_REPLICAST_SUPPORT) {
+		err = tipc_udp_xmit(net, skb, ub, src, dst,
+				    &ub->rcast.dst_cache);
+		local_bh_enable();
+		return err;
+	}
 
 	/* Replicast, send an skb to each configured IP address */
 	list_for_each_entry_rcu(rcast, &ub->rcast.list, list) {
@@ -259,6 +264,7 @@ static int tipc_udp_send_msg(struct net *net, struct sk_buff *skb,
 	err = 0;
 out:
 	kfree_skb(skb);
+	local_bh_enable();
 	return err;
 }
 
-- 
2.7.4