Re: [nf-next PATCH] netfilter: nf_tables: Support updating table's owner flag

[Eric Garver <eric@garver.life>]

On Tue, Dec 12, 2023 at 05:23:03PM +0100, Phil Sutter wrote:
> Hi Pablo,
> 
> On Tue, Dec 12, 2023 at 02:08:50PM +0100, Pablo Neira Ayuso wrote:
> > On Fri, Dec 08, 2023 at 02:01:03PM +0100, Phil Sutter wrote:
> > > A process may take ownership of an existing table not owned yet or free
> > > a table it owns already.
> > > 
> > > A practical use-case is Firewalld's CleanupOnExit=no option: If it
> > > starts creating its own tables with owner flag, dropping that flag upon
> > > program exit is the easiest solution to make the ruleset survive.
> > 
> > I can think of a package update as use-case for this feature?
> > Meanwhile, package is being updated the ruleset remains in place.
> 
> Usually (with the distros I am familiar with at least), the daemon just
> keeps running while its package is updated. The run-time change then
> happens after reboot (or explicit restart). RHEL/Fedora support
> '%systemd_postun_with_restart' macro to request restart of the service
> upon package update, but it runs after the actual update process, so
> the time-window in between old service and new one is short (in theory).
> 
> Unless I'm mistaken, firewalld service restart is internally just "stop
> && start", not a distinct action.

Yes. This is typical. "systemctl restart firewalld". This is what's done
on a package update.

> Temporarily changing the config to
> make firewalld not clean up in that case to reduce/eliminate the
> downtime is a nice idea, though. Eric, WDYT?

It would be nice to eliminate the downtime, yes.

The original intention of CleanupOnExit is to allow shutting down the
daemon while retaining the runtime nftables rules, i.e. zero cost
abstraction.

> > Is there any more scenario are you having in mind for this?
> 
> No, it was basically just that. When discussing with Eric whether using
> 'flags owner' is good (to avoid clashes with other nf_tables users) or
> bad (ruleset is lost upon (unexpected) program exit), I thought of a
> switchable owner flag as a nice alternative to dropping and recreating
> the owned tables without owner flag before exiting.

It would be nice, but not a show stopper.

> BTW: A known limitation is that crashing firewalld will leave the system
> without ruleset. I could think of a second flag, "persist" or so, which
> makes nft_rcv_nl_event() just drop the owner flag from the table instead
> of deleting it. What do you think?

I'm not concerned with optimizing for the crash case. We wouldn't be
able to make any assumptions about the state of nftables. The only safe
option is to flush and reload all the rules.

> > > Mostly for consistency, this patch enables taking ownership of an
> > > existing table, too. This would allow firewalld to retake the ruleset it
> > > has previously left.
> > 
> > Isn't it better to start from scratch? Basically, flush previous the
> > table that you know it was there and reload the ruleset.
> 
> Yes, this is what firewalld currently does. Looking at the package
> update scenario you mentioned, a starting daemon can't really expect the
> existing table to be in shape and should better just recreate it from
> scratch.

Indeed. Always flush at start. Same as after a crash, IMO.

> > Maybe also goal in this case is to keep counters (and other stateful
> > objects) around?
> 
> Yes, this is a nice side-effect, too.
> 
> In my opinion, support for owner flag update (both add and remove) is
> simple enough to maintain in code and relatively straightforward
> regarding security (if owned tables may only be changed by the owner) so
> there is not much reason to not provide it for whoever may find use in
> it.
> 
> For firewalld on the other hand, I think introducing this "persist" flag
> would be a full replacement to the proposed owner flag update.

I don't think we need a persist flag. If we want it to persist then
we'll just avoid setting the owner flag entirely.