[PATCH nf] netfilter: nf_tables: don't unregister hook when table is dormant

[PATCH nf] netfilter: nf_tables: don't unregister hook when table is dormant

[Florian Westphal]

When nf_tables_updchain encounters an error, hook registration needs to
be rolled back.

This should only be done if the hook has been registered, which won't
happen when the table is flagged as dormant (inactive).

Just move the assignment into the registration block.

Reported-by: syzbot+53ed3a6440173ddbf499@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=53ed3a6440173ddbf499
Fixes: b9703ed44ffb ("netfilter: nf_tables: support for adding new devices to an existing netdev chain")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_tables_api.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index c2df81b7e950..567cea329e1f 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2840,9 +2840,9 @@ static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
 			if (err < 0)
 				goto err_hooks;
 		}
-	}
 
-	unregister = true;
+		unregister = true;
+	}
 
 	if (nla[NFTA_CHAIN_COUNTERS]) {
 		if (!nft_is_base_chain(chain)) {
-- 
2.49.0

Re: [PATCH nf] netfilter: nf_tables: don't unregister hook when table is dormant

[Pablo Neira Ayuso]

Hi Florian,

On Tue, Apr 01, 2025 at 02:36:47PM +0200, Florian Westphal wrote:
> When nf_tables_updchain encounters an error, hook registration needs to
> be rolled back.
> 
> This should only be done if the hook has been registered, which won't
> happen when the table is flagged as dormant (inactive).
> 
> Just move the assignment into the registration block.

I just made another pass today on this, I think this needs to be:

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index c2df81b7e950..a133e1c175ce 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2839,11 +2839,11 @@ static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
                        err = nft_netdev_register_hooks(ctx->net, &hook.list);
                        if (err < 0)
                                goto err_hooks;
+
+                       unregister = true;
                }
        }
 
-       unregister = true;
-
        if (nla[NFTA_CHAIN_COUNTERS]) {
                if (!nft_is_base_chain(chain)) {
                        err = -EOPNOTSUPP;

This is the rationale:

- only updchain allows for adding new basechains for netdev family,
  this is to add new devices to this netdev basechain.
- !netdev families only check if the hook is the same in the chain
  update. Otherwise, it reports EEXIST. That is, chain update of
  !netdev families is not possible.

I am moving "unregister = true" under this check:

  if (nft_base_chain_netdev(table->family, basechain->ops.hooknum)) {

only in this case hook registrations can happen. Then, in case of
error, unwind only applies if basechain's family is netdev.

Re: [PATCH nf] netfilter: nf_tables: don't unregister hook when table is dormant

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> I just made another pass today on this, I think this needs to be:
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index c2df81b7e950..a133e1c175ce 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -2839,11 +2839,11 @@ static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
>                         err = nft_netdev_register_hooks(ctx->net, &hook.list);
>                         if (err < 0)
>                                 goto err_hooks;
> +
> +                       unregister = true;
>                 }
>         }
>  
> -       unregister = true;
> -
>         if (nla[NFTA_CHAIN_COUNTERS]) {
>                 if (!nft_is_base_chain(chain)) {
>                         err = -EOPNOTSUPP;
> 
> This is the rationale:

[..]

I've marked the patch as rejected.  I'm not sure what the pre and
postconditions for non-netdev is in this function, so I won't send a v2.