Re: [nf-next PATCH v6 00/12] Dynamic hook interface binding part 2

[Phil Sutter <phil@nwl.cc>]

On Wed, May 21, 2025 at 05:51:50PM +0200, Pablo Neira Ayuso wrote:
> Hi Phil,
> 
> On Wed, May 21, 2025 at 05:32:57PM +0200, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Wed, May 21, 2025 at 12:28:01AM +0200, Pablo Neira Ayuso wrote:
> [...]
> > > 2) I wonder if nft_hook_find_ops()  will need a hashtable sooner or
> > >    later. With the wildcard, the number of devices could be significantly
> > >    large in this list lookup.
> > 
> > Maybe, yes. Is it useful to have a single flowtable for all virtual
> > functions (e.g.) on a hypervisor? Or would one rather have distinct
> > flowtables for each VM/container?
> [...]
> > Callers are:
> > 
> > - nft_{flowtable,netdev}_event(): Interface is added or removed
> > - nft_flow_offload_eval(): New flow being offloaded
> > - nft_offload_netdev_event(): Interface is removed
> > 
> > All these are "slow path" at least. I could try building a test case to
> > see how performance scales, but since we hit the function just once for
> > each new connection, I guess it's hard to get significant data out of
> > it.
> 
> This can be added later, not a deal breaker.

ACK.

> This is event path which might slow down adding new entries via
> rtnetlink maybe, but I would need to have a closer look.

We'd optimize for flowtables with many devices. The same system could
also have many flowtables with few devices each, then the hash table
adds to the overhead. A global table for all flowtables could serve
both. We could also have the same device in multiple chains, so each
hash table entry needs a list of ops pointers?

> [...]
> > > == netfilter: nf_tables: Add "notications" <-- typo: "notifications"
> > > 
> > > I suggest you add a new NFNLGRP_NFT_DEV group for these notifications,
> > > so NFNLGRP_NFTABLES is only used for control plane updates via
> > > nfnetlink API. In this case, these events are triggered by rtnetlink
> > > when a new device is registered and it matches the existing an
> > > existing device if I understood the rationale.
> > 
> > Yes, MSG_NEWDEV and MSG_DELDEV are triggered if a new device matches a
> > hook or if a hooked device is removed (or renamed, so the hook won't
> > match anymore).
> > 
> > Having a distinct NFNLGRP for them requires a new 'nft monitor' mode,
> > right? So we can't have a single monitor process for ruleset changes and
> > these device events. Should not be a problem, though.
> 
> Having a different group allows to filter out events you might not
> care about, it is a simple netlink event filtering facility.
> 
> I think this feature is for debugging purpose only, correct? So a
> separated group should be fine. IIUC, this event does not modify the
> ruleset, it only tells us a hook for a matching device is being
> registered.

Yes, you're right. Strictly speaking, these events are different from
NFNLGRP_NFTABLES as they don't indicate a ruleset change. Their actual
purpose is to satisfy the requirement of hook reg/unreg being visible to
user space. :)

nft monitor needs some work, though: Right now it's in trace or !trace
mode depending on whether NFT_MSG_TRACE is present in monitor_flags. Now
there will be a third modus operandi. OK, we could hide that internally
and enable the new group automatically if not tracing, then add a
'hooks' keyword so users may 'nft monitor (new|destroy) hooks'.

Cheers, Phil