[PATCH] net: don't forget to free sk_filter

[PATCH] net: don't forget to free sk_filter

[Andrey Vagin]

sk_filter isn't freed if bpf_func is equal to sk_run_filter.

This memory leak was introduced by
commit d45ed4a4e33ae103053c0a53d280014e7101bb5c
Author: Alexei Starovoitov <ast@plumgrid.com>
Date:   Fri Oct 4 00:14:06 2013 -0700

    net: fix unsafe set_memory_rw from softirq

Before this patch sk_filter was freed in sk_filter_release_rcu,
now it is freed in bpf_jit_free.

Here is output of kmemleak:
unreferenced object 0xffff8800b774eab0 (size 128):
  comm "systemd", pid 1, jiffies 4294669014 (age 124.062s)
  hex dump (first 32 bytes):
    00 00 00 00 0b 00 00 00 20 63 7f b7 00 88 ff ff  ........ c......
    60 d4 55 81 ff ff ff ff 30 d9 55 81 ff ff ff ff  `.U.....0.U.....
  backtrace:
    [<ffffffff816444be>] kmemleak_alloc+0x4e/0xb0
    [<ffffffff811845af>] __kmalloc+0xef/0x260
    [<ffffffff81534028>] sock_kmalloc+0x38/0x60
    [<ffffffff8155d4dd>] sk_attach_filter+0x5d/0x190
    [<ffffffff815378a1>] sock_setsockopt+0x991/0x9e0
    [<ffffffff81531bd6>] SyS_setsockopt+0xb6/0xd0
    [<ffffffff8165f3e9>] system_call_fastpath+0x16/0x1b
    [<ffffffffffffffff>] 0xffffffffffffffff

Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: stable@vger.kernel.org # 3.12
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 arch/x86/net/bpf_jit_comp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 516593e..3c55de5 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -788,5 +788,6 @@ void bpf_jit_free(struct sk_filter *fp)
 	if (fp->bpf_func != sk_run_filter) {
 		INIT_WORK(&fp->work, bpf_jit_free_deferred);
 		schedule_work(&fp->work);
-	}
+	} else
+		kfree(fp);
 }
-- 
1.8.3.1

Re: [PATCH] net: don't forget to free sk_filter

[Alexei Starovoitov]

On Wed, Nov 6, 2013 at 7:51 AM, Andrey Vagin <avagin@openvz.org> wrote:
> sk_filter isn't freed if bpf_func is equal to sk_run_filter.

good catch! thanks.

> This memory leak was introduced by
> commit d45ed4a4e33ae103053c0a53d280014e7101bb5c
> Author: Alexei Starovoitov <ast@plumgrid.com>
> Date:   Fri Oct 4 00:14:06 2013 -0700
>
>     net: fix unsafe set_memory_rw from softirq
>
> Before this patch sk_filter was freed in sk_filter_release_rcu,
> now it is freed in bpf_jit_free.
>
> Here is output of kmemleak:
> unreferenced object 0xffff8800b774eab0 (size 128):
>   comm "systemd", pid 1, jiffies 4294669014 (age 124.062s)
>   hex dump (first 32 bytes):
>     00 00 00 00 0b 00 00 00 20 63 7f b7 00 88 ff ff  ........ c......
>     60 d4 55 81 ff ff ff ff 30 d9 55 81 ff ff ff ff  `.U.....0.U.....
>   backtrace:
>     [<ffffffff816444be>] kmemleak_alloc+0x4e/0xb0
>     [<ffffffff811845af>] __kmalloc+0xef/0x260
>     [<ffffffff81534028>] sock_kmalloc+0x38/0x60
>     [<ffffffff8155d4dd>] sk_attach_filter+0x5d/0x190
>     [<ffffffff815378a1>] sock_setsockopt+0x991/0x9e0
>     [<ffffffff81531bd6>] SyS_setsockopt+0xb6/0xd0
>     [<ffffffff8165f3e9>] system_call_fastpath+0x16/0x1b
>     [<ffffffffffffffff>] 0xffffffffffffffff
>
> Cc: Alexei Starovoitov <ast@plumgrid.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: stable@vger.kernel.org # 3.12
> Signed-off-by: Andrey Vagin <avagin@openvz.org>
> ---
>  arch/x86/net/bpf_jit_comp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 516593e..3c55de5 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -788,5 +788,6 @@ void bpf_jit_free(struct sk_filter *fp)
>         if (fp->bpf_func != sk_run_filter) {
>                 INIT_WORK(&fp->work, bpf_jit_free_deferred);
>                 schedule_work(&fp->work);
> -       }
> +       } else
> +               kfree(fp);

