On Fri, Jun 19, 2015 at 02:03:39PM -0500, Eric W. Biederman wrote:
>
> Add code to nf_unregister_hook to flush the nf_queue when a hook is
> unregistered. This guarantees that the pointer that the nf_queue code
> retains into the nf_hook list will remain valid while a packet is
> queued.
I think the real problem is that struct nf_queue_entry holds a pointer
to struct nf_hook_ops, which will be gone after removal. So you
uncovered a long standing problem that will amplify by when pernet
hooks are in place.
Regarding the pointer to nf_hook_list, now that new netdevice variant
doesn't support nf_queue yet, so that nf_hook_list will be always
valid since it will point to the global nf_hooks in the core.
> I tested what would happen if we do not flush queued packets and was
> trivially able to obtain the oops below. All that was required was
> to stop the nf_queue listening process, to delete all of the nf_tables,
> and to awaken the nf_queue listening process.
[...]
Please, route netfilter patches through the netfilter trees, ie. nf
and nf-next.
> Cc: stable@xxxxxxxxxxxxxxx
I guess this is a leftover since there is no Cc to stable. Anyway,
we have to wait until this hits master before we ask for -stable
inclusion.
More comments below. Thanks for this fix BTW.
> Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> ---
>
> Apologies for the duplicate send but I forgot to include the appropriate
> mailing lists.
>
> include/net/netfilter/nf_queue.h | 2 ++
> net/netfilter/core.c | 1 +
> net/netfilter/nf_internals.h | 1 +
> net/netfilter/nf_queue.c | 17 +++++++++++++++++
> net/netfilter/nfnetlink_queue_core.c | 24 +++++++++++++++++++++++-
> 5 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/netfilter/nf_queue.h b/include/net/netfilter/nf_queue.h
> index d81d584157e1..e8635854a55b 100644
> --- a/include/net/netfilter/nf_queue.h
> +++ b/include/net/netfilter/nf_queue.h
> @@ -24,6 +24,8 @@ struct nf_queue_entry {
> struct nf_queue_handler {
> int (*outfn)(struct nf_queue_entry *entry,
> unsigned int queuenum);
> + void (*nf_hook_drop)(struct net *net,
> + struct nf_hook_ops *ops);
> };
>
> void nf_register_queue_handler(const struct nf_queue_handler *qh);
> diff --git a/net/netfilter/core.c b/net/netfilter/core.c
> index 653e32eac08c..a0e54974e2c9 100644
> --- a/net/netfilter/core.c
> +++ b/net/netfilter/core.c
> @@ -118,6 +118,7 @@ void nf_unregister_hook(struct nf_hook_ops *reg)
> static_key_slow_dec(&nf_hooks_needed[reg->pf][reg->hooknum]);
> #endif
> synchronize_net();
> + nf_queue_nf_hook_drop(reg);
> }
> EXPORT_SYMBOL(nf_unregister_hook);
>
> diff --git a/net/netfilter/nf_internals.h b/net/netfilter/nf_internals.h
> index ea7f36784b3d..399210693c2a 100644
> --- a/net/netfilter/nf_internals.h
> +++ b/net/netfilter/nf_internals.h
> @@ -19,6 +19,7 @@ unsigned int nf_iterate(struct list_head *head, struct sk_buff *skb,
> /* nf_queue.c */
> int nf_queue(struct sk_buff *skb, struct nf_hook_ops *elem,
> struct nf_hook_state *state, unsigned int queuenum);
> +void nf_queue_nf_hook_drop(struct nf_hook_ops *ops);
> int __init netfilter_queue_init(void);
>
> /* nf_log.c */
> diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
> index 2e88032cd5ad..cd60d397fe05 100644
> --- a/net/netfilter/nf_queue.c
> +++ b/net/netfilter/nf_queue.c
> @@ -105,6 +105,23 @@ bool nf_queue_entry_get_refs(struct nf_queue_entry *entry)
> }
> EXPORT_SYMBOL_GPL(nf_queue_entry_get_refs);
>
> +void nf_queue_nf_hook_drop(struct nf_hook_ops *ops)
I'd suggest you rename all these 'nf_hook_drop' to 'flush'.
> +{
> + const struct nf_queue_handler *qh;
> + struct net *net;
> +
> + rtnl_lock();
Why rtnl_lock() here?
> + rcu_read_lock();
> + qh = rcu_dereference(queue_handler);
> + if (qh) {
> + for_each_net(net) {
> + qh->nf_hook_drop(net, ops);
> + }
> + }
> + rcu_read_unlock();
> + rtnl_unlock();
> +}
> +
> /*
> * Any packet that leaves via this function must come back
> * through nf_reinject().
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
