[PATCH net] net: adjust skb->truesize in ___pskb_trim()

[PATCH net] net: adjust skb->truesize in ___pskb_trim()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Andrey found a way to trigger the WARN_ON_ONCE(delta < len) in
skb_try_coalesce() using syzkaller and a filter attached to a TCP
socket.

As we did recently in commit 158f323b9868 ("net: adjust skb->truesize in
pskb_expand_head()") we can adjust skb->truesize from ___pskb_trim(),
via a call to skb_condense().

If all frags were freed, then skb->truesize can be recomputed.

This call can be done if skb is not yet owned, or destructor is
sock_edemux().

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Andrey Konovalov <andreyknvl@google.com>
Cc: Willem de Bruijn <willemb@google.com>
---
 net/core/skbuff.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f86bf69cfb8d8bc17262cdba5d9f57a4726cd476..f1d04592ace02f32efa6e05df89c9a5e0023157f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1576,6 +1576,8 @@ int ___pskb_trim(struct sk_buff *skb, unsigned int len)
 		skb_set_tail_pointer(skb, len);
 	}
 
+	if (!skb->sk || skb->destructor == sock_edemux)
+		skb_condense(skb);
 	return 0;
 }
 EXPORT_SYMBOL(___pskb_trim);

Re: [PATCH net] net: adjust skb->truesize in ___pskb_trim()

[Andrey Konovalov]

On Wed, Apr 26, 2017 at 6:07 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Andrey found a way to trigger the WARN_ON_ONCE(delta < len) in
> skb_try_coalesce() using syzkaller and a filter attached to a TCP
> socket.
>
> As we did recently in commit 158f323b9868 ("net: adjust skb->truesize in
> pskb_expand_head()") we can adjust skb->truesize from ___pskb_trim(),
> via a call to skb_condense().
>
> If all frags were freed, then skb->truesize can be recomputed.
>
> This call can be done if skb is not yet owned, or destructor is
> sock_edemux().

Hi Eric,

I still see the warning even with your patch.

Thanks!

>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Andrey Konovalov <andreyknvl@google.com>
> Cc: Willem de Bruijn <willemb@google.com>
> ---
>  net/core/skbuff.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index f86bf69cfb8d8bc17262cdba5d9f57a4726cd476..f1d04592ace02f32efa6e05df89c9a5e0023157f 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1576,6 +1576,8 @@ int ___pskb_trim(struct sk_buff *skb, unsigned int len)
>                 skb_set_tail_pointer(skb, len);
>         }
>
> +       if (!skb->sk || skb->destructor == sock_edemux)
> +               skb_condense(skb);
>         return 0;
>  }
>  EXPORT_SYMBOL(___pskb_trim);
>
>

[PATCH net] tcp: do not underestimate skb->truesize in tcp_trim_head()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Andrey found a way to trigger the WARN_ON_ONCE(delta < len) in
skb_try_coalesce() using syzkaller and a filter attached to a TCP
socket over loopback interface.

I believe one issue with looped skbs is that tcp_trim_head() can end up
producing skb with under estimated truesize.

It hardly matters for normal conditions, since packets sent over
loopback are never truncated.

Bytes trimmed from skb->head should not change skb truesize, since
skb->head is not reallocated.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Andrey Konovalov <andreyknvl@google.com>
---
 net/ipv4/tcp_output.c |   19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index c3c082ed38796f65948e7a1042e37813b71d5abf..a85d863c44196e60fd22e25471cf773e72d2c133 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1267,7 +1267,7 @@ int tcp_fragment(struct sock *sk, struct sk_buff *skb, u32 len,
  * eventually). The difference is that pulled data not copied, but
  * immediately discarded.
  */
-static void __pskb_trim_head(struct sk_buff *skb, int len)
+static int __pskb_trim_head(struct sk_buff *skb, int len)
 {
 	struct skb_shared_info *shinfo;
 	int i, k, eat;
@@ -1277,7 +1277,7 @@ static void __pskb_trim_head(struct sk_buff *skb, int len)
 		__skb_pull(skb, eat);
 		len -= eat;
 		if (!len)
-			return;
+			return 0;
 	}
 	eat = len;
 	k = 0;
