Re: [nf PATCH v2 7/8] netfilter: nf_tables: Pass reset bit in nft_set_dump_ctx

[Phil Sutter <phil@nwl.cc>]

On Fri, Sep 29, 2023 at 12:56:15PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Sep 29, 2023 at 12:18:28PM +0200, Phil Sutter wrote:
> > On Fri, Sep 29, 2023 at 12:15:16PM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Sep 29, 2023 at 12:08:18PM +0200, Phil Sutter wrote:
> > > > On Thu, Sep 28, 2023 at 08:53:11PM +0200, Pablo Neira Ayuso wrote:
> > > > > On Thu, Sep 28, 2023 at 06:52:43PM +0200, Phil Sutter wrote:
> > > > > > Relieve the dump callback from having to check nlmsg_type upon each
> > > > > > call. Prep work for set element reset locking.
> > > > > 
> > > > > Maybe add this as a preparation patch first place in this series,
> > > > > rather making this cleanup at this late stage of the batch.
> > > > 
> > > > Sure, no problem. I extracted it from v1 of patch 8 and so they are
> > > > closely related.
> > > > 
> > > > Maybe I should split the series up in per-callback ones? I'd start with
> > > > the getsetelem_reset one as that is most cumbersome it seems.
> > > 
> > > Thanks.
> > > 
> > > Side note: I also read a comment from Florian regarding the use of
> > > ctx.table. You have to be very careful with what you cache in the dump
> > > context area, since such pointer might just go away.
> > > 
> > > So far this code caches was "careful" to cache only to check if the
> > > table was still there, but iterating over the table list again
> > > (another safer approach could be to use the table handle which is
> > > unique).
> > > 
> > > All this is also related to the chunked nature of netlink dumps
> > > (in other words, userspace retrieves part of it in every
> > > netlink_recvmsg() call).
> > 
> > Good point. I think we may reduce all this to 'strdup(table->name)' and
> > not care what happens in other CPUs. The only requirement is to cache
> > table->family for audit logging also (IIRC). I'll give this a try.
> 
> table handle is u64 and you don't need to clone, right? Handle
> allocation is monotonic.

The intention was to use it for audit logging as well which requires the
name (and family). Pulling anything that accesses the cached data into
the critical section is probably more straightforward.