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

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

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

Guys,
After our discussion this morning regarding this problem I gave it some 
more thought.

What if we simply limit the search depth in the receive queue to some 
fix number, 10, 20, 50 or something and return NULL if nothing is found 
within this range. This would be a simple stack counter inside 
tipc_skb_dequeue(), and would cost almost nothing.

If you experiment with this, of course in combination with a max limit 
of some milliseconds as we also discussed, we might obtain acceptable 
results.

What do you think?

///jon


On 28/07/2021 04:04, Hoang Huu Le wrote:
> Hi Jon,
> 
> Let's enjoy your vacation.
> Our new team member (CCed) will take a look at it.
> 
> Regards,
> Hoang
>> -----Original Message-----
>> From: Jon Maloy <jm...@re...>
>> Sent: Wednesday, July 28, 2021 6:20 AM
>> To: tip...@li...; Tung Quang Nguyen <tun...@de...>; Hoang Huu Le
>> <hoa...@de...>; Xin Long <luc...@gm...>; Ying Xue <yin...@wi...>
>> Subject: Strange behavior in socket.c::tipc_sk_enqueue()
>>
>> I did by accident discover a strange behavior in the function
>> tipc_sk_enqueue:
>>
>>
>> static void tipc_sk_enqueue(struct sk_buff_head *inputq,
>>                               struct sock *sk, u32 dport,
>>                               struct sk_buff_head *xmitq)
>> {
>>           struct tipc_sock *tsk = tipc_sk(sk);
>>           unsigned long time_limit = jiffies + 2;
>>           struct sk_buff *skb;
>>           unsigned int lim;
>>           atomic_t *dcnt;
>>           u32 onode;
>>
>>           while (skb_queue_len(inputq)) {
>>           if (unlikely(time_after_eq(jiffies, time_limit)))
>>                 return;
>>           [...]
>>           }
>> }
>>
>> At the moment we call time_after_eq() the two jiffies often
>> have already passed, and the skb is not dequeued.
>> I noticed that tipc_sk_rcv() may call tipc_sk_enqueue()
>> with the same skb dozens of times before the buffer can
>> be delivered further upwards in the stack.
>>
>> Needless to say that this cannot be good for performance.
>>
>> I believe the value of 2 jiffies was hard coded at a time
>> when machines were slower, and a jiffie represented a much
>> longer time interval.
>>
>> Now it is clearly too short, and should be replaced with something
>> longer and more consisten, e.g. msec_to_jiffies(2).
>>
>> Can anybody look into this?
>>
>> Also, I will be on vacation the next two weeks, which means we
>> should cancel the bi-weekly meeting next Tuesday.
>>
>> ///jon
>>
> 

[tipc-discussion] [PATCH net] tipc: call tipc_wait_for_connect only when dlen is not 0

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

__tipc_sendmsg() is called to send SYN packet by either tipc_sendmsg()
or tipc_connect(). The difference is in tipc_connect(), it will call
tipc_wait_for_connect() after __tipc_sendmsg() to wait until connecting
is done. So there's no need to wait in __tipc_sendmsg() for this case.

This patch is to fix it by calling tipc_wait_for_connect() only when dlen
is not 0 in __tipc_sendmsg(), which means it's called by tipc_connect().

Note this also fixes the failure in tipcutils/test/ptts/:

  # ./tipcTS &
  # ./tipcTC 9
  (hang)

