[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <6av46ydgbufp5x23lempwmutcsjuy6efpysbvnqxjoirng43tr@gcyqxhln2x6f>
Date: Tue, 18 Jul 2023 15:45:32 -0600
From: Daniel Xu <dxu@...uu.xyz>
To: Florian Westphal <fw@...len.de>
Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>,
Andrii Nakryiko <andrii@...nel.org>,
Alexei Starovoitov <ast@...nel.org>,
"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
Hi Florian,
On Fri, Jul 14, 2023 at 11:47:41AM +0200, Florian Westphal wrote:
> 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?
That sounds reasonable to me. I'll give it a shot. Thanks for the input!
Daniel
Powered by blists - more mailing lists