netfilter: nf_tables: do not defer rule destruction via call_rcu

nf_tables_chain_destroy can sleep, it can't be used from call_rcu
callbacks.

Moreover, nf_tables_rule_release() is only safe for error unwinding,
while transaction mutex is held and the to-be-desroyed rule was not
exposed to either dataplane or dumps, as it deactives+frees without
the required synchronize_rcu() in-between.

nft_rule_expr_deactivate() callbacks will change ->use counters
of other chains/sets, see e.g. nft_lookup .deactivate callback, these
must be serialized via transaction mutex.

Also add a few lockdep asserts to make this more explicit.

Calling synchronize_rcu() isn't ideal, but fixing this without is hard
and way more intrusive.  As-is, we can get:

WARNING: .. net/netfilter/nf_tables_api.c:5515 nft_set_destroy+0x..
Workqueue: events nf_tables_trans_destroy_work
RIP: 0010:nft_set_destroy+0x3fe/0x5c0
Call Trace:
 <TASK>
 nf_tables_trans_destroy_work+0x6b7/0xad0
 process_one_work+0x64a/0xce0
 worker_thread+0x613/0x10d0

In case the synchronize_rcu becomes an issue, we can explore alternatives.

One way would be to allocate nft_trans_rule objects + one nft_trans_chain
object, deactivate the rules + the chain and then defer the freeing to the
nft destroy workqueue.  We'd still need to keep the synchronize_rcu path as
a fallback to handle -ENOMEM corner cases though.

Reported-by: syzbot+b26935466701e56cfdc2@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/67478d92.050a0220.253251.0062.GAE@google.com/T/
Fixes: c03d278fdf35 ("netfilter: nf_tables: wait for rcu grace period on net_device removal")
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 80a537ac26cd2fc085cfb8188ad9a12888f6c65d..4afa64c813045f2b334d1a1f7b5b2ad8da04dd44 100644 (file)
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1103,7 +1103,6 @@ struct nft_rule_blob {
  *     @name: name of the chain
  *     @udlen: user data length
  *     @udata: user data in the chain
- *     @rcu_head: rcu head for deferred release
  *     @blob_next: rule blob pointer to the next in the chain
  */
 struct nft_chain {
@@ -1121,7 +1120,6 @@ struct nft_chain {
        char                            *name;
        u16                             udlen;
        u8                              *udata;
-       struct rcu_head                 rcu_head;
 
        /* Only used during control plane commit phase: */
        struct nft_rule_blob            *blob_next;
@@ -1265,7 +1263,6 @@ static inline void nft_use_inc_restore(u32 *use)
  *     @sets: sets in the table
  *     @objects: stateful objects in the table
  *     @flowtables: flow tables in the table
- *     @net: netnamespace this table belongs to
  *     @hgenerator: handle generator state
  *     @handle: table handle
  *     @use: number of chain references to this table
@@ -1285,7 +1282,6 @@ struct nft_table {
        struct list_head                sets;
        struct list_head                objects;
        struct list_head                flowtables;
-       possible_net_t                  net;
        u64                             hgenerator;
        u64                             handle;
        u32                             use;

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 21b6f7410a1f545b8eb3323196f47c41179bf573..0b9f1e8dfe49b8e15365e5a5a1fa8ef751ac9877 100644 (file)
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1596,7 +1596,6 @@ static int nf_tables_newtable(struct sk_buff *skb, const struct nfnl_info *info,
        INIT_LIST_HEAD(&table->sets);
        INIT_LIST_HEAD(&table->objects);
        INIT_LIST_HEAD(&table->flowtables);
-       write_pnet(&table->net, net);
        table->family = family;
        table->flags = flags;
        table->handle = ++nft_net->table_handle;
@@ -3987,8 +3986,11 @@ void nf_tables_rule_destroy(const struct nft_ctx *ctx, struct nft_rule *rule)
        kfree(rule);
 }
 
+/* can only be used if rule is no longer visible to dumps */
 static void nf_tables_rule_release(const struct nft_ctx *ctx, struct nft_rule *rule)
 {
+       lockdep_commit_lock_is_held(ctx->net);
+
        nft_rule_expr_deactivate(ctx, rule, NFT_TRANS_RELEASE);
        nf_tables_rule_destroy(ctx, rule);
 }
@@ -5757,6 +5759,8 @@ void nf_tables_deactivate_set(const struct nft_ctx *ctx, struct nft_set *set,
                              struct nft_set_binding *binding,
                              enum nft_trans_phase phase)
 {
+       lockdep_commit_lock_is_held(ctx->net);
+
        switch (phase) {
        case NFT_TRANS_PREPARE_ERROR:
                nft_set_trans_unbind(ctx, set);
@@ -11695,19 +11699,6 @@ static void __nft_release_basechain_now(struct nft_ctx *ctx)
        nf_tables_chain_destroy(ctx->chain);
 }
 
-static void nft_release_basechain_rcu(struct rcu_head *head)
-{
-       struct nft_chain *chain = container_of(head, struct nft_chain, rcu_head);
-       struct nft_ctx ctx = {
-               .family = chain->table->family,
-               .chain  = chain,
-               .net    = read_pnet(&chain->table->net),
-       };
-
-       __nft_release_basechain_now(&ctx);
-       put_net(ctx.net);
-}
-
 int __nft_release_basechain(struct nft_ctx *ctx)
 {
        struct nft_rule *rule;
@@ -11722,11 +11713,18 @@ int __nft_release_basechain(struct nft_ctx *ctx)
        nft_chain_del(ctx->chain);
        nft_use_dec(&ctx->table->use);
 
-       if (maybe_get_net(ctx->net))
-               call_rcu(&ctx->chain->rcu_head, nft_release_basechain_rcu);
-       else
+       if (!maybe_get_net(ctx->net)) {
                __nft_release_basechain_now(ctx);
+               return 0;
+       }
+
+       /* wait for ruleset dumps to complete.  Owning chain is no longer in
+        * lists, so new dumps can't find any of these rules anymore.
+        */
+       synchronize_rcu();
 
+       __nft_release_basechain_now(ctx);
+       put_net(ctx->net);
        return 0;
 }
 EXPORT_SYMBOL_GPL(__nft_release_basechain);