Fixes: 36239dab6da7 ("tipc: fix implicit-connect for SYN+")
Reported-by: Shuang Li <sh...@re...>
Signed-off-by: Xin Long <luc...@gm...>
Acked-by: Jon Maloy <jm...@re...>
---
 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 75b99b7eda22..8754bd885169 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1518,7 +1518,7 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 
 	if (unlikely(syn && !rc)) {
 		tipc_set_sk_state(sk, TIPC_CONNECTING);
-		if (timeout) {
+		if (dlen && timeout) {
 			timeout = msecs_to_jiffies(timeout);
 			tipc_wait_for_connect(sock, &timeout);
 		}
-- 
2.27.0

[tipc-discussion] [net] Revert "tipc: Return the correct errno code"

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

This reverts commit 0efea3c649f0 because of:
- The returning -ENOBUF error is fine on socket buffer allocation.
- There is side effect in the calling path
tipc_node_xmit()->tipc_link_xmit() when checking error code returning.

Fixes: 0efea3c649f0 ("tipc: Return the correct errno code")
Acked-by: Jon Maloy <jm...@re...>
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/link.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index cf586840caeb..1b7a487c8841 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -913,7 +913,7 @@ static int link_schedule_user(struct tipc_link *l, struct tipc_msg *hdr)
 	skb = tipc_msg_create(SOCK_WAKEUP, 0, INT_H_SIZE, 0,
 			      dnode, l->addr, dport, 0, 0);
 	if (!skb)
-		return -ENOMEM;
+		return -ENOBUFS;
 	msg_set_dest_droppable(buf_msg(skb), true);
 	TIPC_SKB_CB(skb)->chain_imp = msg_importance(hdr);
 	skb_queue_tail(&l->wakeupq, skb);
@@ -1031,7 +1031,7 @@ void tipc_link_reset(struct tipc_link *l)
  *
  * Consumes the buffer chain.
  * Messages at TIPC_SYSTEM_IMPORTANCE are always accepted
- * Return: 0 if success, or errno: -ELINKCONG, -EMSGSIZE or -ENOBUFS or -ENOMEM
+ * Return: 0 if success, or errno: -ELINKCONG, -EMSGSIZE or -ENOBUFS
  */
 int tipc_link_xmit(struct tipc_link *l, struct sk_buff_head *list,
 		   struct sk_buff_head *xmitq)
@@ -1089,7 +1089,7 @@ int tipc_link_xmit(struct tipc_link *l, struct sk_buff_head *list,
 			if (!_skb) {
 				kfree_skb(skb);
 				__skb_queue_purge(list);
-				return -ENOMEM;
+				return -ENOBUFS;
 			}
 			__skb_queue_tail(transmq, skb);
 			tipc_link_set_skb_retransmit_time(skb, l);
-- 
2.30.2

Re: [tipc-discussion] [PATCH net] tipc: call tipc_wait_for_connect only when dlen is not 0

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

On 03/08/2021 22:27, Xin Long wrote:
> __tipc_sendmsg() is called to send SYN packet by either tipc_sendmsg()
> or tipc_connect(). The difference is in tipc_connect(), it will call
> tipc_wait_for_connect() after __tipc_sendmsg() to wait until connecting
> is done. So there's no need to wait in __tipc_sendmsg() for this case.
> 
> This patch is to fix it by calling tipc_wait_for_connect() only when dlen
> is not 0 in __tipc_sendmsg(), which means it's called by tipc_connect().
> 
> Note this also fixes the failure in tipcutils/test/ptts/:
> 
>    # ./tipcTS &
>    # ./tipcTC 9
>    (hang)
> 
> Fixes: 36239dab6da7 ("tipc: fix implicit-connect for SYN+")
> Reported-by: Shuang Li <sh...@re...>
> Signed-off-by: Xin Long <luc...@gm...>
> ---
>   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 75b99b7eda22..8754bd885169 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -1518,7 +1518,7 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
>   
>   	if (unlikely(syn && !rc)) {
>   		tipc_set_sk_state(sk, TIPC_CONNECTING);
> -		if (timeout) {
> +		if (dlen && timeout) {
>   			timeout = msecs_to_jiffies(timeout);
>   			tipc_wait_for_connect(sock, &timeout);
>   		}
> 

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

Re: [tipc-discussion] [net] Revert "tipc: Return the correct errno code"

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

 

    On Tuesday, August 10, 2021, 05:33:55 AM EDT, Hoang Le <hoa...@de...> wrote:  
 
 This reverts commit 0efea3c649f0 because of:
- The returning -ENOBUF error is fine on socket buffer allocation.
- There is side effect in the calling path
tipc_node_xmit()->tipc_link_xmit() when checking error code returning.

Fixes: 0efea3c649f0 ("tipc: Return the correct errno code")
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/link.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index cf586840caeb..1b7a487c8841 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -913,7 +913,7 @@ static int link_schedule_user(struct tipc_link *l, struct tipc_msg *hdr)
     skb = tipc_msg_create(SOCK_WAKEUP, 0, INT_H_SIZE, 0,
                   dnode, l->addr, dport, 0, 0);
     if (!skb)
-        return -ENOMEM;
+        return -ENOBUFS;
     msg_set_dest_droppable(buf_msg(skb), true);
     TIPC_SKB_CB(skb)->chain_imp = msg_importance(hdr);
     skb_queue_tail(&l->wakeupq, skb);
@@ -1031,7 +1031,7 @@ void tipc_link_reset(struct tipc_link *l)
  *
  * Consumes the buffer chain.
  * Messages at TIPC_SYSTEM_IMPORTANCE are always accepted
- * Return: 0 if success, or errno: -ELINKCONG, -EMSGSIZE or -ENOBUFS or -ENOMEM
+ * Return: 0 if success, or errno: -ELINKCONG, -EMSGSIZE or -ENOBUFS
  */
 int tipc_link_xmit(struct tipc_link *l, struct sk_buff_head *list,
           struct sk_buff_head *xmitq)
@@ -1089,7 +1089,7 @@ int tipc_link_xmit(struct tipc_link *l, struct sk_buff_head *list,
             if (!_skb) {
                 kfree_skb(skb);
                 __skb_queue_purge(list);
-                return -ENOMEM;
+                return -ENOBUFS;
             }
             __skb_queue_tail(transmq, skb);
             tipc_link_set_skb_retransmit_time(skb, l);
-- 
2.30.2

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

[tipc-discussion] [net] Revert "tipc: Return the correct errno code"

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

This reverts commit 0efea3c649f0 because of:
- The returning -ENOBUF error is fine on socket buffer allocation.
- There is side effect in the calling path
tipc_node_xmit()->tipc_link_xmit() when checking error code returning.

Fixes: 0efea3c649f0 ("tipc: Return the correct errno code")
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/link.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index cf586840caeb..1b7a487c8841 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -913,7 +913,7 @@ static int link_schedule_user(struct tipc_link *l, struct tipc_msg *hdr)
 	skb = tipc_msg_create(SOCK_WAKEUP, 0, INT_H_SIZE, 0,
 			      dnode, l->addr, dport, 0, 0);
 	if (!skb)
-		return -ENOMEM;
+		return -ENOBUFS;
 	msg_set_dest_droppable(buf_msg(skb), true);
 	TIPC_SKB_CB(skb)->chain_imp = msg_importance(hdr);
 	skb_queue_tail(&l->wakeupq, skb);
@@ -1031,7 +1031,7 @@ void tipc_link_reset(struct tipc_link *l)
  *
  * Consumes the buffer chain.
  * Messages at TIPC_SYSTEM_IMPORTANCE are always accepted
- * Return: 0 if success, or errno: -ELINKCONG, -EMSGSIZE or -ENOBUFS or -ENOMEM
+ * Return: 0 if success, or errno: -ELINKCONG, -EMSGSIZE or -ENOBUFS
  */
 int tipc_link_xmit(struct tipc_link *l, struct sk_buff_head *list,
 		   struct sk_buff_head *xmitq)
@@ -1089,7 +1089,7 @@ int tipc_link_xmit(struct tipc_link *l, struct sk_buff_head *list,
 			if (!_skb) {
 				kfree_skb(skb);
 				__skb_queue_purge(list);
-				return -ENOMEM;
+				return -ENOBUFS;
 			}
 			__skb_queue_tail(transmq, skb);
 			tipc_link_set_skb_retransmit_time(skb, l);
-- 
2.30.2

[tipc-discussion] [PATCH net] tipc: call tipc_wait_for_connect only when dlen is not 0

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

__tipc_sendmsg() is called to send SYN packet by either tipc_sendmsg()
or tipc_connect(). The difference is in tipc_connect(), it will call
tipc_wait_for_connect() after __tipc_sendmsg() to wait until connecting
is done. So there's no need to wait in __tipc_sendmsg() for this case.

This patch is to fix it by calling tipc_wait_for_connect() only when dlen
is not 0 in __tipc_sendmsg(), which means it's called by tipc_connect().

Note this also fixes the failure in tipcutils/test/ptts/:

  # ./tipcTS &
  # ./tipcTC 9
  (hang)

Fixes: 36239dab6da7 ("tipc: fix implicit-connect for SYN+")
Reported-by: Shuang Li <sh...@re...>
Signed-off-by: Xin Long <luc...@gm...>
---
 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 75b99b7eda22..8754bd885169 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1518,7 +1518,7 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 
 	if (unlikely(syn && !rc)) {
 		tipc_set_sk_state(sk, TIPC_CONNECTING);
-		if (timeout) {
+		if (dlen && timeout) {
 			timeout = msecs_to_jiffies(timeout);
 			tipc_wait_for_connect(sock, &timeout);
 		}
-- 
2.27.0

Re: [tipc-discussion] [PATCH net-next] tipc: Return the correct errno code

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

Hi Zheng,

The patch was being merged by accident. Will have you planning to revert it?
We need to do ASAP since calling path tipc_node_xmit() -> tipc_link_xmit() broken as side effect.

Thanks,
hoang
> -----Original Message-----
> From: zhengyongjun <zhe...@hu...>
> Sent: Friday, June 4, 2021 8:35 AM
> To: jm...@re...; yin...@wi...; da...@da...; ku...@ke...; ne...@vg...; tipc-
> dis...@li...; lin...@vg...
> Subject: 答复: [PATCH net-next] tipc: Return the correct errno code
> 
> Sorry, this patch is wrong, please ignore it, thanks :)
> 
> -----邮件原件-----
> 发件人: zhengyongjun
> 发送时间: 2021年6月4日 9:47
> 收件人: jm...@re...; yin...@wi...; da...@da...; ku...@ke...; ne...@vg...; tipc-
> dis...@li...; lin...@vg...
> 抄送: zhengyongjun <zhe...@hu...>
> 主题: [PATCH net-next] tipc: Return the correct errno code
> 
> When kalloc or kmemdup failed, should return ENOMEM rather than ENOBUF.
> 
> Signed-off-by: Zheng Yongjun <zhe...@hu...>
> ---
>  net/tipc/link.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/tipc/link.c b/net/tipc/link.c index c44b4bfaaee6..5b6181277cc5 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -912,7 +912,7 @@ static int link_schedule_user(struct tipc_link *l, struct tipc_msg *hdr)
>  	skb = tipc_msg_create(SOCK_WAKEUP, 0, INT_H_SIZE, 0,
>  			      dnode, l->addr, dport, 0, 0);
>  	if (!skb)
> -		return -ENOBUFS;
> +		return -ENOMEM;
>  	msg_set_dest_droppable(buf_msg(skb), true);
>  	TIPC_SKB_CB(skb)->chain_imp = msg_importance(hdr);
>  	skb_queue_tail(&l->wakeupq, skb);
> @@ -1030,7 +1030,7 @@ void tipc_link_reset(struct tipc_link *l)
>   *
>   * Consumes the buffer chain.
>   * Messages at TIPC_SYSTEM_IMPORTANCE are always accepted
> - * Return: 0 if success, or errno: -ELINKCONG, -EMSGSIZE or -ENOBUFS
> + * Return: 0 if success, or errno: -ELINKCONG, -EMSGSIZE or -ENOBUFS or
> + -ENOMEM
>   */
>  int tipc_link_xmit(struct tipc_link *l, struct sk_buff_head *list,
>  		   struct sk_buff_head *xmitq)
> @@ -1088,7 +1088,7 @@ int tipc_link_xmit(struct tipc_link *l, struct sk_buff_head *list,
>  			if (!_skb) {
>  				kfree_skb(skb);
>  				__skb_queue_purge(list);
> -				return -ENOBUFS;
> +				return -ENOMEM;
>  			}
>  			__skb_queue_tail(transmq, skb);
>  			tipc_link_set_skb_retransmit_time(skb, l);
> --
> 2.25.1

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

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

Hi Jon,

Let's enjoy your vacation.
Our new team member (CCed) will take a look at it. 

Regards,
Hoang
> -----Original Message-----
> From: Jon Maloy <jm...@re...>
> Sent: Wednesday, July 28, 2021 6:20 AM
> To: tip...@li...; Tung Quang Nguyen <tun...@de...>; Hoang Huu Le
> <hoa...@de...>; Xin Long <luc...@gm...>; Ying Xue <yin...@wi...>
> Subject: Strange behavior in socket.c::tipc_sk_enqueue()
> 
> I did by accident discover a strange behavior in the function
> tipc_sk_enqueue:
> 
> 
> static void tipc_sk_enqueue(struct sk_buff_head *inputq,
>                              struct sock *sk, u32 dport,
>                              struct sk_buff_head *xmitq)
> {
>          struct tipc_sock *tsk = tipc_sk(sk);
>          unsigned long time_limit = jiffies + 2;
>          struct sk_buff *skb;
>          unsigned int lim;
>          atomic_t *dcnt;
>          u32 onode;
> 
>          while (skb_queue_len(inputq)) {
>          if (unlikely(time_after_eq(jiffies, time_limit)))
>                return;
>          [...]
>          }
> }
> 
> At the moment we call time_after_eq() the two jiffies often
> have already passed, and the skb is not dequeued.
> I noticed that tipc_sk_rcv() may call tipc_sk_enqueue()
> with the same skb dozens of times before the buffer can
> be delivered further upwards in the stack.
> 
> Needless to say that this cannot be good for performance.
> 
> I believe the value of 2 jiffies was hard coded at a time
> when machines were slower, and a jiffie represented a much
> longer time interval.
> 
> Now it is clearly too short, and should be replaced with something
> longer and more consisten, e.g. msec_to_jiffies(2).
> 
> Can anybody look into this?
> 
> Also, I will be on vacation the next two weeks, which means we
> should cancel the bi-weekly meeting next Tuesday.
> 
> ///jon
> 

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

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

I did by accident discover a strange behavior in the function 
tipc_sk_enqueue:


static void tipc_sk_enqueue(struct sk_buff_head *inputq,
                             struct sock *sk, u32 dport,
                             struct sk_buff_head *xmitq)
{
         struct tipc_sock *tsk = tipc_sk(sk);
         unsigned long time_limit = jiffies + 2;
         struct sk_buff *skb;
         unsigned int lim;
         atomic_t *dcnt;
         u32 onode;

         while (skb_queue_len(inputq)) {
         if (unlikely(time_after_eq(jiffies, time_limit)))
               return;
         [...]
         }
}

At the moment we call time_after_eq() the two jiffies often
have already passed, and the skb is not dequeued.
I noticed that tipc_sk_rcv() may call tipc_sk_enqueue()
with the same skb dozens of times before the buffer can
be delivered further upwards in the stack.

Needless to say that this cannot be good for performance.

I believe the value of 2 jiffies was hard coded at a time
when machines were slower, and a jiffie represented a much
longer time interval.

Now it is clearly too short, and should be replaced with something
longer and more consisten, e.g. msec_to_jiffies(2).

Can anybody look into this?

Also, I will be on vacation the next two weeks, which means we
should cancel the bi-weekly meeting next Tuesday.

///jon

[tipc-discussion] [PATCH net] tipc: do not write skb_shinfo frags when doing decrytion

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

One skb's skb_shinfo frags are not writable, and they can be shared with
other skbs' like by pskb_copy(). To write the frags may cause other skb's
data crash.

So before doing en/decryption, skb_cow_data() should always be called for
a cloned or nonlinear skb if req dst is using the same sg as req src.
While at it, the likely branch can be removed, as it will be covered
by skb_cow_data().

Note that esp_input() has the same issue, and I will fix it in another
patch. tipc_aead_encrypt() doesn't have this issue, as it only processes
linear data in the unlikely branch.

Fixes: fc1b6d6de220 ("tipc: introduce TIPC encryption & authentication")
Reported-by: Shuang Li <sh...@re...>
Signed-off-by: Xin Long <luc...@gm...>
Acked-by: Jon Maloy <jm...@re...>
---
 net/tipc/crypto.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index e5c43d4d5a75..c9391d38de85 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -898,16 +898,10 @@ static int tipc_aead_decrypt(struct net *net, struct tipc_aead *aead,
 	if (unlikely(!aead))
 		return -ENOKEY;
 
-	/* Cow skb data if needed */
-	if (likely(!skb_cloned(skb) &&
-		   (!skb_is_nonlinear(skb) || !skb_has_frag_list(skb)))) {
-		nsg = 1 + skb_shinfo(skb)->nr_frags;
-	} else {
-		nsg = skb_cow_data(skb, 0, &unused);
-		if (unlikely(nsg < 0)) {
-			pr_err("RX: skb_cow_data() returned %d\n", nsg);
-			return nsg;
-		}
+	nsg = skb_cow_data(skb, 0, &unused);
+	if (unlikely(nsg < 0)) {
+		pr_err("RX: skb_cow_data() returned %d\n", nsg);
+		return nsg;
 	}
 
 	/* Allocate memory for the AEAD operation */
-- 
2.27.0

Re: [tipc-discussion] [PATCH net] tipc: do not write skb_shinfo frags when doing decrytion

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

On 23/07/2021 13:43, Xin Long wrote:
> One skb's skb_shinfo frags are not writable, and they can be shared with
> other skbs' like by pskb_copy(). To write the frags may cause other skb's
> data crash.
> 
> So before doing en/decryption, skb_cow_data() should always be called for
> a cloned or nonlinear skb if req dst is using the same sg as req src.
> While at it, the likely branch can be removed, as it will be covered
> by skb_cow_data().
> 
> Note that esp_input() has the same issue, and I will fix it in another
> patch. tipc_aead_encrypt() doesn't have this issue, as it only processes
> linear data in the unlikely branch.
> 
> Fixes: fc1b6d6de220 ("tipc: introduce TIPC encryption & authentication")
> Reported-by: Shuang Li <sh...@re...>
> Signed-off-by: Xin Long <luc...@gm...>
> ---
>   net/tipc/crypto.c | 14 ++++----------
>   1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
> index e5c43d4d5a75..c9391d38de85 100644
> --- a/net/tipc/crypto.c
> +++ b/net/tipc/crypto.c
> @@ -898,16 +898,10 @@ static int tipc_aead_decrypt(struct net *net, struct tipc_aead *aead,
>   	if (unlikely(!aead))
>   		return -ENOKEY;
>   
> -	/* Cow skb data if needed */
> -	if (likely(!skb_cloned(skb) &&
> -		   (!skb_is_nonlinear(skb) || !skb_has_frag_list(skb)))) {
> -		nsg = 1 + skb_shinfo(skb)->nr_frags;
> -	} else {
> -		nsg = skb_cow_data(skb, 0, &unused);
> -		if (unlikely(nsg < 0)) {
> -			pr_err("RX: skb_cow_data() returned %d\n", nsg);
> -			return nsg;
> -		}
> +	nsg = skb_cow_data(skb, 0, &unused);
> +	if (unlikely(nsg < 0)) {
> +		pr_err("RX: skb_cow_data() returned %d\n", nsg);
> +		return nsg;
>   	}
>   
>   	/* Allocate memory for the AEAD operation */
> 
Acked-by: Jon Maloy <jm...@re...>

[tipc-discussion] [PATCH net] tipc: do not write skb_shinfo frags when doing decrytion

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

One skb's skb_shinfo frags are not writable, and they can be shared with
other skbs' like by pskb_copy(). To write the frags may cause other skb's
data crash.

So before doing en/decryption, skb_cow_data() should always be called for
a cloned or nonlinear skb if req dst is using the same sg as req src.
While at it, the likely branch can be removed, as it will be covered
by skb_cow_data().

Note that esp_input() has the same issue, and I will fix it in another
patch. tipc_aead_encrypt() doesn't have this issue, as it only processes
linear data in the unlikely branch.

Fixes: fc1b6d6de220 ("tipc: introduce TIPC encryption & authentication")
Reported-by: Shuang Li <sh...@re...>
Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/crypto.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index e5c43d4d5a75..c9391d38de85 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -898,16 +898,10 @@ static int tipc_aead_decrypt(struct net *net, struct tipc_aead *aead,
 	if (unlikely(!aead))
 		return -ENOKEY;
 
-	/* Cow skb data if needed */
-	if (likely(!skb_cloned(skb) &&
-		   (!skb_is_nonlinear(skb) || !skb_has_frag_list(skb)))) {
-		nsg = 1 + skb_shinfo(skb)->nr_frags;
-	} else {
-		nsg = skb_cow_data(skb, 0, &unused);
-		if (unlikely(nsg < 0)) {
-			pr_err("RX: skb_cow_data() returned %d\n", nsg);
-			return nsg;
-		}
+	nsg = skb_cow_data(skb, 0, &unused);
+	if (unlikely(nsg < 0)) {
+		pr_err("RX: skb_cow_data() returned %d\n", nsg);
+		return nsg;
 	}
 
 	/* Allocate memory for the AEAD operation */
-- 
2.27.0

[tipc-discussion] [PATCH net-next] tipc: fix an use-after-free issue in tipc_recvmsg

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

syzbot reported an use-after-free crash:

  BUG: KASAN: use-after-free in tipc_recvmsg+0xf77/0xf90 net/tipc/socket.c:1979
  Call Trace:
   tipc_recvmsg+0xf77/0xf90 net/tipc/socket.c:1979
   sock_recvmsg_nosec net/socket.c:943 [inline]
   sock_recvmsg net/socket.c:961 [inline]
   sock_recvmsg+0xca/0x110 net/socket.c:957
   tipc_conn_rcv_from_sock+0x162/0x2f0 net/tipc/topsrv.c:398
   tipc_conn_recv_work+0xeb/0x190 net/tipc/topsrv.c:421
   process_one_work+0x98d/0x1630 kernel/workqueue.c:2276
   worker_thread+0x658/0x11f0 kernel/workqueue.c:2422

As Hoang pointed out, it was caused by skb_cb->bytes_read still accessed
after calling tsk_advance_rx_queue() to free the skb in tipc_recvmsg().

This patch is to fix it by accessing skb_cb->bytes_read earlier than
calling tsk_advance_rx_queue().

Fixes: f4919ff59c28 ("tipc: keep the skb in rcv queue until the whole data is read")
Reported-by: syz...@sy...
Signed-off-by: Xin Long <luc...@gm...>
Acked-by: Jon Maloy <jm...@re...>
---
 net/tipc/socket.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 9b0b311c7ec1..b0dd183a4dbc 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1973,10 +1973,12 @@ static int tipc_recvmsg(struct socket *sock, struct msghdr *m,
 		tipc_node_distr_xmit(sock_net(sk), &xmitq);
 	}
 
-	if (!skb_cb->bytes_read)
-		tsk_advance_rx_queue(sk);
+	if (skb_cb->bytes_read)
+		goto exit;
+
+	tsk_advance_rx_queue(sk);
 
-	if (likely(!connected) || skb_cb->bytes_read)
+	if (likely(!connected))
 		goto exit;
 
 	/* Send connection flow control advertisement when applicable */
-- 
2.27.0

Re: [tipc-discussion] [PATCH net-next] tipc: fix an use-after-free issue in tipc_recvmsg

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

 

    On Friday, July 23, 2021, 12:40:22 PM EDT, Xin Long <luc...@gm...> wrote:  
 
 syzbot reported an use-after-free crash:

  BUG: KASAN: use-after-free in tipc_recvmsg+0xf77/0xf90 net/tipc/socket.c:1979
  Call Trace:
  tipc_recvmsg+0xf77/0xf90 net/tipc/socket.c:1979
  sock_recvmsg_nosec net/socket.c:943 [inline]
  sock_recvmsg net/socket.c:961 [inline]
  sock_recvmsg+0xca/0x110 net/socket.c:957
  tipc_conn_rcv_from_sock+0x162/0x2f0 net/tipc/topsrv.c:398
  tipc_conn_recv_work+0xeb/0x190 net/tipc/topsrv.c:421
  process_one_work+0x98d/0x1630 kernel/workqueue.c:2276
  worker_thread+0x658/0x11f0 kernel/workqueue.c:2422

As Hoang pointed out, it was caused by skb_cb->bytes_read still accessed
after calling tsk_advance_rx_queue() to free the skb in tipc_recvmsg().

This patch is to fix it by accessing skb_cb->bytes_read earlier than
calling tsk_advance_rx_queue().

Fixes: f4919ff59c28 ("tipc: keep the skb in rcv queue until the whole data is read")
Reported-by: syz...@sy...
Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/socket.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 9b0b311c7ec1..b0dd183a4dbc 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1973,10 +1973,12 @@ static int tipc_recvmsg(struct socket *sock, struct msghdr *m,
         tipc_node_distr_xmit(sock_net(sk), &xmitq);
     }
 
-    if (!skb_cb->bytes_read)
-        tsk_advance_rx_queue(sk);
+    if (skb_cb->bytes_read)
+        goto exit;
+
+    tsk_advance_rx_queue(sk);
 
-    if (likely(!connected) || skb_cb->bytes_read)
+    if (likely(!connected))
         goto exit;
 
     /* Send connection flow control advertisement when applicable */
-- 
2.27.0

For some reason this one did not show up in my RH mailbox.Anyway,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: fix an use-after-free issue in tipc_recvmsg

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

syzbot reported an use-after-free crash:

  BUG: KASAN: use-after-free in tipc_recvmsg+0xf77/0xf90 net/tipc/socket.c:1979
  Call Trace:
   tipc_recvmsg+0xf77/0xf90 net/tipc/socket.c:1979
   sock_recvmsg_nosec net/socket.c:943 [inline]
   sock_recvmsg net/socket.c:961 [inline]
   sock_recvmsg+0xca/0x110 net/socket.c:957
   tipc_conn_rcv_from_sock+0x162/0x2f0 net/tipc/topsrv.c:398
   tipc_conn_recv_work+0xeb/0x190 net/tipc/topsrv.c:421
   process_one_work+0x98d/0x1630 kernel/workqueue.c:2276
   worker_thread+0x658/0x11f0 kernel/workqueue.c:2422

As Hoang pointed out, it was caused by skb_cb->bytes_read still accessed
after calling tsk_advance_rx_queue() to free the skb in tipc_recvmsg().

This patch is to fix it by accessing skb_cb->bytes_read earlier than
calling tsk_advance_rx_queue().

Fixes: f4919ff59c28 ("tipc: keep the skb in rcv queue until the whole data is read")
Reported-by: syz...@sy...
Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/socket.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 9b0b311c7ec1..b0dd183a4dbc 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1973,10 +1973,12 @@ static int tipc_recvmsg(struct socket *sock, struct msghdr *m,
 		tipc_node_distr_xmit(sock_net(sk), &xmitq);
 	}
 
-	if (!skb_cb->bytes_read)
-		tsk_advance_rx_queue(sk);
+	if (skb_cb->bytes_read)
+		goto exit;
+
+	tsk_advance_rx_queue(sk);
 
-	if (likely(!connected) || skb_cb->bytes_read)
+	if (likely(!connected))
 		goto exit;
 
 	/* Send connection flow control advertisement when applicable */
-- 
2.27.0

[tipc-discussion] [net] tipc: fix sleeping in tipc accept routine

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

The release_sock() is blocking function, it would change the state
after sleeping. In order to evaluate the stated condition outside
the socket lock context, switch to use wait_woken() instead.

Fixes: 6398e23cdb1d8 ("tipc: standardize accept routine")
Acked-by: Jon Maloy <jm...@re...>
Signed-off-by: Hoang Le <hoa...@de...>
---
 net/tipc/socket.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 9b0b311c7ec1..2c71828b7e5c 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2664,7 +2664,7 @@ static int tipc_listen(struct socket *sock, int len)
 static int tipc_wait_for_accept(struct socket *sock, long timeo)
 {
 	struct sock *sk = sock->sk;
-	DEFINE_WAIT(wait);
+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
 	int err;
 
 	/* True wake-one mechanism for incoming connections: only
@@ -2673,12 +2673,12 @@ static int tipc_wait_for_accept(struct socket *sock, long timeo)
 	 * anymore, the common case will execute the loop only once.
 	*/
 	for (;;) {
-		prepare_to_wait_exclusive(sk_sleep(sk), &wait,
-					  TASK_INTERRUPTIBLE);
 		if (timeo && skb_queue_empty(&sk->sk_receive_queue)) {
+			add_wait_queue(sk_sleep(sk), &wait);
 			release_sock(sk);
-			timeo = schedule_timeout(timeo);
+			timeo = wait_woken(&wait, TASK_INTERRUPTIBLE, timeo);
 			lock_sock(sk);
+			remove_wait_queue(sk_sleep(sk), &wait);
 		}
 		err = 0;
 		if (!skb_queue_empty(&sk->sk_receive_queue))
@@ -2690,7 +2690,6 @@ static int tipc_wait_for_accept(struct socket *sock, long timeo)
 		if (signal_pending(current))
 			break;
 	}
-	finish_wait(sk_sleep(sk), &wait);
 	return err;
 }
 
-- 
2.30.2

Re: [tipc-discussion] FW: [syzbot] KASAN: use-after-free Read in tipc_recvmsg

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

On Thu, Jul 22, 2021 at 9:55 PM Hoang Huu Le <hoa...@de...> wrote:
>
> Hi Xin,
>
> I think the issue caused by your patch:
>
> f4919ff59c282 ("tipc: keep the skb in rcv queue until the whole data is read)
> <snip>
> 1976         if (!skb_cb->bytes_read)
> 1977                 tsk_advance_rx_queue(sk);   <-- skb free-ed here
> 1978
> 1979         if (likely(!connected) || skb_cb->bytes_read) <- use-after-free
> 1980                 goto exit;
> </snip>
>
> Can you take a look at the issue.
will do, thanks for reminding.

>
> Thanks,
> Hoang
> -----Original Message-----
> From: syzbot <syz...@sy...>
> Sent: Monday, July 19, 2021 12:15 AM
> To: da...@da...; dev...@vg...; fro...@gm...; gr...@li...; jm...@re...; ku...@ke...; lin...@vg...; ne...@vg...; ra...@ke...; ro...@ke...; ro...@ke...; syz...@go...; tip...@li...; yin...@wi...
> Subject: [syzbot] KASAN: use-after-free Read in tipc_recvmsg
>
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit:    ab0441b4a920 Merge branch 'vmxnet3-version-6'
> git tree:       net-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=1744ac6a300000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=da140227e4f25b17
> dashboard link: https://syzkaller.appspot.com/bug?extid=e6741b97d5552f97c24d
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=13973a74300000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=17ffc902300000
>
> The issue was bisected to:
>
> commit 67a3156453859ceb40dc4448b7a6a99ea0ad27c7
> Author: Rob Herring <ro...@ke...>
> Date:   Thu May 27 19:45:47 2021 +0000
>
>     of: Merge of_address_to_resource() and of_pci_address_to_resource() implementations
>
> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=129b0438300000
> final oops:     https://syzkaller.appspot.com/x/report.txt?x=119b0438300000
> console output: https://syzkaller.appspot.com/x/log.txt?x=169b0438300000
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syz...@sy...
> Fixes: 67a315645385 ("of: Merge of_address_to_resource() and of_pci_address_to_resource() implementations")
>
> ==================================================================
> BUG: KASAN: use-after-free in tipc_recvmsg+0xf77/0xf90 net/tipc/socket.c:1979
> Read of size 4 at addr ffff8880328cf1c0 by task kworker/u4:0/8
>
> CPU: 1 PID: 8 Comm: kworker/u4:0 Not tainted 5.14.0-rc1-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Workqueue: tipc_rcv tipc_conn_recv_work
> Call Trace:
>  __dump_stack lib/dump_stack.c:88 [inline]
>  dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:105
>  print_address_description.constprop.0.cold+0x6c/0x309 mm/kasan/report.c:233
>  __kasan_report mm/kasan/report.c:419 [inline]
>  kasan_report.cold+0x83/0xdf mm/kasan/report.c:436
>  tipc_recvmsg+0xf77/0xf90 net/tipc/socket.c:1979
>  sock_recvmsg_nosec net/socket.c:943 [inline]
>  sock_recvmsg net/socket.c:961 [inline]
>  sock_recvmsg+0xca/0x110 net/socket.c:957
>  tipc_conn_rcv_from_sock+0x162/0x2f0 net/tipc/topsrv.c:398
>  tipc_conn_recv_work+0xeb/0x190 net/tipc/topsrv.c:421
>  process_one_work+0x98d/0x1630 kernel/workqueue.c:2276
>  worker_thread+0x658/0x11f0 kernel/workqueue.c:2422
>  kthread+0x3e5/0x4d0 kernel/kthread.c:319
>  ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
>
> Allocated by task 8446:
>  kasan_save_stack+0x1b/0x40 mm/kasan/common.c:38
>  kasan_set_track mm/kasan/common.c:46 [inline]
>  set_alloc_info mm/kasan/common.c:434 [inline]
>  __kasan_slab_alloc+0x84/0xa0 mm/kasan/common.c:467
>  kasan_slab_alloc include/linux/kasan.h:253 [inline]
>  slab_post_alloc_hook mm/slab.h:512 [inline]
>  slab_alloc_node mm/slub.c:2981 [inline]
>  kmem_cache_alloc_node+0x266/0x3e0 mm/slub.c:3017
>  __alloc_skb+0x20b/0x340 net/core/skbuff.c:414
>  alloc_skb_fclone include/linux/skbuff.h:1162 [inline]
>  tipc_buf_acquire+0x25/0xe0 net/tipc/msg.c:72
>  tipc_msg_build+0xf7/0x10a0 net/tipc/msg.c:386
>  __tipc_sendstream+0x6d0/0x1150 net/tipc/socket.c:1610
>  tipc_sendstream+0x4c/0x70 net/tipc/socket.c:1541
>  sock_sendmsg_nosec net/socket.c:703 [inline]
>  sock_sendmsg+0xcf/0x120 net/socket.c:723
>  sock_write_iter+0x289/0x3c0 net/socket.c:1056
>  call_write_iter include/linux/fs.h:2114 [inline]
>  new_sync_write+0x426/0x650 fs/read_write.c:518
>  vfs_write+0x75a/0xa40 fs/read_write.c:605
>  ksys_write+0x1ee/0x250 fs/read_write.c:658
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> Freed by task 8:
>  kasan_save_stack+0x1b/0x40 mm/kasan/common.c:38
>  kasan_set_track+0x1c/0x30 mm/kasan/common.c:46
>  kasan_set_free_info+0x20/0x30 mm/kasan/generic.c:360
>  ____kasan_slab_free mm/kasan/common.c:366 [inline]
>  ____kasan_slab_free mm/kasan/common.c:328 [inline]
>  __kasan_slab_free+0xfb/0x130 mm/kasan/common.c:374
>  kasan_slab_free include/linux/kasan.h:229 [inline]
>  slab_free_hook mm/slub.c:1650 [inline]
>  slab_free_freelist_hook+0xdf/0x240 mm/slub.c:1675
>  slab_free mm/slub.c:3235 [inline]
>  kmem_cache_free+0x8e/0x5a0 mm/slub.c:3251
>  kfree_skbmem+0x166/0x1b0 net/core/skbuff.c:709
>  __kfree_skb net/core/skbuff.c:745 [inline]
>  kfree_skb net/core/skbuff.c:762 [inline]
>  kfree_skb+0x140/0x3f0 net/core/skbuff.c:756
>  tipc_recvmsg+0x70d/0xf90 net/tipc/socket.c:1977
>  sock_recvmsg_nosec net/socket.c:943 [inline]
>  sock_recvmsg net/socket.c:961 [inline]
>  sock_recvmsg+0xca/0x110 net/socket.c:957
>  tipc_conn_rcv_from_sock+0x162/0x2f0 net/tipc/topsrv.c:398
>  tipc_conn_recv_work+0xeb/0x190 net/tipc/topsrv.c:421
>  process_one_work+0x98d/0x1630 kernel/workqueue.c:2276
>  worker_thread+0x658/0x11f0 kernel/workqueue.c:2422
>  kthread+0x3e5/0x4d0 kernel/kthread.c:319
>  ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
>
> The buggy address belongs to the object at ffff8880328cf180
>  which belongs to the cache skbuff_fclone_cache of size 472
> The buggy address is located 64 bytes inside of
>  472-byte region [ffff8880328cf180, ffff8880328cf358)
> The buggy address belongs to the page:
> page:ffffea0000ca3380 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x328ce
> head:ffffea0000ca3380 order:1 compound_mapcount:0
> flags: 0xfff00000010200(slab|head|node=0|zone=1|lastcpupid=0x7ff)
> raw: 00fff00000010200 ffffea0000811500 0000000300000003 ffff8881400ee280
> raw: 0000000000000000 00000000800c000c 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
> page_owner tracks the page as allocated
> page last allocated via order 1, migratetype Unmovable, gfp_mask 0xd20c0(__GFP_IO|__GFP_FS|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP|__GFP_NOMEMALLOC), pid 8424, ts 65082628156, free_ts 64879784131
>  prep_new_page mm/page_alloc.c:2433 [inline]
>  get_page_from_freelist+0xa72/0x2f80 mm/page_alloc.c:4166
>  __alloc_pages+0x1b2/0x500 mm/page_alloc.c:5374
>  alloc_pages+0x18c/0x2a0 mm/mempolicy.c:2244
>  alloc_slab_page mm/slub.c:1713 [inline]
>  allocate_slab+0x32b/0x4c0 mm/slub.c:1853
>  new_slab mm/slub.c:1916 [inline]
>  new_slab_objects mm/slub.c:2662 [inline]
>  ___slab_alloc+0x4ba/0x820 mm/slub.c:2825
>  __slab_alloc.constprop.0+0xa7/0xf0 mm/slub.c:2865
>  slab_alloc_node mm/slub.c:2947 [inline]
>  kmem_cache_alloc_node+0x12c/0x3e0 mm/slub.c:3017
>  __alloc_skb+0x20b/0x340 net/core/skbuff.c:414
>  alloc_skb_fclone include/linux/skbuff.h:1162 [inline]
>  sk_stream_alloc_skb+0x109/0xc30 net/ipv4/tcp.c:887
>  tcp_sendmsg_locked+0xc78/0x2f10 net/ipv4/tcp.c:1309
>  tcp_sendmsg+0x2b/0x40 net/ipv4/tcp.c:1461
>  inet_sendmsg+0x99/0xe0 net/ipv4/af_inet.c:821
>  sock_sendmsg_nosec net/socket.c:703 [inline]
>  sock_sendmsg+0xcf/0x120 net/socket.c:723
>  sock_write_iter+0x289/0x3c0 net/socket.c:1056
>  call_write_iter include/linux/fs.h:2114 [inline]
>  new_sync_write+0x426/0x650 fs/read_write.c:518
>  vfs_write+0x75a/0xa40 fs/read_write.c:605
> page last free stack trace:
>  reset_page_owner include/linux/page_owner.h:24 [inline]
>  free_pages_prepare mm/page_alloc.c:1343 [inline]
>  free_pcp_prepare+0x2c5/0x780 mm/page_alloc.c:1394
>  free_unref_page_prepare mm/page_alloc.c:3329 [inline]
>  free_unref_page+0x19/0x690 mm/page_alloc.c:3408
>  unfreeze_partials+0x17c/0x1d0 mm/slub.c:2443
>  put_cpu_partial+0x13d/0x230 mm/slub.c:2479
>  qlink_free mm/kasan/quarantine.c:146 [inline]
>  qlist_free_all+0x5a/0xc0 mm/kasan/quarantine.c:165
>  kasan_quarantine_reduce+0x180/0x200 mm/kasan/quarantine.c:272
>  __kasan_slab_alloc+0x8e/0xa0 mm/kasan/common.c:444
>  kasan_slab_alloc include/linux/kasan.h:253 [inline]
>  slab_post_alloc_hook mm/slab.h:512 [inline]
>  slab_alloc_node mm/slub.c:2981 [inline]
>  slab_alloc mm/slub.c:2989 [inline]
>  kmem_cache_alloc+0x216/0x3a0 mm/slub.c:2994
>  getname_flags.part.0+0x50/0x4f0 fs/namei.c:138
>  getname_flags fs/namei.c:2747 [inline]
>  user_path_at_empty+0xa1/0x100 fs/namei.c:2747
>  user_path_at include/linux/namei.h:57 [inline]
>  vfs_statx+0x142/0x390 fs/stat.c:203
>  vfs_fstatat fs/stat.c:225 [inline]
>  vfs_lstat include/linux/fs.h:3386 [inline]
>  __do_sys_newlstat+0x91/0x110 fs/stat.c:380
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> Memory state around the buggy address:
>  ffff8880328cf080: fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc fc
>  ffff8880328cf100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >ffff8880328cf180: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>                                            ^
>  ffff8880328cf200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>  ffff8880328cf280: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ==================================================================
>
>
> ---
> This report is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syz...@go....
>
> syzbot will keep track of this issue. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
> For information about bisection process see: https://goo.gl/tpsmEJ#bisection
> syzbot can test patches for this issue, for details see:
> https://goo.gl/tpsmEJ#testing-patches

[tipc-discussion] FW: [syzbot] KASAN: use-after-free Read in tipc_recvmsg

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

Hi Xin,

I think the issue caused by your patch:

f4919ff59c282 ("tipc: keep the skb in rcv queue until the whole data is read)
<snip>
1976         if (!skb_cb->bytes_read)
1977                 tsk_advance_rx_queue(sk);   <-- skb free-ed here
1978
1979         if (likely(!connected) || skb_cb->bytes_read) <- use-after-free
1980                 goto exit;
</snip>

Can you take a look at the issue.

Thanks,
Hoang
-----Original Message-----
From: syzbot <syz...@sy...> 
Sent: Monday, July 19, 2021 12:15 AM
To: da...@da...; dev...@vg...; fro...@gm...; gr...@li...; jm...@re...; ku...@ke...; lin...@vg...; ne...@vg...; ra...@ke...; ro...@ke...; ro...@ke...; syz...@go...; tip...@li...; yin...@wi...
Subject: [syzbot] KASAN: use-after-free Read in tipc_recvmsg

Hello,

syzbot found the following issue on:

HEAD commit:    ab0441b4a920 Merge branch 'vmxnet3-version-6'
git tree:       net-next
console output: https://syzkaller.appspot.com/x/log.txt?x=1744ac6a300000
kernel config:  https://syzkaller.appspot.com/x/.config?x=da140227e4f25b17
dashboard link: https://syzkaller.appspot.com/bug?extid=e6741b97d5552f97c24d
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=13973a74300000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=17ffc902300000

The issue was bisected to:

commit 67a3156453859ceb40dc4448b7a6a99ea0ad27c7
Author: Rob Herring <ro...@ke...>
Date:   Thu May 27 19:45:47 2021 +0000

    of: Merge of_address_to_resource() and of_pci_address_to_resource() implementations

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=129b0438300000
final oops:     https://syzkaller.appspot.com/x/report.txt?x=119b0438300000
console output: https://syzkaller.appspot.com/x/log.txt?x=169b0438300000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syz...@sy...
Fixes: 67a315645385 ("of: Merge of_address_to_resource() and of_pci_address_to_resource() implementations")

==================================================================
BUG: KASAN: use-after-free in tipc_recvmsg+0xf77/0xf90 net/tipc/socket.c:1979
Read of size 4 at addr ffff8880328cf1c0 by task kworker/u4:0/8

CPU: 1 PID: 8 Comm: kworker/u4:0 Not tainted 5.14.0-rc1-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: tipc_rcv tipc_conn_recv_work
Call Trace:
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:105
 print_address_description.constprop.0.cold+0x6c/0x309 mm/kasan/report.c:233
 __kasan_report mm/kasan/report.c:419 [inline]
 kasan_report.cold+0x83/0xdf mm/kasan/report.c:436
 tipc_recvmsg+0xf77/0xf90 net/tipc/socket.c:1979
 sock_recvmsg_nosec net/socket.c:943 [inline]
 sock_recvmsg net/socket.c:961 [inline]
 sock_recvmsg+0xca/0x110 net/socket.c:957
 tipc_conn_rcv_from_sock+0x162/0x2f0 net/tipc/topsrv.c:398
 tipc_conn_recv_work+0xeb/0x190 net/tipc/topsrv.c:421
 process_one_work+0x98d/0x1630 kernel/workqueue.c:2276
 worker_thread+0x658/0x11f0 kernel/workqueue.c:2422
 kthread+0x3e5/0x4d0 kernel/kthread.c:319
 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295

Allocated by task 8446:
 kasan_save_stack+0x1b/0x40 mm/kasan/common.c:38
 kasan_set_track mm/kasan/common.c:46 [inline]
 set_alloc_info mm/kasan/common.c:434 [inline]
 __kasan_slab_alloc+0x84/0xa0 mm/kasan/common.c:467
 kasan_slab_alloc include/linux/kasan.h:253 [inline]
 slab_post_alloc_hook mm/slab.h:512 [inline]
 slab_alloc_node mm/slub.c:2981 [inline]
 kmem_cache_alloc_node+0x266/0x3e0 mm/slub.c:3017
 __alloc_skb+0x20b/0x340 net/core/skbuff.c:414
 alloc_skb_fclone include/linux/skbuff.h:1162 [inline]
 tipc_buf_acquire+0x25/0xe0 net/tipc/msg.c:72
 tipc_msg_build+0xf7/0x10a0 net/tipc/msg.c:386
 __tipc_sendstream+0x6d0/0x1150 net/tipc/socket.c:1610
 tipc_sendstream+0x4c/0x70 net/tipc/socket.c:1541
 sock_sendmsg_nosec net/socket.c:703 [inline]
 sock_sendmsg+0xcf/0x120 net/socket.c:723
 sock_write_iter+0x289/0x3c0 net/socket.c:1056
 call_write_iter include/linux/fs.h:2114 [inline]
 new_sync_write+0x426/0x650 fs/read_write.c:518
 vfs_write+0x75a/0xa40 fs/read_write.c:605
 ksys_write+0x1ee/0x250 fs/read_write.c:658
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Freed by task 8:
 kasan_save_stack+0x1b/0x40 mm/kasan/common.c:38
 kasan_set_track+0x1c/0x30 mm/kasan/common.c:46
 kasan_set_free_info+0x20/0x30 mm/kasan/generic.c:360
 ____kasan_slab_free mm/kasan/common.c:366 [inline]
 ____kasan_slab_free mm/kasan/common.c:328 [inline]
 __kasan_slab_free+0xfb/0x130 mm/kasan/common.c:374
 kasan_slab_free include/linux/kasan.h:229 [inline]
 slab_free_hook mm/slub.c:1650 [inline]
 slab_free_freelist_hook+0xdf/0x240 mm/slub.c:1675
 slab_free mm/slub.c:3235 [inline]
 kmem_cache_free+0x8e/0x5a0 mm/slub.c:3251
 kfree_skbmem+0x166/0x1b0 net/core/skbuff.c:709
 __kfree_skb net/core/skbuff.c:745 [inline]
 kfree_skb net/core/skbuff.c:762 [inline]
 kfree_skb+0x140/0x3f0 net/core/skbuff.c:756
 tipc_recvmsg+0x70d/0xf90 net/tipc/socket.c:1977
 sock_recvmsg_nosec net/socket.c:943 [inline]
 sock_recvmsg net/socket.c:961 [inline]
 sock_recvmsg+0xca/0x110 net/socket.c:957
 tipc_conn_rcv_from_sock+0x162/0x2f0 net/tipc/topsrv.c:398
 tipc_conn_recv_work+0xeb/0x190 net/tipc/topsrv.c:421
 process_one_work+0x98d/0x1630 kernel/workqueue.c:2276
 worker_thread+0x658/0x11f0 kernel/workqueue.c:2422
 kthread+0x3e5/0x4d0 kernel/kthread.c:319
 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295

The buggy address belongs to the object at ffff8880328cf180
 which belongs to the cache skbuff_fclone_cache of size 472
The buggy address is located 64 bytes inside of
 472-byte region [ffff8880328cf180, ffff8880328cf358)
The buggy address belongs to the page:
page:ffffea0000ca3380 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x328ce
head:ffffea0000ca3380 order:1 compound_mapcount:0
flags: 0xfff00000010200(slab|head|node=0|zone=1|lastcpupid=0x7ff)
raw: 00fff00000010200 ffffea0000811500 0000000300000003 ffff8881400ee280
raw: 0000000000000000 00000000800c000c 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
page_owner tracks the page as allocated
page last allocated via order 1, migratetype Unmovable, gfp_mask 0xd20c0(__GFP_IO|__GFP_FS|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP|__GFP_NOMEMALLOC), pid 8424, ts 65082628156, free_ts 64879784131
 prep_new_page mm/page_alloc.c:2433 [inline]
 get_page_from_freelist+0xa72/0x2f80 mm/page_alloc.c:4166
 __alloc_pages+0x1b2/0x500 mm/page_alloc.c:5374
 alloc_pages+0x18c/0x2a0 mm/mempolicy.c:2244
 alloc_slab_page mm/slub.c:1713 [inline]
 allocate_slab+0x32b/0x4c0 mm/slub.c:1853
 new_slab mm/slub.c:1916 [inline]
 new_slab_objects mm/slub.c:2662 [inline]
 ___slab_alloc+0x4ba/0x820 mm/slub.c:2825
 __slab_alloc.constprop.0+0xa7/0xf0 mm/slub.c:2865
 slab_alloc_node mm/slub.c:2947 [inline]
 kmem_cache_alloc_node+0x12c/0x3e0 mm/slub.c:3017
 __alloc_skb+0x20b/0x340 net/core/skbuff.c:414
 alloc_skb_fclone include/linux/skbuff.h:1162 [inline]
 sk_stream_alloc_skb+0x109/0xc30 net/ipv4/tcp.c:887
 tcp_sendmsg_locked+0xc78/0x2f10 net/ipv4/tcp.c:1309
 tcp_sendmsg+0x2b/0x40 net/ipv4/tcp.c:1461
 inet_sendmsg+0x99/0xe0 net/ipv4/af_inet.c:821
 sock_sendmsg_nosec net/socket.c:703 [inline]
 sock_sendmsg+0xcf/0x120 net/socket.c:723
 sock_write_iter+0x289/0x3c0 net/socket.c:1056
 call_write_iter include/linux/fs.h:2114 [inline]
 new_sync_write+0x426/0x650 fs/read_write.c:518
 vfs_write+0x75a/0xa40 fs/read_write.c:605
page last free stack trace:
 reset_page_owner include/linux/page_owner.h:24 [inline]
 free_pages_prepare mm/page_alloc.c:1343 [inline]
 free_pcp_prepare+0x2c5/0x780 mm/page_alloc.c:1394
 free_unref_page_prepare mm/page_alloc.c:3329 [inline]
 free_unref_page+0x19/0x690 mm/page_alloc.c:3408
 unfreeze_partials+0x17c/0x1d0 mm/slub.c:2443
 put_cpu_partial+0x13d/0x230 mm/slub.c:2479
 qlink_free mm/kasan/quarantine.c:146 [inline]
 qlist_free_all+0x5a/0xc0 mm/kasan/quarantine.c:165
 kasan_quarantine_reduce+0x180/0x200 mm/kasan/quarantine.c:272
 __kasan_slab_alloc+0x8e/0xa0 mm/kasan/common.c:444
 kasan_slab_alloc include/linux/kasan.h:253 [inline]
 slab_post_alloc_hook mm/slab.h:512 [inline]
 slab_alloc_node mm/slub.c:2981 [inline]
 slab_alloc mm/slub.c:2989 [inline]
 kmem_cache_alloc+0x216/0x3a0 mm/slub.c:2994
 getname_flags.part.0+0x50/0x4f0 fs/namei.c:138
 getname_flags fs/namei.c:2747 [inline]
 user_path_at_empty+0xa1/0x100 fs/namei.c:2747
 user_path_at include/linux/namei.h:57 [inline]
 vfs_statx+0x142/0x390 fs/stat.c:203
 vfs_fstatat fs/stat.c:225 [inline]
 vfs_lstat include/linux/fs.h:3386 [inline]
 __do_sys_newlstat+0x91/0x110 fs/stat.c:380
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Memory state around the buggy address:
 ffff8880328cf080: fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc fc
 ffff8880328cf100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff8880328cf180: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                                           ^
 ffff8880328cf200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff8880328cf280: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syz...@go....

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

[tipc-discussion] [PATCH net] tipc: fix implicit-connect for SYN+

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

For implicit-connect, when it's either SYN- or SYN+, an ACK should
be sent back to the client immediately. It's not appropriate for
the client to enter established state only after receiving data
from the server.

On client side, after the SYN is sent out, tipc_wait_for_connect()
should be called to wait for the ACK if timeout is set.

This patch also restricts __tipc_sendstream() to call __sendmsg()
only when it's in TIPC_OPEN state, so that the client can program
in a single loop doing both connecting and data sending like:

  for (...)
      sendmsg(dest, buf);

This makes the implicit-connect more implicit.

Fixes: b97bf3fd8f6a ("[TIPC] Initial merge")
Signed-off-by: Xin Long <luc...@gm...>
Acked-by: Jon Maloy <jm...@re...>
---
 net/tipc/socket.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 34a97ea36cc8..ebd300c26a44 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -158,6 +158,7 @@ static void tipc_sk_remove(struct tipc_sock *tsk);
 static int __tipc_sendstream(struct socket *sock, struct msghdr *m, size_t dsz);
 static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dsz);
 static void tipc_sk_push_backlog(struct tipc_sock *tsk, bool nagle_ack);
+static int tipc_wait_for_connect(struct socket *sock, long *timeo_p);
 
 static const struct proto_ops packet_ops;
 static const struct proto_ops stream_ops;
@@ -1515,8 +1516,13 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 		rc = 0;
 	}
 
-	if (unlikely(syn && !rc))
+	if (unlikely(syn && !rc)) {
 		tipc_set_sk_state(sk, TIPC_CONNECTING);
+		if (timeout) {
+			timeout = msecs_to_jiffies(timeout);
+			tipc_wait_for_connect(sock, &timeout);
+		}
+	}
 
 	return rc ? rc : dlen;
 }
@@ -1564,7 +1570,7 @@ static int __tipc_sendstream(struct socket *sock, struct msghdr *m, size_t dlen)
 		return -EMSGSIZE;
 
 	/* Handle implicit connection setup */
-	if (unlikely(dest)) {
+	if (unlikely(dest && sk->sk_state == TIPC_OPEN)) {
 		rc = __tipc_sendmsg(sock, m, dlen);
 		if (dlen && dlen == rc) {
 			tsk->peer_caps = tipc_node_get_capabilities(net, dnode);
@@ -2689,9 +2695,10 @@ static int tipc_accept(struct socket *sock, struct socket *new_sock, int flags,
 		       bool kern)
 {
 	struct sock *new_sk, *sk = sock->sk;
-	struct sk_buff *buf;
 	struct tipc_sock *new_tsock;
+	struct msghdr m = {NULL,};
 	struct tipc_msg *msg;
+	struct sk_buff *buf;
 	long timeo;
 	int res;
 
@@ -2737,19 +2744,17 @@ static int tipc_accept(struct socket *sock, struct socket *new_sock, int flags,
 	}
 
 	/*
-	 * Respond to 'SYN-' by discarding it & returning 'ACK'-.
-	 * Respond to 'SYN+' by queuing it on new socket.
+	 * Respond to 'SYN-' by discarding it & returning 'ACK'.
+	 * Respond to 'SYN+' by queuing it on new socket & returning 'ACK'.
 	 */
 	if (!msg_data_sz(msg)) {
-		struct msghdr m = {NULL,};
-
 		tsk_advance_rx_queue(sk);
-		__tipc_sendstream(new_sock, &m, 0);
 	} else {
 		__skb_dequeue(&sk->sk_receive_queue);
 		__skb_queue_head(&new_sk->sk_receive_queue, buf);
 		skb_set_owner_r(buf, new_sk);
 	}
+	__tipc_sendstream(new_sock, &m, 0);
 	release_sock(new_sk);
 exit:
 	release_sock(sk);
-- 
2.27.0

Re: [tipc-discussion] [PATCH net] tipc: fix implicit-connect for SYN+

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

On 10/07/2021 12:46, Xin Long wrote:
> For implicit-connect, when it's either SYN- or SYN+, an ACK should
> be sent back to the client immediately. It's not appropriate for
> the client to enter established state only after receiving data
> from the server.
> 
> On client side, after the SYN is sent out, tipc_wait_for_connect()
> should be called to wait for the ACK if timeout is set.
> 
> This patch also restricts __tipc_sendstream() to call __sendmsg()
> only when it's in TIPC_OPEN state, so that the client can program
> in a single loop doing both connecting and data sending like:
> 
>    for (...)
>        sendmsg(dest, buf);
> 
> This makes the implicit-connect more implicit.
> 
> Fixes: b97bf3fd8f6a ("[TIPC] Initial merge")
> Signed-off-by: Xin Long <luc...@gm...>
> ---
>   net/tipc/socket.c | 21 +++++++++++++--------
>   1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 34a97ea36cc8..ebd300c26a44 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -158,6 +158,7 @@ static void tipc_sk_remove(struct tipc_sock *tsk);
>   static int __tipc_sendstream(struct socket *sock, struct msghdr *m, size_t dsz);
>   static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dsz);
>   static void tipc_sk_push_backlog(struct tipc_sock *tsk, bool nagle_ack);
> +static int tipc_wait_for_connect(struct socket *sock, long *timeo_p);
>   
>   static const struct proto_ops packet_ops;
>   static const struct proto_ops stream_ops;
> @@ -1515,8 +1516,13 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
>   		rc = 0;
>   	}
>   
> -	if (unlikely(syn && !rc))
> +	if (unlikely(syn && !rc)) {
>   		tipc_set_sk_state(sk, TIPC_CONNECTING);
> +		if (timeout) {
> +			timeout = msecs_to_jiffies(timeout);
> +			tipc_wait_for_connect(sock, &timeout);
> +		}
> +	}
>   
>   	return rc ? rc : dlen;
>   }
> @@ -1564,7 +1570,7 @@ static int __tipc_sendstream(struct socket *sock, struct msghdr *m, size_t dlen)
>   		return -EMSGSIZE;
>   
>   	/* Handle implicit connection setup */
> -	if (unlikely(dest)) {
> +	if (unlikely(dest && sk->sk_state == TIPC_OPEN)) {
>   		rc = __tipc_sendmsg(sock, m, dlen);
>   		if (dlen && dlen == rc) {
>   			tsk->peer_caps = tipc_node_get_capabilities(net, dnode);
> @@ -2689,9 +2695,10 @@ static int tipc_accept(struct socket *sock, struct socket *new_sock, int flags,
>   		       bool kern)
>   {
>   	struct sock *new_sk, *sk = sock->sk;
> -	struct sk_buff *buf;
>   	struct tipc_sock *new_tsock;
> +	struct msghdr m = {NULL,};
>   	struct tipc_msg *msg;
> +	struct sk_buff *buf;
>   	long timeo;
>   	int res;
>   
> @@ -2737,19 +2744,17 @@ static int tipc_accept(struct socket *sock, struct socket *new_sock, int flags,
>   	}
>   
>   	/*
> -	 * Respond to 'SYN-' by discarding it & returning 'ACK'-.
> -	 * Respond to 'SYN+' by queuing it on new socket.
> +	 * Respond to 'SYN-' by discarding it & returning 'ACK'.
> +	 * Respond to 'SYN+' by queuing it on new socket & returning 'ACK'.
>   	 */
>   	if (!msg_data_sz(msg)) {
> -		struct msghdr m = {NULL,};
> -
>   		tsk_advance_rx_queue(sk);
> -		__tipc_sendstream(new_sock, &m, 0);
>   	} else {
>   		__skb_dequeue(&sk->sk_receive_queue);
>   		__skb_queue_head(&new_sk->sk_receive_queue, buf);
>   		skb_set_owner_r(buf, new_sk);
>   	}
> +	__tipc_sendstream(new_sock, &m, 0);
>   	release_sock(new_sk);
>   exit:
>   	release_sock(sk);
> 
Acked by: Jon Maloy <jm...@re...>

Re: [tipc-discussion] [net] tipc: fix sleeping in tipc accept routine

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

On 21/07/2021 05:13, Hoang Le wrote:
> The release_sock() is blocking function, it would change the state
> after sleeping. In order to evaluate the stated condition outside
> the socket lock context, switch to use wait_woken() instead.
> 
> Signed-off-by: Hoang Le <hoa...@de...>
> ---
>   net/tipc/socket.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 9b0b311c7ec1..2c71828b7e5c 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -2664,7 +2664,7 @@ static int tipc_listen(struct socket *sock, int len)
>   static int tipc_wait_for_accept(struct socket *sock, long timeo)
>   {
>   	struct sock *sk = sock->sk;
> -	DEFINE_WAIT(wait);
> +	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>   	int err;
>   
>   	/* True wake-one mechanism for incoming connections: only
> @@ -2673,12 +2673,12 @@ static int tipc_wait_for_accept(struct socket *sock, long timeo)
>   	 * anymore, the common case will execute the loop only once.
>   	*/
>   	for (;;) {
> -		prepare_to_wait_exclusive(sk_sleep(sk), &wait,
> -					  TASK_INTERRUPTIBLE);
>   		if (timeo && skb_queue_empty(&sk->sk_receive_queue)) {
> +			add_wait_queue(sk_sleep(sk), &wait);
>   			release_sock(sk);
> -			timeo = schedule_timeout(timeo);
> +			timeo = wait_woken(&wait, TASK_INTERRUPTIBLE, timeo);
>   			lock_sock(sk);
> +			remove_wait_queue(sk_sleep(sk), &wait);
>   		}
>   		err = 0;
>   		if (!skb_queue_empty(&sk->sk_receive_queue))
> @@ -2690,7 +2690,6 @@ static int tipc_wait_for_accept(struct socket *sock, long timeo)
>   		if (signal_pending(current))
>   			break;
>   	}
> -	finish_wait(sk_sleep(sk), &wait);
>   	return err;
>   }
>   
> 
Acked-by: Jon Maloy <jm...@re...>

[tipc-discussion] [net] tipc: fix sleeping in tipc accept routine

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

The release_sock() is blocking function, it would change the state
after sleeping. In order to evaluate the stated condition outside
the socket lock context, switch to use wait_woken() instead.

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

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 9b0b311c7ec1..2c71828b7e5c 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2664,7 +2664,7 @@ static int tipc_listen(struct socket *sock, int len)
 static int tipc_wait_for_accept(struct socket *sock, long timeo)
 {
 	struct sock *sk = sock->sk;
-	DEFINE_WAIT(wait);
+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
 	int err;
 
 	/* True wake-one mechanism for incoming connections: only
@@ -2673,12 +2673,12 @@ static int tipc_wait_for_accept(struct socket *sock, long timeo)
 	 * anymore, the common case will execute the loop only once.
 	*/
 	for (;;) {
-		prepare_to_wait_exclusive(sk_sleep(sk), &wait,
-					  TASK_INTERRUPTIBLE);
 		if (timeo && skb_queue_empty(&sk->sk_receive_queue)) {
+			add_wait_queue(sk_sleep(sk), &wait);
 			release_sock(sk);
-			timeo = schedule_timeout(timeo);
+			timeo = wait_woken(&wait, TASK_INTERRUPTIBLE, timeo);
 			lock_sock(sk);
+			remove_wait_queue(sk_sleep(sk), &wait);
 		}
 		err = 0;
 		if (!skb_queue_empty(&sk->sk_receive_queue))
@@ -2690,7 +2690,6 @@ static int tipc_wait_for_accept(struct socket *sock, long timeo)
 		if (signal_pending(current))
 			break;
 	}
-	finish_wait(sk_sleep(sk), &wait);
 	return err;
 }
 
-- 
2.30.2

[tipc-discussion] [PATCH net-next] tipc: keep the skb in rcv queue until the whole data is read

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

Currently, when userspace reads a datagram with a buffer that is
smaller than this datagram, the data will be truncated and only
part of it can be received by users. It doesn't seem right that
users don't know the datagram size and have to use a huge buffer
to read it to avoid the truncation.

This patch to fix it by keeping the skb in rcv queue until the
whole data is read by users. Only the last msg of the datagram
will be marked with MSG_EOR, just as TCP/SCTP does.

Note that this will work as above only when MSG_EOR is set in the
flags parameter of recvmsg(), so that it won't break any old user
applications.

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

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 34a97ea36cc8..9b0b311c7ec1 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1880,6 +1880,7 @@ static int tipc_recvmsg(struct socket *sock, struct msghdr *m,
 	bool connected = !tipc_sk_type_connectionless(sk);
 	struct tipc_sock *tsk = tipc_sk(sk);
 	int rc, err, hlen, dlen, copy;
+	struct tipc_skb_cb *skb_cb;
 	struct sk_buff_head xmitq;
 	struct tipc_msg *hdr;
 	struct sk_buff *skb;
@@ -1903,6 +1904,7 @@ static int tipc_recvmsg(struct socket *sock, struct msghdr *m,
 		if (unlikely(rc))
 			goto exit;
 		skb = skb_peek(&sk->sk_receive_queue);
+		skb_cb = TIPC_SKB_CB(skb);
 		hdr = buf_msg(skb);
 		dlen = msg_data_sz(hdr);
 		hlen = msg_hdr_sz(hdr);
@@ -1922,18 +1924,33 @@ static int tipc_recvmsg(struct socket *sock, struct msghdr *m,
 
 	/* Capture data if non-error msg, otherwise just set return value */
 	if (likely(!err)) {
-		copy = min_t(int, dlen, buflen);
-		if (unlikely(copy != dlen))
-			m->msg_flags |= MSG_TRUNC;
-		rc = skb_copy_datagram_msg(skb, hlen, m, copy);
+		int offset = skb_cb->bytes_read;
+
+		copy = min_t(int, dlen - offset, buflen);
+		rc = skb_copy_datagram_msg(skb, hlen + offset, m, copy);
+		if (unlikely(rc))
+			goto exit;
+		if (unlikely(offset + copy < dlen)) {
+			if (flags & MSG_EOR) {
+				if (!(flags & MSG_PEEK))
+					skb_cb->bytes_read = offset + copy;
+			} else {
+				m->msg_flags |= MSG_TRUNC;
+				skb_cb->bytes_read = 0;
+			}
+		} else {
+			if (flags & MSG_EOR)
+				m->msg_flags |= MSG_EOR;
+			skb_cb->bytes_read = 0;
+		}
 	} else {
 		copy = 0;
 		rc = 0;
-		if (err != TIPC_CONN_SHUTDOWN && connected && !m->msg_control)
+		if (err != TIPC_CONN_SHUTDOWN && connected && !m->msg_control) {
 			rc = -ECONNRESET;
+			goto exit;
+		}
 	}
-	if (unlikely(rc))
-		goto exit;
 
 	/* Mark message as group event if applicable */
 	if (unlikely(grp_evt)) {
@@ -1956,9 +1973,10 @@ static int tipc_recvmsg(struct socket *sock, struct msghdr *m,
 		tipc_node_distr_xmit(sock_net(sk), &xmitq);
 	}
 
-	tsk_advance_rx_queue(sk);
+	if (!skb_cb->bytes_read)
+		tsk_advance_rx_queue(sk);
 
-	if (likely(!connected))
+	if (likely(!connected) || skb_cb->bytes_read)
 		goto exit;
 
 	/* Send connection flow control advertisement when applicable */
-- 
2.27.0

[tipc-discussion] [PATCH net] tipc: fix implicit-connect for SYN+

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

For implicit-connect, when it's either SYN- or SYN+, an ACK should
be sent back to the client immediately. It's not appropriate for
the client to enter established state only after receiving data
from the server.

On client side, after the SYN is sent out, tipc_wait_for_connect()
should be called to wait for the ACK if timeout is set.

This patch also restricts __tipc_sendstream() to call __sendmsg()
only when it's in TIPC_OPEN state, so that the client can program
in a single loop doing both connecting and data sending like:

  for (...)
      sendmsg(dest, buf);

This makes the implicit-connect more implicit.

Fixes: b97bf3fd8f6a ("[TIPC] Initial merge")
Signed-off-by: Xin Long <luc...@gm...>
---
 net/tipc/socket.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 34a97ea36cc8..ebd300c26a44 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -158,6 +158,7 @@ static void tipc_sk_remove(struct tipc_sock *tsk);
 static int __tipc_sendstream(struct socket *sock, struct msghdr *m, size_t dsz);
 static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dsz);
 static void tipc_sk_push_backlog(struct tipc_sock *tsk, bool nagle_ack);
+static int tipc_wait_for_connect(struct socket *sock, long *timeo_p);
 
 static const struct proto_ops packet_ops;
 static const struct proto_ops stream_ops;
@@ -1515,8 +1516,13 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 		rc = 0;
 	}
 
-	if (unlikely(syn && !rc))
+	if (unlikely(syn && !rc)) {
 		tipc_set_sk_state(sk, TIPC_CONNECTING);
+		if (timeout) {
+			timeout = msecs_to_jiffies(timeout);
+			tipc_wait_for_connect(sock, &timeout);
+		}
+	}
 
 	return rc ? rc : dlen;
 }
@@ -1564,7 +1570,7 @@ static int __tipc_sendstream(struct socket *sock, struct msghdr *m, size_t dlen)
 		return -EMSGSIZE;
 
 	/* Handle implicit connection setup */
-	if (unlikely(dest)) {
+	if (unlikely(dest && sk->sk_state == TIPC_OPEN)) {
 		rc = __tipc_sendmsg(sock, m, dlen);
 		if (dlen && dlen == rc) {
 			tsk->peer_caps = tipc_node_get_capabilities(net, dnode);
@@ -2689,9 +2695,10 @@ static int tipc_accept(struct socket *sock, struct socket *new_sock, int flags,
 		       bool kern)
 {
 	struct sock *new_sk, *sk = sock->sk;
-	struct sk_buff *buf;
 	struct tipc_sock *new_tsock;
+	struct msghdr m = {NULL,};
 	struct tipc_msg *msg;
+	struct sk_buff *buf;
 	long timeo;
 	int res;
 
@@ -2737,19 +2744,17 @@ static int tipc_accept(struct socket *sock, struct socket *new_sock, int flags,
 	}
 
 	/*
-	 * Respond to 'SYN-' by discarding it & returning 'ACK'-.
-	 * Respond to 'SYN+' by queuing it on new socket.
+	 * Respond to 'SYN-' by discarding it & returning 'ACK'.
+	 * Respond to 'SYN+' by queuing it on new socket & returning 'ACK'.
 	 */
 	if (!msg_data_sz(msg)) {
-		struct msghdr m = {NULL,};
-
 		tsk_advance_rx_queue(sk);
-		__tipc_sendstream(new_sock, &m, 0);
 	} else {
 		__skb_dequeue(&sk->sk_receive_queue);
 		__skb_queue_head(&new_sk->sk_receive_queue, buf);
 		skb_set_owner_r(buf, new_sk);
 	}
+	__tipc_sendstream(new_sock, &m, 0);
 	release_sock(new_sk);
 exit:
 	release_sock(sk);
-- 
2.27.0