On Tue, Mar 12, 2024 at 03:33:00PM +0100, Florian Westphal wrote: > Quan Tian <tianquan23@xxxxxxxxx> 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.
