netdev - Re: [RFC net-next 0/6] xdp: make stack perform remove and tests

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171124205505.2d3a11a8@cakuba.netronome.com>
Date:   Fri, 24 Nov 2017 20:55:05 -0800
From:   Jakub Kicinski <kubakici@...pl>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     Linux Netdev List <netdev@...r.kernel.org>,
        oss-drivers@...ronome.com, Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Subject: Re: [RFC net-next 0/6] xdp: make stack perform remove and tests

On Fri, 24 Nov 2017 00:02:32 -0800, Jakub Kicinski wrote:
> >>Something I'm still battling with, and would appreciate help of
> >>wiser people is that occasionally during the test something makes
> >>the refcount of init_net drop to 0 :S  I tried to create a simple
> >>reproducer, but seems like just running the script in the loop is
> >>the easiest way to go...  Could it have something to do with the
> >>recent TC work?  The driver is pretty simple and never touches  
> >
> > I don't see how...  
> 
> To be clear I meant the changes made to destruction of filters, not
> your work. The BPF code doesn't touch ref counts and cls exts do seem
> to hold a ref on the net...  but perhaps that's just pointing the
> finger unnecessarily :)  I will try to investigate again tomorrow.

Looks like I was lazy when adding the offload and just called
__cls_bpf_delete_prog() instead of extending the error path.  
Cong missed this extra call in aae2c35ec892 ("cls_bpf: use
tcf_exts_get_net() before call_rcu()").  We need something like 
this:

diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index a9f3e317055c..40d4289aea28 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -514,12 +514,8 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
                goto errout_idr;
 
        ret = cls_bpf_offload(tp, prog, oldprog);
-       if (ret) {
-               if (!oldprog)
-                       idr_remove_ext(&head->handle_idr, prog->handle);
-               __cls_bpf_delete_prog(prog);
-               return ret;
-       }
+       if (ret)
+               goto errout_parms;
 
        if (!tc_in_hw(prog->gen_flags))
                prog->gen_flags |= TCA_CLS_FLAGS_NOT_IN_HW;
@@ -537,6 +533,13 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
        *arg = prog;
        return 0;
 
+errout_parms:
+       if (cls_bpf_is_ebpf(prog))
+               bpf_prog_put(prog->filter);
+       else
+               bpf_prog_destroy(prog->filter);
+       kfree(prog->bpf_name);
+       kfree(prog->bpf_ops);
 errout_idr:
        if (!oldprog)
                idr_remove_ext(&head->handle_idr, prog->handle);