[PATCH nf] netfilter: nf_tables: do not compare internal table flags on updates

[PATCH nf] netfilter: nf_tables: do not compare internal table flags on updates

[Pablo Neira Ayuso]

Restore skipping transaction if table update does not modify flags.

Fixes: 179d9ba5559a ("netfilter: nf_tables: fix table flag updates")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
This restores:

nft -f -<<EOF
add table ip t { flags dormant ; }
EOF

nft -f -<<EOF
add table ip t
add chain ip t c1 { type filter hook input priority 1; }
add table ip t
add chain ip t c2 { type filter hook input priority 2; }
EOF

after c9bd26513b3a ("netfilter: nf_tables: disable toggling dormant
table state more than once") which IMO is not the real issue.

This provides an alternative fix for:
[PATCH nf] netfilter: nf_tables: fix consistent table updates being rejected

 net/netfilter/nf_tables_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index fb319a3cd923..2d8828dbe980 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1211,7 +1211,7 @@ static int nf_tables_updtable(struct nft_ctx *ctx)
 	if (flags & ~NFT_TABLE_F_MASK)
 		return -EOPNOTSUPP;
 
-	if (flags == ctx->table->flags)
+	if (flags == (ctx->table->flags & NFT_TABLE_F_MASK))
 		return 0;
 
 	if ((nft_table_has_owner(ctx->table) &&
-- 
2.30.2

Re: [PATCH nf] netfilter: nf_tables: do not compare internal table flags on updates

[Quan Tian]

Hi Pablo,

On Thu, Mar 14, 2024 at 09:16:02PM +0100, Pablo Neira Ayuso wrote:
> Restore skipping transaction if table update does not modify flags.
> 
> Fixes: 179d9ba5559a ("netfilter: nf_tables: fix table flag updates")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> This restores:
> 
> nft -f -<<EOF
> add table ip t { flags dormant ; }
> EOF
> 
> nft -f -<<EOF
> add table ip t
> add chain ip t c1 { type filter hook input priority 1; }
> add table ip t
> add chain ip t c2 { type filter hook input priority 2; }
> EOF
> 
> after c9bd26513b3a ("netfilter: nf_tables: disable toggling dormant
> table state more than once") which IMO is not the real issue.
> 
> This provides an alternative fix for:
> [PATCH nf] netfilter: nf_tables: fix consistent table updates being rejected

The alternative fix definitely makes sense. But I thought "[PATCH nf]
netfilter: nf_tables: fix consistent table updates being rejected" also
fixes the case that two individual updates updating different flags in
a batch, for example:

* The 1st update adopts an orphan table, NFT_TABLE_F_OWNER and
__NFT_TABLE_F_WAS_ORPHAN were turned on.
* The 2nd update activates/inactivates it, trying to turn off/on
NFT_TABLE_F_DORMANT, which would be rejected currently if it only
checks if any flag is set in __NFT_TABLE_F_UPDATE, I thought it's
not the intention according to the code comments.

Thanks,
Quan

Re: [PATCH nf] netfilter: nf_tables: do not compare internal table flags on updates

[Pablo Neira Ayuso]

On Fri, Mar 15, 2024 at 09:06:42AM +0000, Quan Tian wrote:
> Hi Pablo,
> 
> On Thu, Mar 14, 2024 at 09:16:02PM +0100, Pablo Neira Ayuso wrote:
> > Restore skipping transaction if table update does not modify flags.
> > 
> > Fixes: 179d9ba5559a ("netfilter: nf_tables: fix table flag updates")
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> > This restores:
> > 
> > nft -f -<<EOF
> > add table ip t { flags dormant ; }
> > EOF
> > 
> > nft -f -<<EOF
> > add table ip t
> > add chain ip t c1 { type filter hook input priority 1; }
> > add table ip t
> > add chain ip t c2 { type filter hook input priority 2; }
> > EOF
> > 
> > after c9bd26513b3a ("netfilter: nf_tables: disable toggling dormant
> > table state more than once") which IMO is not the real issue.
> > 
> > This provides an alternative fix for:
> > [PATCH nf] netfilter: nf_tables: fix consistent table updates being rejected
> 
> The alternative fix definitely makes sense. But I thought "[PATCH nf]
> netfilter: nf_tables: fix consistent table updates being rejected" also
> fixes the case that two individual updates updating different flags in
> a batch, for example:
> 
> * The 1st update adopts an orphan table, NFT_TABLE_F_OWNER and
> __NFT_TABLE_F_WAS_ORPHAN were turned on.
> * The 2nd update activates/inactivates it, trying to turn off/on
> NFT_TABLE_F_DORMANT, which would be rejected currently if it only
> checks if any flag is set in __NFT_TABLE_F_UPDATE, I thought it's
> not the intention according to the code comments.

NFT_TABLE_F_OWNER to pick up an orphan table is a new feature in 6.8.

You are correct this is not allowed. I am inclined not to support
NFT_TABLE_F_OWNER (on an orphan table) and NFT_TABLE_F_DORMANT in two
separated updates in the same batch, unless someone comes with a
use-case for this.

Thanks.