@@ -1303,23 +1303,28 @@ static void __pskb_trim_head(struct sk_buff *skb, int len)
 	skb_reset_tail_pointer(skb);
 	skb->data_len -= len;
 	skb->len = skb->data_len;
+	return len;
 }
 
 /* Remove acked data from a packet in the transmit queue. */
 int tcp_trim_head(struct sock *sk, struct sk_buff *skb, u32 len)
 {
+	u32 delta_truesize;
+
 	if (skb_unclone(skb, GFP_ATOMIC))
 		return -ENOMEM;
 
-	__pskb_trim_head(skb, len);
+	delta_truesize = __pskb_trim_head(skb, len);
 
 	TCP_SKB_CB(skb)->seq += len;
 	skb->ip_summed = CHECKSUM_PARTIAL;
 
-	skb->truesize	     -= len;
-	sk->sk_wmem_queued   -= len;
-	sk_mem_uncharge(sk, len);
-	sock_set_flag(sk, SOCK_QUEUE_SHRUNK);
+	if (delta_truesize) {
+		skb->truesize	   -= delta_truesize;
+		sk->sk_wmem_queued -= delta_truesize;
+		sk_mem_uncharge(sk, delta_truesize);
+		sock_set_flag(sk, SOCK_QUEUE_SHRUNK);
+	}
 
 	/* Any change of skb->len requires recalculation of tso factor. */
 	if (tcp_skb_pcount(skb) > 1)

Re: [PATCH net] tcp: do not underestimate skb->truesize in tcp_trim_head()

[Andrey Konovalov]

On Thu, Apr 27, 2017 at 2:15 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Andrey found a way to trigger the WARN_ON_ONCE(delta < len) in
> skb_try_coalesce() using syzkaller and a filter attached to a TCP
> socket over loopback interface.
>
> I believe one issue with looped skbs is that tcp_trim_head() can end up
> producing skb with under estimated truesize.
>
> It hardly matters for normal conditions, since packets sent over
> loopback are never truncated.
>
> Bytes trimmed from skb->head should not change skb truesize, since
> skb->head is not reallocated.

Hi Eric,

With all 3 of your patches applied to net-next I don't see the warning any more.

Thanks!

Tested-by: Andrey Konovalov <andreyknvl@google.com>

>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Andrey Konovalov <andreyknvl@google.com>
> ---
>  net/ipv4/tcp_output.c |   19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index c3c082ed38796f65948e7a1042e37813b71d5abf..a85d863c44196e60fd22e25471cf773e72d2c133 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1267,7 +1267,7 @@ int tcp_fragment(struct sock *sk, struct sk_buff *skb, u32 len,
>   * eventually). The difference is that pulled data not copied, but
>   * immediately discarded.
>   */
> -static void __pskb_trim_head(struct sk_buff *skb, int len)
> +static int __pskb_trim_head(struct sk_buff *skb, int len)
>  {
>         struct skb_shared_info *shinfo;
>         int i, k, eat;
> @@ -1277,7 +1277,7 @@ static void __pskb_trim_head(struct sk_buff *skb, int len)
>                 __skb_pull(skb, eat);
>                 len -= eat;
>                 if (!len)
> -                       return;
> +                       return 0;
>         }
>         eat = len;
>         k = 0;
> @@ -1303,23 +1303,28 @@ static void __pskb_trim_head(struct sk_buff *skb, int len)
>         skb_reset_tail_pointer(skb);
>         skb->data_len -= len;
>         skb->len = skb->data_len;
> +       return len;
>  }
>
>  /* Remove acked data from a packet in the transmit queue. */
>  int tcp_trim_head(struct sock *sk, struct sk_buff *skb, u32 len)
>  {
> +       u32 delta_truesize;
> +
>         if (skb_unclone(skb, GFP_ATOMIC))
>                 return -ENOMEM;
>
> -       __pskb_trim_head(skb, len);
> +       delta_truesize = __pskb_trim_head(skb, len);
>
>         TCP_SKB_CB(skb)->seq += len;
>         skb->ip_summed = CHECKSUM_PARTIAL;
>
> -       skb->truesize        -= len;
> -       sk->sk_wmem_queued   -= len;
> -       sk_mem_uncharge(sk, len);
> -       sock_set_flag(sk, SOCK_QUEUE_SHRUNK);
> +       if (delta_truesize) {
> +               skb->truesize      -= delta_truesize;
> +               sk->sk_wmem_queued -= delta_truesize;
> +               sk_mem_uncharge(sk, delta_truesize);
> +               sock_set_flag(sk, SOCK_QUEUE_SHRUNK);
> +       }
>
>         /* Any change of skb->len requires recalculation of tso factor. */
>         if (tcp_skb_pcount(skb) > 1)
>
>

Re: [PATCH net] net: adjust skb->truesize in ___pskb_trim()

[Willem de Bruijn]

On Wed, Apr 26, 2017 at 1:08 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
> On Wed, Apr 26, 2017 at 6:07 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> From: Eric Dumazet <edumazet@google.com>
>>
>> Andrey found a way to trigger the WARN_ON_ONCE(delta < len) in
>> skb_try_coalesce() using syzkaller and a filter attached to a TCP
>> socket.
>>
>> As we did recently in commit 158f323b9868 ("net: adjust skb->truesize in
>> pskb_expand_head()") we can adjust skb->truesize from ___pskb_trim(),
>> via a call to skb_condense().
>>
>> If all frags were freed, then skb->truesize can be recomputed.
>>
>> This call can be done if skb is not yet owned, or destructor is
>> sock_edemux().
>
> Hi Eric,
>
> I still see the warning even with your patch.

Can this happen if sk_trim_filter_cap trims the skb to free some,
but not all, of the frags? If skb->data_len remains larger than
skb->end - skb->tail, skb_condense will not adjust the truesize.

Re: [PATCH net] tcp: do not underestimate skb->truesize in tcp_trim_head()

[David Miller]

From: Andrey Konovalov <andreyknvl@google.com>
Date: Thu, 27 Apr 2017 13:49:57 +0200

> On Thu, Apr 27, 2017 at 2:15 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> From: Eric Dumazet <edumazet@google.com>
>>
>> Andrey found a way to trigger the WARN_ON_ONCE(delta < len) in
>> skb_try_coalesce() using syzkaller and a filter attached to a TCP
>> socket over loopback interface.
>>
>> I believe one issue with looped skbs is that tcp_trim_head() can end up
>> producing skb with under estimated truesize.
>>
>> It hardly matters for normal conditions, since packets sent over
>> loopback are never truncated.
>>
>> Bytes trimmed from skb->head should not change skb truesize, since
>> skb->head is not reallocated.
> 
> Hi Eric,
> 
> With all 3 of your patches applied to net-next I don't see the warning any more.
> 
> Thanks!
> 
> Tested-by: Andrey Konovalov <andreyknvl@google.com>

Applied and queued up for -stable, thanks Eric.

Re: [PATCH net] net: adjust skb->truesize in ___pskb_trim()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 26 Apr 2017 09:07:46 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> Andrey found a way to trigger the WARN_ON_ONCE(delta < len) in
> skb_try_coalesce() using syzkaller and a filter attached to a TCP
> socket.
> 
> As we did recently in commit 158f323b9868 ("net: adjust skb->truesize in
> pskb_expand_head()") we can adjust skb->truesize from ___pskb_trim(),
> via a call to skb_condense().
> 
> If all frags were freed, then skb->truesize can be recomputed.
> 
> This call can be done if skb is not yet owned, or destructor is
> sock_edemux().
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Andrey Konovalov <andreyknvl@google.com>

Also applied, thanks Eric.