Re: [PATCH RFC] netfilter: nf_tables: add flowtable map for xdp offload

[Florian Westphal <fw@strlen.de>]

Florian Westphal <fw@strlen.de> wrote:

Pablo, I am going to make changes to the flowtable infra, it would
be nice if you could nack/ack the following approach before I start to
spend cycles on this:

> Florian Westphal <fw@strlen.de> wrote:
> This is fine or at least can be made to work.
> 
> > +	case FLOW_BLOCK_UNBIND:
> > +		nf_flowtable_by_dev_remove(dev);
> 
> This is broken.  UNBIND comes too late when things are torn down.
> 
> I only see two solutions:
> 
> 1. add a new nf_flow_offload_unbind_prepare() that does this
> 2. Decouple nf_flowtable from nft_flowtable and make nf_flowtable
>    refcounted.  As-is, the UNBIND will result in UAF because the
>    underlying structures will be free'd immediately after this,
>    without any synchronize_rcu().

I'll go with 2).  Rough Plan is:

1. Do not embed nf_flowtable into nft_flowtable or the act_ct struct,
   make this a pointer, which is allocated/freed via kmalloc/kfree.

2. add a refcount_t to nf_flowtable, so the nf_flowtable can have
   its reference incremented for as long as an XDP program might
   be using this.

3. Change nf_flowtable_free so that it will honor the reference count.
   Last _put will queue destruction to rcu worker, so the teardown of
   the rhashtable and other items passes through another
   synchronize_rcu().  This will also move kfree() of nf_flowtable into
   nf_flowtable_free.

4. Refactor the nf_flowtable init function so it will allocate and
   return the flowtable structure.

Only alternative I see is to rework both act_ct and nf_tables_api.c
to perform UNBINDs *before* synchronize_rcu (and the flowtable
teardown).

Doing that means that the xdp prog will not be able to 'pin' the
underlying nf_flowtable (as its embedded in another data structure,
either tcf_ct_flow_table or nft_flowtable), and would have to rely
on the callers to prevent the kfunc from every returning an nf_flowtable
that has already been free'd.

Not a problem now, but would be a problem in case bpf would gain
the ability to expose struct nf_flowtable as a refcounted kptr.