Daniel Xu <dxu@xxxxxxxxx> 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?
