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

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

[Phil Sutter]

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.

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.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_tables_api.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index a75dcce2c6c4..ef89298cd11a 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1198,24 +1198,21 @@ static void nf_tables_table_disable(struct net *net, struct nft_table *table)
 static int nf_tables_updtable(struct nft_ctx *ctx)
 {
 	struct nft_trans *trans;
-	u32 flags;
+	u32 flags = 0;
 	int ret;
 
-	if (!ctx->nla[NFTA_TABLE_FLAGS])
-		return 0;
+	if (ctx->nla[NFTA_TABLE_FLAGS])
+		flags = ntohl(nla_get_be32(ctx->nla[NFTA_TABLE_FLAGS]));
 
-	flags = ntohl(nla_get_be32(ctx->nla[NFTA_TABLE_FLAGS]));
 	if (flags & ~NFT_TABLE_F_MASK)
 		return -EOPNOTSUPP;
 
 	if (flags == ctx->table->flags)
 		return 0;
 
-	if ((nft_table_has_owner(ctx->table) &&
-	     !(flags & NFT_TABLE_F_OWNER)) ||
-	    (!nft_table_has_owner(ctx->table) &&
-	     flags & NFT_TABLE_F_OWNER))
-		return -EOPNOTSUPP;
+	if (nft_table_has_owner(ctx->table) &&
+	    ctx->table->nlpid != ctx->portid)
+		return -EPERM;
 
 	/* No dormant off/on/off/on games in single transaction */
 	if (ctx->table->flags & __NFT_TABLE_F_UPDATE)
@@ -1226,6 +1223,14 @@ static int nf_tables_updtable(struct nft_ctx *ctx)
 	if (trans == NULL)
 		return -ENOMEM;
 
+	if (flags & NFT_TABLE_F_OWNER) {
+		ctx->table->flags |= NFT_TABLE_F_OWNER;
+		ctx->table->nlpid = ctx->portid;
+	} else if (nft_table_has_owner(ctx->table)) {
+		ctx->table->flags &= ~NFT_TABLE_F_OWNER;
+		ctx->table->nlpid = 0;
+	}
+
 	if ((flags & NFT_TABLE_F_DORMANT) &&
 	    !(ctx->table->flags & NFT_TABLE_F_DORMANT)) {
 		ctx->table->flags |= NFT_TABLE_F_DORMANT;
-- 
2.41.0

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

[Pablo Neira Ayuso]

Hi Phil,

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.

Is there any more scenario are you having in mind for this?

> 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.

Maybe also goal in this case is to keep counters (and other stateful
objects) around?

Thanks.

> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  net/netfilter/nf_tables_api.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index a75dcce2c6c4..ef89298cd11a 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -1198,24 +1198,21 @@ static void nf_tables_table_disable(struct net *net, struct nft_table *table)
>  static int nf_tables_updtable(struct nft_ctx *ctx)
>  {
>  	struct nft_trans *trans;
> -	u32 flags;
> +	u32 flags = 0;
>  	int ret;
>  
> -	if (!ctx->nla[NFTA_TABLE_FLAGS])
> -		return 0;
> +	if (ctx->nla[NFTA_TABLE_FLAGS])
> +		flags = ntohl(nla_get_be32(ctx->nla[NFTA_TABLE_FLAGS]));
>  
> -	flags = ntohl(nla_get_be32(ctx->nla[NFTA_TABLE_FLAGS]));
>  	if (flags & ~NFT_TABLE_F_MASK)
>  		return -EOPNOTSUPP;
>  
>  	if (flags == ctx->table->flags)
>  		return 0;
>  
> -	if ((nft_table_has_owner(ctx->table) &&
> -	     !(flags & NFT_TABLE_F_OWNER)) ||
> -	    (!nft_table_has_owner(ctx->table) &&
> -	     flags & NFT_TABLE_F_OWNER))
> -		return -EOPNOTSUPP;
> +	if (nft_table_has_owner(ctx->table) &&
> +	    ctx->table->nlpid != ctx->portid)
> +		return -EPERM;
>  
>  	/* No dormant off/on/off/on games in single transaction */
>  	if (ctx->table->flags & __NFT_TABLE_F_UPDATE)
> @@ -1226,6 +1223,14 @@ static int nf_tables_updtable(struct nft_ctx *ctx)
>  	if (trans == NULL)
>  		return -ENOMEM;
>  
> +	if (flags & NFT_TABLE_F_OWNER) {
> +		ctx->table->flags |= NFT_TABLE_F_OWNER;
> +		ctx->table->nlpid = ctx->portid;
> +	} else if (nft_table_has_owner(ctx->table)) {
> +		ctx->table->flags &= ~NFT_TABLE_F_OWNER;
> +		ctx->table->nlpid = 0;
> +	}
> +
>  	if ((flags & NFT_TABLE_F_DORMANT) &&
>  	    !(ctx->table->flags & NFT_TABLE_F_DORMANT)) {
>  		ctx->table->flags |= NFT_TABLE_F_DORMANT;
> -- 
> 2.41.0
> 

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

[Phil Sutter]

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. 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?

> 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.

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?

> > 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.

> 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.

Cheers, Phil

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

[Eric Garver]

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.

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

[Phil Sutter]

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.

[...]
> > 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.

Cheers, Phil

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

[Eric Garver]

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.

I'd rather see the daemon be automatically restarted. After a crash you
still have a flush + re-apply on daemon restart. Avoiding the cleanup
due to table owner flag only shortens the window.

I think we're getting off topic for this list. Let's discuss off list if
you want. :)

[..]

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

[Pablo Neira Ayuso]

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.

- add another flag to keep the ruleset around when the owner process
  goes away.

Probably it can be the same flag for both cases.

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.

> [...]
> > > 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.

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

[Florian Westphal]

Eric Garver <eric@garver.life> 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.
> 
> I'd rather see the daemon be automatically restarted. After a crash you
> still have a flush + re-apply on daemon restart. Avoiding the cleanup
> due to table owner flag only shortens the window.

But the filter rules are gone for a short time, leaving e.g. an
ipv6 network we're routing for wide open.

Same for any exposed containers or VMs.
So I'd say as-is the owner flag is harmful for filtering.

I'm fine with adding a flag that keeps the orphaned table around
and allows to (re)take ownership.

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

[Phil Sutter]

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

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

[Pablo Neira Ayuso]

On Wed, Dec 13, 2023 at 04:51:02PM +0100, Phil Sutter wrote:
> On Wed, Dec 13, 2023 at 04:15:50PM +0100, Pablo Neira Ayuso wrote:
[...]
> 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?

I think so, otherwise such orphaned table will become an inmortal
zombie that noone can remove :)

[...]
> > 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.

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

[Thomas Haller]

On Wed, 2023-12-13 at 16:15 +0100, Pablo Neira Ayuso wrote:
> On Wed, Dec 13, 2023 at 01:13:54PM +0100, Phil Sutter wrote:
> 
> > > 
> > > 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.
> 

Isn't the problem to solve that `nft flush ruleset` deletes tables
owned by somebody else (firewalld)?

Using the owner flag for that seems wrong, if the overall semantics of
that flag are not desired.

A "persist" flag sounds like a good solution. It would just have
informational value (for user space) to be skipped by `nft flush
ruleset`.


Thomas

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

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> Isn't the problem to solve that `nft flush ruleset` deletes tables
> owned by somebody else (firewalld)?

If they are 'owned', then no, they are not flushed, thats one of the
points of the owner thing.

> A "persist" flag sounds like a good solution. It would just have
> informational value (for user space) to be skipped by `nft flush
> ruleset`.

'flush' doesn't pass the to-be deleted tables to the kernel, so
this cannot be implemented via informational tags in userspace.

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

[Thomas Haller]

On Wed, 2023-12-13 at 17:45 +0100, Florian Westphal wrote:
> Thomas Haller <thaller@redhat.com> wrote:
> > Isn't the problem to solve that `nft flush ruleset` deletes tables
> > owned by somebody else (firewalld)?
> 
> If they are 'owned', then no, they are not flushed, thats one of the
> points of the owner thing.

With "tables owned by somebody else", I meant to be logically owned by
firewalld (while not having NFT_TABLE_F_OWNER flag). Sorry for being
unclear.

> 
> > A "persist" flag sounds like a good solution. It would just have
> > informational value (for user space) to be skipped by `nft flush
> > ruleset`.
> 
> 'flush' doesn't pass the to-be deleted tables to the kernel, so
> this cannot be implemented via informational tags in userspace.
> 

I see. Thanks.


Thomas