tipc-discussion

[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

[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 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

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 2/3] tipc: fix wrong socket reference counter after tipc_sk_timeout() returns

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

Acked-by: Jon

> -----Original Message-----
> From: Tung Nguyen <tun...@de...>
> Sent: 13-Aug-19 06:02
> To: tip...@li...; Jon Maloy
> <jon...@er...>; ma...@do...; yin...@wi...
> Subject: [tipc-discussion][net v1 2/3] tipc: fix wrong socket reference counter
> after tipc_sk_timeout() returns
> 
> 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

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);
> 

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

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

Acked-by: Jon

> -----Original Message-----
> From: Ying Xue <yin...@wi...>
> Sent: 14-Aug-19 07:41
> To: Tung Quang Nguyen <tun...@de...>; tipc-
> dis...@li...; Jon Maloy <jon...@er...>;
> ma...@do...
> Subject: Re: [tipc-discussion][net v1 1/3] tipc: fix potential memory leak in
> __tipc_sendmsg()
> 
> 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 0/4] Fix some bugs at socket layer

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

This series fixes some bugs at socket layer.

Tung Nguyen (4):
  tipc: fix potential memory leak in __tipc_sendmsg()
  tipc: fix wrong socket reference counter after tipc_sk_timeout()
    returns
  tipc: fix wrong timeout input for tipc_wait_for_cond()
  tipc: fix duplicate SYN messages under link congestion

 net/tipc/socket.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

-- 
2.17.1

[tipc-discussion] [net v1 4/4] tipc: fix duplicate SYN messages under link congestion

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

Scenario:
1. A client socket initiates a SYN message to a listening socket.
2. The send link is congested, the SYN message is put in the
send link and a wakeup message is put in wakeup queue.
3. The congestion situation is abated, the wakeup message is
pulled out of the wakeup queue. Function tipc_sk_push_backlog()
is called to send out delayed messages by Nagle. However,
the client socket is still in CONNECTING state. So, it sends
the SYN message in the socket write queue to the listening socket
again.
4. The listening socket receives the first SYN message and creates
first server socket. The client socket receives ACK- and establishes
a connection to the first server socket. The client socket closes
its connection with the first server socket.
5. The listening socket receives the second SYN message and creates
second server socket. The second server socket sends ACK- to the
client socket, but it has been closed. It results in connection
reset error when reading from the server socket in user space.

Solution: return from function tipc_sk_push_backlog() immediately
if there is pending SYN message in the socket write queue.

Fixes: c0bceb97db9e ("tipc: add smart nagle feature")
Signed-off-by: Tung Nguyen <tun...@de...>
Acked-by: Jon Maloy <jon...@er...>
---
 net/tipc/socket.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index da5fb84852a6..41688da233ab 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -540,12 +540,10 @@ static void __tipc_shutdown(struct socket *sock, int error)
 	tipc_wait_for_cond(sock, &timeout, (!tsk->cong_link_cnt &&
 					    !tsk_conn_cong(tsk)));
 
-	/* Push out unsent messages or remove if pending SYN */
-	skb = skb_peek(&sk->sk_write_queue);
-	if (skb && !msg_is_syn(buf_msg(skb)))
-		tipc_sk_push_backlog(tsk);
-	else
-		__skb_queue_purge(&sk->sk_write_queue);
+	/* Push out delayed messages if in Nagle mode */
+	tipc_sk_push_backlog(tsk);
+	/* Remove pending SYN */
+	__skb_queue_purge(&sk->sk_write_queue);
 
 	/* Reject all unreceived messages, except on an active connection
 	 * (which disconnects locally & sends a 'FIN+' to peer).
@@ -1248,9 +1246,14 @@ static void tipc_sk_push_backlog(struct tipc_sock *tsk)
 	struct sk_buff_head *txq = &tsk->sk.sk_write_queue;
 	struct net *net = sock_net(&tsk->sk);
 	u32 dnode = tsk_peer_node(tsk);
+	struct sk_buff *skb = skb_peek(txq);
 	int rc;
 
-	if (skb_queue_empty(txq) || tsk->cong_link_cnt)
+	if (!skb || tsk->cong_link_cnt)
+		return;
+
+	/* Do not send SYN again after congestion */
+	if (msg_is_syn(buf_msg(skb)))
 		return;
 
 	tsk->snt_unacked += tsk->snd_backlog;
-- 
2.17.1

[tipc-discussion] [net v1 3/4] tipc: fix wrong timeout input for tipc_wait_for_cond()

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

In function __tipc_shutdown(), the timeout value passed to
tipc_wait_for_cond() is not jiffies.

This commit fixes it by converting that value from milliseconds
to jiffies.

Fixes: 365ad353c256 ("tipc: reduce risk of user starvation during link congestion")
Signed-off-by: Tung Nguyen <tun...@de...>
Acked-by: Jon Maloy <jon...@er...>
---
 net/tipc/socket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index fb5595081a05..da5fb84852a6 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -532,7 +532,7 @@ static void __tipc_shutdown(struct socket *sock, int error)
 	struct sock *sk = sock->sk;
 	struct tipc_sock *tsk = tipc_sk(sk);
 	struct net *net = sock_net(sk);
-	long timeout = CONN_TIMEOUT_DEFAULT;
+	long timeout = msecs_to_jiffies(CONN_TIMEOUT_DEFAULT);
 	u32 dnode = tsk_peer_node(tsk);
 	struct sk_buff *skb;
 
-- 
2.17.1

[tipc-discussion] [net v1 1/4] 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...>
Acked-by: Ying Xue <yin...@wi...>
Acked-by: Jon Maloy <jon...@er...>
---
 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 a1c8d722ca20..7baed2c2c93d 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1447,8 +1447,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

[tipc-discussion] [net v1 2/4] 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...>
Acked-by: Ying Xue <yin...@wi...>
Acked-by: Jon Maloy <jon...@er...>
---
 net/tipc/socket.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 7baed2c2c93d..fb5595081a05 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2759,6 +2759,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

Re: [tipc-discussion] [net v1 0/4] Fix some bugs at socket layer

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

From: Tung Nguyen <tun...@de...>
Date: Thu, 28 Nov 2019 10:10:04 +0700

> This series fixes some bugs at socket layer.

Series applied, thanks.