Kernel/linux-stable
1
0
Fork 0
mirror of https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git synced 2024-12-28 16:56:26 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

netfilter: nf_tables: do not defer rule destruction via call_rcu

Browse Source 
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: c03d278fdf ("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>
This commit is contained in:
Florian Westphal 2024-12-07 12:14:48 +01:00 committed by Pablo Neira Ayuso
parent f36b01994d
commit b04df3da1b
2 changed files with 15 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
include/net/netfilter/nf_tables.h
View File

@ -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;

32
net/netfilter/nf_tables_api.c
View File

@ -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);