linux-kernel - Re: [PATCH bpf-next v4 2/6] netfilter: bpf: Support BPF_F_NETFILTER_IP

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]

Date:   Fri, 14 Jul 2023 11:47:41 +0200
From:   Florian Westphal <fw@...len.de>
To:     Daniel Xu <dxu@...uu.xyz>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Andrii Nakryiko <andrii@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Florian Westphal <fw@...len.de>,
        "David S. Miller" <davem@...emloft.net>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Jozsef Kadlecsik <kadlec@...filter.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
        bpf <bpf@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>,
        netfilter-devel <netfilter-devel@...r.kernel.org>,
        coreteam@...filter.org,
        Network Development <netdev@...r.kernel.org>,
        David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH bpf-next v4 2/6] netfilter: bpf: Support
 BPF_F_NETFILTER_IP_DEFRAG in netfilter link

Daniel Xu <dxu@...uu.xyz> wrote:
> On Thu, Jul 13, 2023 at 04:10:03PM -0700, Alexei Starovoitov wrote:
> > Why is rcu_assign_pointer() used?
> > If it's not RCU protected, what is the point of rcu_*() accessors
> > and rcu_read_lock() ?
> > 
> > In general, the pattern:
> > rcu_read_lock();
> > ptr = rcu_dereference(...);
> > rcu_read_unlock();
> > ptr->..
> > is a bug. 100%.

FWIW, I agree with Alexei, it does look... dodgy.

> The reason I left it like this is b/c otherwise I think there is a race
> with module unload and taking a refcnt. For example:
> 
> ptr = READ_ONCE(global_var)
>                                              <module unload on other cpu>
> // ptr invalid
> try_module_get(ptr->owner) 
>

Yes, I agree.

> I think the the synchronize_rcu() call in
> kernel/module/main.c:free_module() protects against that race based on
> my reading.
> 
> Maybe the ->enable() path can store a copy of the hook ptr in
> struct bpf_nf_link to get rid of the odd rcu_dereference()?
> 
> Open to other ideas too -- would appreciate any hints.

I would suggest the following:

- Switch ordering of patches 2 and 3.
  What is currently patch 3 would add the .owner fields only.

Then, what is currently patch #2 would document the rcu/modref
interaction like this (omitting error checking for brevity):

rcu_read_lock();
v6_hook = rcu_dereference(nf_defrag_v6_hook);
if (!v6_hook) {
        rcu_read_unlock();
        err = request_module("nf_defrag_ipv6");
        if (err)
                 return err < 0 ? err : -EINVAL;
        rcu_read_lock();
	v6_hook = rcu_dereference(nf_defrag_v6_hook);
}

if (v6_hook && try_module_get(v6_hook->owner))
	v6_hook = rcu_pointer_handoff(v6_hook);
else
	v6_hook = NULL;

rcu_read_unlock();

if (!v6_hook)
	err();
v6_hook->enable();


I'd store the v4/6_hook pointer in the nf bpf link struct, its probably more
self-explanatory for the disable side in that we did pick up a module reference
that we still own at delete time, without need for any rcu involvement.

Because above handoff is repetitive for ipv4 and ipv6,
I suggest to add an agnostic helper for this.

I know you added distinct structures for ipv4 and ipv6 but if they would use
 the same one you could add

static const struct nf_defrag_hook *get_proto_frag_hook(const struct nf_defrag_hook __rcu *hook,
							const char *modulename);

And then use it like:

v4_hook = get_proto_frag_hook(nf_defrag_v4_hook, "nf_defrag_ipv4");

Without a need to copy the modprobe and handoff part.

What do you think?