* [PATCH net] net: adjust skb->truesize in ___pskb_trim()
@ 2017-04-26 16:07 Eric Dumazet
2017-04-26 17:08 ` Andrey Konovalov
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Eric Dumazet @ 2017-04-26 16:07 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Andrey Konovalov, Willem de Bruijn
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);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net] net: adjust skb->truesize in ___pskb_trim()
2017-04-26 16:07 [PATCH net] net: adjust skb->truesize in ___pskb_trim() Eric Dumazet
@ 2017-04-26 17:08 ` Andrey Konovalov
2017-04-27 18:43 ` Willem de Bruijn
2017-04-27 0:15 ` [PATCH net] tcp: do not underestimate skb->truesize in tcp_trim_head() Eric Dumazet
2017-04-28 20:07 ` [PATCH net] net: adjust skb->truesize in ___pskb_trim() David Miller
2 siblings, 1 reply; 7+ messages in thread
From: Andrey Konovalov @ 2017-04-26 17:08 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Willem de Bruijn
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);
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net] tcp: do not underestimate skb->truesize in tcp_trim_head()
2017-04-26 16:07 [PATCH net] net: adjust skb->truesize in ___pskb_trim() Eric Dumazet
2017-04-26 17:08 ` Andrey Konovalov
@ 2017-04-27 0:15 ` Eric Dumazet
2017-04-27 11:49 ` Andrey Konovalov
2017-04-28 20:07 ` [PATCH net] net: adjust skb->truesize in ___pskb_trim() David Miller
2 siblings, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2017-04-27 0:15 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Andrey Konovalov
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)
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net] tcp: do not underestimate skb->truesize in tcp_trim_head()
2017-04-27 0:15 ` [PATCH net] tcp: do not underestimate skb->truesize in tcp_trim_head() Eric Dumazet
@ 2017-04-27 11:49 ` Andrey Konovalov
2017-04-28 20:05 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: Andrey Konovalov @ 2017-04-27 11:49 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
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)
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] net: adjust skb->truesize in ___pskb_trim()
2017-04-26 17:08 ` Andrey Konovalov
@ 2017-04-27 18:43 ` Willem de Bruijn
0 siblings, 0 replies; 7+ messages in thread
From: Willem de Bruijn @ 2017-04-27 18:43 UTC (permalink / raw)
To: Andrey Konovalov; +Cc: Eric Dumazet, David Miller, netdev, 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] tcp: do not underestimate skb->truesize in tcp_trim_head()
2017-04-27 11:49 ` Andrey Konovalov
@ 2017-04-28 20:05 ` David Miller
0 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2017-04-28 20:05 UTC (permalink / raw)
To: andreyknvl; +Cc: eric.dumazet, netdev
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] net: adjust skb->truesize in ___pskb_trim()
2017-04-26 16:07 [PATCH net] net: adjust skb->truesize in ___pskb_trim() Eric Dumazet
2017-04-26 17:08 ` Andrey Konovalov
2017-04-27 0:15 ` [PATCH net] tcp: do not underestimate skb->truesize in tcp_trim_head() Eric Dumazet
@ 2017-04-28 20:07 ` David Miller
2 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2017-04-28 20:07 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, andreyknvl, willemb
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-04-28 20:07 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-26 16:07 [PATCH net] net: adjust skb->truesize in ___pskb_trim() Eric Dumazet
2017-04-26 17:08 ` Andrey Konovalov
2017-04-27 18:43 ` Willem de Bruijn
2017-04-27 0:15 ` [PATCH net] tcp: do not underestimate skb->truesize in tcp_trim_head() Eric Dumazet
2017-04-27 11:49 ` Andrey Konovalov
2017-04-28 20:05 ` David Miller
2017-04-28 20:07 ` [PATCH net] net: adjust skb->truesize in ___pskb_trim() David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.