Re: [PATCH net-next 09/15] netfilter: flowtable: purge entries immediately when destroying the flowtable

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message understates the impact of the bug it fixes.

--

netfilter: flowtable: purge entries immediately when destroying the flowtable

When a flowtable is destroyed, this patch stops relying on the hardware
offload workqueue to purge the remaining flow entries. It now walks the
rhashtable through rhashtable_free_and_destroy() with
nf_flow_table_do_purge(). That callback tears down each flow, removes its
hardware state synchronously and frees it. NF_FLOW_PENDING marks the first
tuplehash visited, so the flow is released only on the second visit.

> Do not use hardware offload workqueue to purge the remaining flow
> entries when the flowtable is being destroyed. Instead, iterate over the
> flowtable to purge the remaining flow offload entries. Work memory
> allocations can fail, leaving us with either waiting indefinitely until
> work can be allocated or leaving entries in the hardware.

[Severity: Low]
The code change looks correct. Does this paragraph understate what the old
nf_flow_table_free() sequence could do when the work allocation failed?

The old sequence was:

    nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
    nf_flow_table_gc_run(flow_table);
    nf_flow_table_offload_flush_cleanup(flow_table);
    rhashtable_destroy(&flow_table->rhashtable);

On that path, nf_flow_offload_gc_step() calls nf_flow_offload_del(). If the
GFP_ATOMIC allocation in nf_flow_offload_work_alloc() fails, it returns
before setting NF_FLOW_HW_DYING:

    offload = nf_flow_offload_work_alloc(flowtable, flow, FLOW_CLS_DESTROY);
    if (!offload)
        return;

    set_bit(NF_FLOW_HW_DYING, &flow->flags);

As a result, flow_offload_del() is never called for that flow, and it stays
in the rhashtable. rhashtable_destroy() frees only the bucket tables. That
leaks the struct flow_offload along with its nf_conn reference and its
route dst references, which goes beyond leaving entries in the hardware.

The allocation could also fail in the first nf_flow_table_gc_run() and then
succeed in the second one, inside nf_flow_table_offload_flush_cleanup():

    flush_workqueue(nf_flow_offload_del_wq);
    nf_flow_table_gc_run(flowtable);

In that case, the del work is queued after the last flush and never flushed
again. nf_tables_flowtable_destroy() calls kfree(flowtable) right after
nf_flow_table_free() returns, and tcf_ct_flow_table_cleanup_work() frees
ct_ft. So when the work runs later, flow_offload_work_handler() would read
offload->flowtable->net, and flow_offload_tuple_del() would walk
offload->flowtable->flow_block.cb_list, both in freed memory.

Could the commit message mention the flow and conntrack reference leak, and
the possible use-after-free of the flowtable from late-queued del work?
That would help stable and security triage assess this fix.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113031251.2437074-2-pablo%40netfilter.org