Re: [PATCH nf 1/2] netfilter: conntrack: make conntrack userspace helpers work again

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Florian Westphal says:
> 
> "Problem is that after the helper hook was merged back into the confirm
> one, the queueing itself occurs from the confirm hook, i.e. we queue
> from the last netfilter callback in the hook-list.
> 
> Therefore, on return, the packet bypasses the confirm action and the
> connection is never committed to the main conntrack table.
> 
> Therefore, on return, the packet bypasses the confirm action and the
> connection is never committed to the main conntrack table.
> 
> To fix this there are several ways:
> 1. revert the 'Fixes' commit and have a extra helper hook again.
>    Works, but has the drawback of adding another indirect call for
>    everyone.
> 
> 2. Special case this: split the hooks only when userspace helper
>    gets added, so queueing occurs at a lower priority again,
>    and normal nqueue reinject would eventually call the last hook.
> 
> 3. Extend the existing nf_queue ct update hook to allow a forced
>    confirmation (plus run the seqadj code).
> 
> This goes for 3)."
> 
> Fixes: 827318feb69cb ("netfilter: conntrack: remove helper hook again")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> + * processing after the helper invocation in nf_confirm().
> + */
> +static int nf_confirm_cthelper(struct sk_buff *skb, struct nf_conn *ct,
> +			       enum ip_conntrack_info ctinfo)
> +{
> +	const struct nf_conntrack_helper *helper;
> +	const struct nf_conn_help *help;
> +	unsigned int protoff;
> +
> +	help = nfct_help(ct);
> +	if (!help)
> +		return 0;
> +
> +	helper = rcu_dereference(help->helper);
> +	if (!(helper->flags & NF_CT_HELPER_F_USERSPACE))
> +		return 0;

Relying on this check means that in case of

... -j CT (assign userspace helper)
... -j NFQUEUE

> +	/* We've seen it coming out the other side: confirm it */
> +	return nf_conntrack_confirm(skb) == NF_DROP ? - 1 : 0;
> +}

> +static int nf_conntrack_update(struct net *net, struct sk_buff *skb)
> +{
[..]
> +	err = nf_confirm_cthelper(skb, ct, ctinfo);
> +	if (err < 0)
> +		return err;
> +
> +	if (nf_ct_is_confirmed(ct))
> +		return 0;

This means that in case of userspace helper, we return here
any bypass the __nf_conntrack_update logic.

I don't think thats a problem either given the userspace
helper presence, so

Acked-by: Florian Westphal <fw@strlen.de>