Florian Westphal <fw@xxxxxxxxx> 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@xxxxxxxxx> 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.
