Re: [PATCH net-next 04/15] netfilter: nf_queue: limit the hook drop flush to the unregistered hook point

[Florian Westphal <fw@strlen.de>]

netdev-bot+sashiko@kernel.org <netdev-bot+sashiko@kernel.org> wrote:

@Antonio: TL;DR: if Pablo wants a v2, please send a new patch that adds
the needed nf_queue_nf_hook_drop() calls in brnf and conntrack.

Otherwise, please followup after this hits net-next tree.

> - [High] Use-after-free of module code: after this patch, `rmmod
>   br_netfilter` no longer drops queued packets that point into…
> - [Medium] Conntrack module unload can now stall on queued packets, and
>   the commit message does not mention it.
> - [Low] The commit message has no Fixes: tag.
> 
> --
> 
> [Severity: Low]
> Should this have a Fixes: tag?

Absolutely NOT.

> Without the trailer, stable maintainers may miss this fix. The message
> also does not say the omission is deliberate.

This is NOT a fix.  And the problems highlight that this belongs
in -next and not in stable trees.

> [Severity: High]
> After this change, can packets whose okfn points into br_netfilter.ko stay
> queued after that module is unloaded?

Yes, br nf strikes again.

> Is a flush-all, such as nf_queue_nf_hook_drop(net, NULL), needed in the
> br_netfilter exit path, or some other flush at module teardown for callers
> whose okfn is module code?

Yes, needs to be added for brnf.  Could be handled as a followup,
though, ample of time to fix this until this gets into a non-rc kernel.

> [Severity: Medium]
> Could rmmod nf_conntrack now block while userspace holds verdicts for
> queued packets?
>
> Before this patch, that unregistration flushed every queued entry. This
> released the nf_conn references held through skb->_nfct.
> 
> This runs while pernet_ops_rwsem is held for write. Netns creation and
> teardown would stall until a verdict is sent or the queue socket is
> closed.
> 
> The commit message says nf_ct_iterate_destroy() keeps the NULL flush on
> module exit. That path covers helpers and NAT, not nf_conntrack itself,
> and the message does not mention this case.

> Should nf_conntrack's exit path also flush with NULL?

Yes, nf_conntrack_fini_net() would be a candiate.

But unlike the brnf case I think this one is relatively harmless.

If v2 wanted: add