Re: [PATCH nf 5/5] netfilter: nfnl_cthelper: fix a race when walk the nf_ct_helper_hash table

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Sun, Mar 19, 2017 at 10:36:02PM +0800, Liping Zhang wrote:
> From: Liping Zhang <zlpnobody@gmail.com>
> 
> The nf_ct_helper_hash table is protected by nf_ct_helper_mutex, while
> nfct_helper operation is protected by nfnl_lock(NFNL_SUBSYS_CTHELPER).
> So it's possible that one CPU is walking the nf_ct_helper_hash for
> cthelper add/get/del, another cpu is doing nf_conntrack_helpers_unregister
> at the same time. This is dangrous, and may cause use after free error.
> 
> Note, delete operation will flush all cthelpers added via nfnetlink, so
> using rcu to do protect is not easy.
> 
> Now introduce a dummy list to record all the cthelpers added via
> nfnetlink, then we can walk the dummy list instead of walking the
> nf_ct_helper_hash. Also, keep nfnl_cthelper_dump_table unchanged, it
> may be invoked without nfnl_lock(NFNL_SUBSYS_CTHELPER) held.
> 
> Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
> ---
>  net/netfilter/nfnetlink_cthelper.c | 176 +++++++++++++++++++------------------
>  1 file changed, 89 insertions(+), 87 deletions(-)
> 
> diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
> index fc4733f..47424ec 100644
> --- a/net/netfilter/nfnetlink_cthelper.c
> +++ b/net/netfilter/nfnetlink_cthelper.c
> @@ -32,6 +32,13 @@ MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Pablo Neira Ayuso <pablo@netfilter.org>");
>  MODULE_DESCRIPTION("nfnl_cthelper: User-space connection tracking helpers");
>  
> +struct nfnl_cthelper {
> +	struct list_head		list;
> +	struct nf_conntrack_helper	*helper;
> +};
> +
> +static LIST_HEAD(nfnl_cthelper_list);

We need a field possible_net_t so we can store what netns this helper
belongs to, thus in case of flush command, we just remove the helpers
that this netns owns.