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

[Phil Sutter <phil@nwl.cc>]

On Wed, Dec 13, 2023 at 04:15:50PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Dec 13, 2023 at 01:13:54PM +0100, Phil Sutter wrote:
> > Hi,
> > 
> > On Tue, Dec 12, 2023 at 05:47:22PM -0500, Eric Garver wrote:
> > > 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.
> > 
> > The problem with crashes is tables with owner flag set will vanish,
> > leaving the system without a firewall.
> 
> So it does currently in a normal process exit.
> 
> Reading all this, a few choices:
> 
> - add an 'orphan' flag that gets set on if the owner process goes
>   away, so only ruleset with such flag can be retaken. This is to
>   avoid allowing a process to take any other ruleset in place.

That's an interesting idea, implementing it should be easy indeed. I
wonder though if this "takeover" protection is effective: The table not
having an owner yet may be deleted and recreated (with owner flag) by
any other process, effectively this is the same as the "takeover" you
probably want to prevent by limiting the add owner update to tables with
'orphan' flag.

> - add another flag to keep the ruleset around when the owner process
>   goes away.
> 
> Probably it can be the same flag for both cases.

ACK: A table may become orphan only if there was an owner in the first
place and it survives. So 'orphan' flag may well be a virtual one for
user space only based on '(flags & (owner|persist)) && (nlpid == 0)'.

> I remember we discussed these superficially at the time that the
> 'owner' flag was introduced, but there were not many use-cases in
> place already, and the goal for the 'owner' flag is to prevent an
> accidental zapping of the ruleset via 'nft flush ruleset' by another
> process.

I find it sensible to protect a table only as long as the owning process
remains alive, at least to prevent zombie tables. This raises the
question what shall happen to orphan tables upon 'nft flush ruleset'?
Flush them like a regular one?

> > [...]
> > > > 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.
> > 
> > The benefit of using it is to avoid interference from other users
> > calling 'nft flush ruleset'. Introducing a "persist" flag would enable
> > this while avoiding the restart/crash downtime.
> 
> I think this 'persist' flag provides semantics the described above,
> that is:
> 
> - keep it in place if process goes away.
> - allow to retake ownership.

I'll give it a try.

Thanks, Phil