Re: [PATCH nf-next v3 0/5] netfilter: nf_tables: reduce set element transaction size

Re: [PATCH nf-next v3 0/5] netfilter: nf_tables: reduce set element transaction size

[Pablo Neira Ayuso]

Cc'ing audit ML.

On Wed, Oct 16, 2024 at 06:10:44PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > This is bad, but I do not know if we can change things to make
> > > nft_audit NOT do that.  Hence add a new workaround patch that
> > > inflates the length based on the number of set elements in the
> > > container structure.
> > 
> > It actually shows the number of entries that have been updated, right?
> > 
> > Before this series, there was a 1:1 mapping between transaction and
> > objects so it was easier to infer it from the number of transaction
> > objects.
> 
> Yes, but... for element add (but not create), we used to not do anything
> (no-op), so we did not allocate a new transaction and pretend request
> did not exist.

You refer to element updates above, those used to be elided, yes. Now
they are shown. I think that is correct.

> Now we can enter update path, so we do allocate a transaction, hence,
> audit record changes.
>
> What if we add an internal special-case 'flush' op in the future?

You mean, if 'flush' does not get expanded to one delete transaction
for each element. Yes, that would require to look at ->nelems as in
this patch.

> It will break, and the workaround added in this series needs to be
> extended.
> 
> Same for an other change that could elide a transaction request, or,
> add expand something to multiple ones (as flush currently does).
> 
> Its doesn't *break* audit, but it changes the output.

My understanding is that audit is exposing the entries that have been
added/updated/deleted, which is already sparse: Note that nftables
audit works at 'table' granurality.

IIRC, one of the audit maintainers mentioned it should be possible to
add chain and set to the audit logs in the future, such change would
necessarily change the audit logs output.

Thanks.

Re: [PATCH nf-next v3 0/5] netfilter: nf_tables: reduce set element transaction size

[Paul Moore]

On Thu, Oct 17, 2024 at 12:24 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Cc'ing audit ML.

Thank you.

> On Wed, Oct 16, 2024 at 06:10:44PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > This is bad, but I do not know if we can change things to make
> > > > nft_audit NOT do that.  Hence add a new workaround patch that
> > > > inflates the length based on the number of set elements in the
> > > > container structure.
> > >
> > > It actually shows the number of entries that have been updated, right?
> > >
> > > Before this series, there was a 1:1 mapping between transaction and
> > > objects so it was easier to infer it from the number of transaction
> > > objects.
> >
> > Yes, but... for element add (but not create), we used to not do anything
> > (no-op), so we did not allocate a new transaction and pretend request
> > did not exist.
>
> You refer to element updates above, those used to be elided, yes. Now
> they are shown. I think that is correct.
>
> > Now we can enter update path, so we do allocate a transaction, hence,
> > audit record changes.
> >
> > What if we add an internal special-case 'flush' op in the future?
>
> You mean, if 'flush' does not get expanded to one delete transaction
> for each element. Yes, that would require to look at ->nelems as in
> this patch.
>
> > It will break, and the workaround added in this series needs to be
> > extended.
> >
> > Same for an other change that could elide a transaction request, or,
> > add expand something to multiple ones (as flush currently does).
> >
> > Its doesn't *break* audit, but it changes the output.
>
> My understanding is that audit is exposing the entries that have been
> added/updated/deleted, which is already sparse: Note that nftables
> audit works at 'table' granurality.
>
> IIRC, one of the audit maintainers mentioned it should be possible to
> add chain and set to the audit logs in the future, such change would
> necessarily change the audit logs output.

For those of us joining the conversation late, can you provide a quick
summary of what you want to change in audit and why?

-- 
paul-moore.com

Re: [PATCH nf-next v3 0/5] netfilter: nf_tables: reduce set element transaction size

[Pablo Neira Ayuso]

On Thu, Oct 17, 2024 at 03:33:14PM -0400, Paul Moore wrote:
> > On Wed, Oct 16, 2024 at 06:10:44PM +0200, Florian Westphal wrote:
> For those of us joining the conversation late, can you provide a quick
> summary of what you want to change in audit and why?

Florian said:

"I failed to realize that nft_audit leaks one implementation detail
to userspace: the length of the transaction log."

        table=t1 family=2 entries=4 op=nft_register_set

He is referring to the 'entries' key.

So far, every object gets one transaction, but now batching several
objects in one transaction is possible.

We have been discussing what the expected semantics for this audit log
key is:

- If this is the transaction log length, then the internal update of the
  transaction logic results in a smaller number of 'entries' in the
  audit log.
- If 'entries' refers to the number of "affected" objects by this
  operation, then this means we have to carry a "workaround" in
  the kernel.

This is because:

1) Element updates are now supported, this currently handles it as a
   _REGISTER operation according to enum audit_nfcfgop. This changed
   the semantics of the add command, now it is "add if it does not exist,
   otherwise update what it already exists". Before, updates where simply
   elided (not counted by 'entries' key) because they were not supported.
   That is, 'entries' now tell how many set element has been added OR
   updated. I think this is fine, this is consistent with chain updates
   where 'entries' also report added OR updated chains. The difference
   is that old kernel do not count updates (because they are elided).

2) There is ongoing work to add more internal transaction batching, ie.
   use one single transaction for several elements. This requires a
   special case to bump the 'entries' key to add the elements that the
   transaction batch contains, see:

https://patchwork.ozlabs.org/project/netfilter-devel/patch/20241016131917.17193-4-fw@strlen.de/

   There is a nft_audit.sh selftest and Florian thinks this is a
   "workaround" patch only to make this test happy, because 'entries'
   refers to the transaction log length (which is now smaller given the
   internal transaction batching approach to accumulate several elements
   is used).