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

[Quan Tian <tianquan23@gmail.com>]

On Tue, Mar 12, 2024 at 11:23:56PM +0100, Pablo Neira Ayuso wrote:
> 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.

There was a use-case in kube-proxy that we wanted to use comment to
store version/compatibility information so we could know whether it
has to recreate the whole table when upgrading to a new version due to
incompatible chain/rule changes (e.g. a chain's hook and priority is
changed). The reason why we wanted to avoid recreating the whole table
when it doesn't have to is because deleting the table would also
destory the dynamic sets in the table, losing some data.

Another minor reason is that the comments could be used to store
human-readable explanation for the objects. And they may be change when
the objects' functions change. It would be great if they could be
updated via the "add" operation, otherwise "delete+add" would always be
needed to keep comments up-to-date.

However, I don't know a use-case for back-to-back comment updates, I 
will check which one is more complex and risky: rejecting it or
supporting it.

> 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.

As explained above, I think there is some value in supporting comment
update for other objects, especially when the objects contain dynamic
data. But I understand there are risks like you mentioned and it would
need to be more careful than tables. I would volunteer to take a try
for other objects, starting with objects similiar to tables first, if
it doesn’t sound bad to you. Please let me know if you feel it's not
worth.

Thanks,
Quan