[PATCH net-next] bpf: restore skb->sk before pskb_trim() call

[PATCH net-next] bpf: restore skb->sk before pskb_trim() call

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

While testing a fix [1] in ___pskb_trim(), addressing the WARN_ON_ONCE()
in skb_try_coalesce() reported by Andrey, I found that we had an skb
with skb->sk set but no skb->destructor.

This invalidated heuristic found in commit 158f323b9868 ("net: adjust
skb->truesize in pskb_expand_head()") and in cited patch.

Considering the BUG_ON(skb->sk) we have in skb_orphan(), we should
restrain the temporary setting to a minimal section.

[1] https://patchwork.ozlabs.org/patch/755570/ 
    net: adjust skb->truesize in ___pskb_trim()

Fixes: 8f917bba0042 ("bpf: pass sk to helper functions")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
---
 net/core/filter.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 9a37860a80fc78378705b681ec3b0468824cbcf4..a253a6197e6b37a7ae2fe451c646b01c861a3e22 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -98,8 +98,8 @@ int sk_filter_trim_cap(struct sock *sk, struct sk_buff *skb, unsigned int cap)
 
 		skb->sk = sk;
 		pkt_len = bpf_prog_run_save_cb(filter->prog, skb);
-		err = pkt_len ? pskb_trim(skb, max(cap, pkt_len)) : -EPERM;
 		skb->sk = save_sk;
+		err = pkt_len ? pskb_trim(skb, max(cap, pkt_len)) : -EPERM;
 	}
 	rcu_read_unlock();
 

Re: [PATCH net-next] bpf: restore skb->sk before pskb_trim() call

[Daniel Borkmann]

On 04/26/2017 06:09 PM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> While testing a fix [1] in ___pskb_trim(), addressing the WARN_ON_ONCE()
> in skb_try_coalesce() reported by Andrey, I found that we had an skb
> with skb->sk set but no skb->destructor.
>
> This invalidated heuristic found in commit 158f323b9868 ("net: adjust
> skb->truesize in pskb_expand_head()") and in cited patch.
>
> Considering the BUG_ON(skb->sk) we have in skb_orphan(), we should
> restrain the temporary setting to a minimal section.
>
> [1] https://patchwork.ozlabs.org/patch/755570/
>      net: adjust skb->truesize in ___pskb_trim()
>
> Fixes: 8f917bba0042 ("bpf: pass sk to helper functions")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Willem de Bruijn <willemb@google.com>
> Cc: Andrey Konovalov <andreyknvl@google.com>

Good point, thanks!

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [PATCH net-next] bpf: restore skb->sk before pskb_trim() call

[Alexei Starovoitov]

On Wed, Apr 26, 2017 at 09:09:23AM -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> While testing a fix [1] in ___pskb_trim(), addressing the WARN_ON_ONCE()
> in skb_try_coalesce() reported by Andrey, I found that we had an skb
> with skb->sk set but no skb->destructor.
> 
> This invalidated heuristic found in commit 158f323b9868 ("net: adjust
> skb->truesize in pskb_expand_head()") and in cited patch.
> 
> Considering the BUG_ON(skb->sk) we have in skb_orphan(), we should
> restrain the temporary setting to a minimal section.
> 
> [1] https://patchwork.ozlabs.org/patch/755570/ 
>     net: adjust skb->truesize in ___pskb_trim()
> 
> Fixes: 8f917bba0042 ("bpf: pass sk to helper functions")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Willem de Bruijn <willemb@google.com>
> Cc: Andrey Konovalov <andreyknvl@google.com>

Ahh. Thanks for the fix.
Acked-by: Alexei Starovoitov <ast@kernel.org>

Re: [PATCH net-next] bpf: restore skb->sk before pskb_trim() call

[Willem de Bruijn]

On Wed, Apr 26, 2017 at 4:53 PM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Wed, Apr 26, 2017 at 09:09:23AM -0700, Eric Dumazet wrote:
>> From: Eric Dumazet <edumazet@google.com>
>>
>> While testing a fix [1] in ___pskb_trim(), addressing the WARN_ON_ONCE()
>> in skb_try_coalesce() reported by Andrey, I found that we had an skb
>> with skb->sk set but no skb->destructor.
>>
>> This invalidated heuristic found in commit 158f323b9868 ("net: adjust
>> skb->truesize in pskb_expand_head()") and in cited patch.
>>
>> Considering the BUG_ON(skb->sk) we have in skb_orphan(), we should
>> restrain the temporary setting to a minimal section.
>>
>> [1] https://patchwork.ozlabs.org/patch/755570/
>>     net: adjust skb->truesize in ___pskb_trim()
>>
>> Fixes: 8f917bba0042 ("bpf: pass sk to helper functions")
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Cc: Willem de Bruijn <willemb@google.com>
>> Cc: Andrey Konovalov <andreyknvl@google.com>
>
> Ahh. Thanks for the fix.
> Acked-by: Alexei Starovoitov <ast@kernel.org>

Acked-by: Willem de Bruijn <willemb@google.com>

Thanks, Eric.

Re: [PATCH net-next] bpf: restore skb->sk before pskb_trim() call

[David Miller]

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

> From: Eric Dumazet <edumazet@google.com>
> 
> While testing a fix [1] in ___pskb_trim(), addressing the WARN_ON_ONCE()
> in skb_try_coalesce() reported by Andrey, I found that we had an skb
> with skb->sk set but no skb->destructor.
> 
> This invalidated heuristic found in commit 158f323b9868 ("net: adjust
> skb->truesize in pskb_expand_head()") and in cited patch.
> 
> Considering the BUG_ON(skb->sk) we have in skb_orphan(), we should
> restrain the temporary setting to a minimal section.
> 
> [1] https://patchwork.ozlabs.org/patch/755570/ 
>     net: adjust skb->truesize in ___pskb_trim()
> 
> Fixes: 8f917bba0042 ("bpf: pass sk to helper functions")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, thanks Eric.