Re: [nf-next PATCH 2/4] netfilter: nfnetlink_hook: Deref hook entry using READ_ONCE()

Pablo Neira Ayuso <[email protected]> wrote:
> Are we sure net/netfilter/core.c is safe to be walked over rcu in its
> current state? Could the dummy_ops be exposed through nfnetlink_hook?

What do you mean with 'safe'?
The walk is safe from memory safety point of view.

dummy_ops *can* be exposed.

Otherwise, hook unregister can fail when low on memory:
ATM, in case we unregister hook and then fail to alloc the replacement
blob (that is same as live one minus the removed hook) we leave the
dummy stub in so old hook function is no longer executed and leave the
outdated/stale blob in place.

One alternative to dummy-ops usage is to keep a spare blob around so we
can avoid the new memory allocation when a hook goes away.

Then, on delete:

1. use the spare (which is large enough) instead
   and prepare the new blob (without removed fn).
2. swap the spare with live version.
3. attempt to allocate a new spare.
   if that fails, force a synchronize_rcu() and make
   the 'old' live the new spare.
   Else, use the new spare and avoid the,
   synchronize_rcu(), old-live is handed off to call_rcu.

Hook-add would always have to keep the size of the spare
up to date, so it is always large enough to hold the
current amount of live hooks.

Its a bit more work, but it avoids the need for dummy_ops.
LLM should be able to generate the transformation patches.

> Maybe net/netfilter/core.c needs a revisited to use
> rcu_assign_pointer() to assign the hook_ops to the blob, then
> nfnetlink_hook uses rcu_dereference() instead of READ_ONCE.

Why?  What is the concern?

> Then the RCU semantics of the hooks would exposed in a better way?
>
> That would made double use of RCU, one from the blob and then for the
> hook_ops.

Its technically not needed, I think, the hook_ops are not supposed
to be munged while the blob is active.

Adding extra rcu_dereference adds additional barriers for each hook/elem
in the blob...

> The hooks are now released using kfree_rcu(), at least in the recent
> nf_nat core updates they are.

Yes.