please add extra { } after else

Thanks
Alexei

Re: [PATCH] net: don't forget to free sk_filter

[Daniel Borkmann]

On 11/06/2013 04:51 PM, Andrey Vagin wrote:
> sk_filter isn't freed if bpf_func is equal to sk_run_filter.
>
> This memory leak was introduced by
> commit d45ed4a4e33ae103053c0a53d280014e7101bb5c
> Author: Alexei Starovoitov <ast@plumgrid.com>
> Date:   Fri Oct 4 00:14:06 2013 -0700
>
>      net: fix unsafe set_memory_rw from softirq
>
> Before this patch sk_filter was freed in sk_filter_release_rcu,
> now it is freed in bpf_jit_free.
>
> Here is output of kmemleak:
> unreferenced object 0xffff8800b774eab0 (size 128):
>    comm "systemd", pid 1, jiffies 4294669014 (age 124.062s)
>    hex dump (first 32 bytes):
>      00 00 00 00 0b 00 00 00 20 63 7f b7 00 88 ff ff  ........ c......
>      60 d4 55 81 ff ff ff ff 30 d9 55 81 ff ff ff ff  `.U.....0.U.....
>    backtrace:
>      [<ffffffff816444be>] kmemleak_alloc+0x4e/0xb0
>      [<ffffffff811845af>] __kmalloc+0xef/0x260
>      [<ffffffff81534028>] sock_kmalloc+0x38/0x60
>      [<ffffffff8155d4dd>] sk_attach_filter+0x5d/0x190
>      [<ffffffff815378a1>] sock_setsockopt+0x991/0x9e0
>      [<ffffffff81531bd6>] SyS_setsockopt+0xb6/0xd0
>      [<ffffffff8165f3e9>] system_call_fastpath+0x16/0x1b
>      [<ffffffffffffffff>] 0xffffffffffffffff
>
> Cc: Alexei Starovoitov <ast@plumgrid.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: stable@vger.kernel.org # 3.12

^^^^ vi Documentation/networking/netdev-FAQ.txt +155

> Signed-off-by: Andrey Vagin <avagin@openvz.org>

When you send v2 with Alexei's feedback, please also be more specific
in your subject like "net: x86: bpf: don't forget to free sk_filter"
or the like. Also it's enough to say 'This memory leak was introduced
by commit d45ed4a4e3 ("net: fix unsafe set_memory_rw from softirq")'
instead of copying the whole log. Anyways, for v2 with feedback included
then:

Acked-by: Daniel Borkmann <dborkman@redhat.com>

Re: [PATCH] net: don't forget to free sk_filter

[Eric Dumazet]

On Wed, 2013-11-06 at 20:19 +0100, Daniel Borkmann wrote:

> When you send v2 with Alexei's feedback, please also be more specific
> in your subject like "net: x86: bpf: don't forget to free sk_filter"
> or the like. Also it's enough to say 'This memory leak was introduced
> by commit d45ed4a4e3 ("net: fix unsafe set_memory_rw from softirq")'
> instead of copying the whole log. Anyways, for v2 with feedback included
> then:

Actually, the new way [1] of doing this would be to use the 'Fixes:' tag
as in :

Fixes: <12 digits SHA1> ("net: fix unsafe set_memory_rw from softirq")

[1] As discussed at last Kernel Summit

Example in 

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6920a1bd037374a632d585de127b6f945199dcb8

Re: [PATCH] net: don't forget to free sk_filter

[Daniel Borkmann]

On 11/06/2013 08:28 PM, Eric Dumazet wrote:
> On Wed, 2013-11-06 at 20:19 +0100, Daniel Borkmann wrote:
>
>> When you send v2 with Alexei's feedback, please also be more specific
>> in your subject like "net: x86: bpf: don't forget to free sk_filter"
>> or the like. Also it's enough to say 'This memory leak was introduced
>> by commit d45ed4a4e3 ("net: fix unsafe set_memory_rw from softirq")'
>> instead of copying the whole log. Anyways, for v2 with feedback included
>> then:
>
> Actually, the new way [1] of doing this would be to use the 'Fixes:' tag
> as in :
>
> Fixes: <12 digits SHA1> ("net: fix unsafe set_memory_rw from softirq")
>
> [1] As discussed at last Kernel Summit
>
> Example in
>
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6920a1bd037374a632d585de127b6f945199dcb8

Cool, good to know, that's even better!

Re: [PATCH] net: don't forget to free sk_filter

[Alexei Starovoitov]

On Wed, Nov 6, 2013 at 11:28 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Actually, the new way [1] of doing this would be to use the 'Fixes:' tag
> as in :
>
> Fixes: <12 digits SHA1> ("net: fix unsafe set_memory_rw from softirq")
>
> [1] As discussed at last Kernel Summit

thx. good to know.
Unfortunately 2013 Kernel Summit proceedings are not public (yet?),
but available to paid LWN subscribers.

Re: [PATCH] net: don't forget to free sk_filter

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 06 Nov 2013 11:28:18 -0800

> On Wed, 2013-11-06 at 20:19 +0100, Daniel Borkmann wrote:
> 
>> When you send v2 with Alexei's feedback, please also be more specific
>> in your subject like "net: x86: bpf: don't forget to free sk_filter"
>> or the like. Also it's enough to say 'This memory leak was introduced
>> by commit d45ed4a4e3 ("net: fix unsafe set_memory_rw from softirq")'
>> instead of copying the whole log. Anyways, for v2 with feedback included
>> then:
> 
> Actually, the new way [1] of doing this would be to use the 'Fixes:' tag
> as in :
> 
> Fixes: <12 digits SHA1> ("net: fix unsafe set_memory_rw from softirq")
> 
> [1] As discussed at last Kernel Summit
> 
> Example in 
> 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6920a1bd037374a632d585de127b6f945199dcb8

Indeed, and I'm totally fine with this new mechanism too, please use it.

Re: [PATCH] net: don't forget to free sk_filter

[Keller, Jacob E]

On Wed, 2013-11-06 at 11:28 -0800, Eric Dumazet wrote:
> On Wed, 2013-11-06 at 20:19 +0100, Daniel Borkmann wrote:
> 
> > When you send v2 with Alexei's feedback, please also be more specific
> > in your subject like "net: x86: bpf: don't forget to free sk_filter"
> > or the like. Also it's enough to say 'This memory leak was introduced
> > by commit d45ed4a4e3 ("net: fix unsafe set_memory_rw from softirq")'
> > instead of copying the whole log. Anyways, for v2 with feedback included
> > then:
> 
> Actually, the new way [1] of doing this would be to use the 'Fixes:' tag
> as in :
> 
> Fixes: <12 digits SHA1> ("net: fix unsafe set_memory_rw from softirq")
> 
> [1] As discussed at last Kernel Summit
> 
> Example in 
> 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6920a1bd037374a632d585de127b6f945199dcb8
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Has there been any mention of a forthcoming patch
to ./Documentation/SubmittingPatches which documents this?

Thanks,
Jake

Re: [PATCH] net: don't forget to free sk_filter

[Eric Dumazet]

On Fri, 2013-11-08 at 00:56 +0000, Keller, Jacob E wrote:

> Has there been any mention of a forthcoming patch
> to ./Documentation/SubmittingPatches which documents this?

I do not remember if this point was raised, but you certainly
can submit a patch !

Thanks