Re: [PATCH v3 nf-next 2/2] netfilter: nf_tables: support updating userdata for nft_table

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

On Tue, Mar 12, 2024 at 03:33:00PM +0100, Florian Westphal wrote:
> Quan Tian <tianquan23@gmail.com> wrote:
> > In nf_tables_commit():
> > The 1st trans swaps old udata with 1st new udata;
> > The 2nd trans swaps 1st new udata with 2nd new udata.
> > 
> > In nft_commit_release():
> > The 1st trans frees old udata;
> > The 2nd trans frees 1st new udata.
> > 
> > So multiple udata requests in a batch could work?
> 
> Yes, it could work indeed but we got bitten by
> subtle bugs with back-to-back updates.

yes, we have seen subtle bugs recently. As for the table flags, the
dormant flag has been particularly a problem, it is tricky one because
it registers hooks from preparation step (which might fail) but it
cannot register hooks because abort path might need undo things, and
re-register the hooks could lead to failures from a later path which
does not admit failures. For the dormant flag, another possibility
would be to handle this from the core, so there is no need to register
and unregister hooks, instead simply set them as inactive.

The dormant flag use case is rather corner case, but hardware offload
will require sooner or later a mode to run in _hardware mode only_
(currently it is both hardware and software for nftables) and
considering the hardware offload API has been designed for packet
classifiers from the late 90s (that is, very strictly express your
policy in a linear ruleset) that means dropping packets early is fine,
but wanted traffic gets evaluated in a linear list.

> If there is a simple way to detect and reject
> this then I believe its better to disallow it.

That requires to iterate over the list of transaction, or add some
kind of flag to reject this.

> Unless you come up with a use-case where such back-to-back
> udate updates make sense of course.

I don't have a use-case for this table userdata myself, this is
currently only used to store comments by userspace, why someone would
be willing to update such comment associated to a table, I don't know.

I would like to know if there are plans to submit similar patches for
other objects. As for sets, this needs to be careful because userdata
contains the set description.