Skip to content

Commit cff3bd0

Browse files
Florian Westphalummakynes
authored andcommitted
netfilter: nf_tables: prefer nft_chain_validate
nft_chain_validate already performs loop detection because a cycle will result in a call stack overflow (ctx->level >= NFT_JUMP_STACK_SIZE). It also follows maps via ->validate callback in nft_lookup, so there appears no reason to iterate the maps again. nf_tables_check_loops() and all its helper functions can be removed. This improves ruleset load time significantly, from 23s down to 12s. This also fixes a crash bug. Old loop detection code can result in unbounded recursion: BUG: TASK stack guard page was hit at .... Oops: stack guard page: 0000 [#1] PREEMPT SMP KASAN CPU: 4 PID: 1539 Comm: nft Not tainted 6.10.0-rc5+ #1 [..] with a suitable ruleset during validation of register stores. I can't see any actual reason to attempt to check for this from nft_validate_register_store(), at this point the transaction is still in progress, so we don't have a full picture of the rule graph. For nf-next it might make sense to either remove it or make this depend on table->validate_state in case we could catch an error earlier (for improved error reporting to userspace). Fixes: 20a6934 ("netfilter: nf_tables: add netlink set API") Signed-off-by: Florian Westphal <[email protected]> Signed-off-by: Pablo Neira Ayuso <[email protected]>
1 parent 631a4b3 commit cff3bd0

File tree

1 file changed

+13
-145
lines changed

1 file changed

+13
-145
lines changed

net/netfilter/nf_tables_api.c

Lines changed: 13 additions & 145 deletions
Original file line numberDiff line numberDiff line change
@@ -3823,6 +3823,15 @@ static void nf_tables_rule_release(const struct nft_ctx *ctx, struct nft_rule *r
38233823
nf_tables_rule_destroy(ctx, rule);
38243824
}
38253825

3826+
/** nft_chain_validate - loop detection and hook validation
3827+
*
3828+
* @ctx: context containing call depth and base chain
3829+
* @chain: chain to validate
3830+
*
3831+
* Walk through the rules of the given chain and chase all jumps/gotos
3832+
* and set lookups until either the jump limit is hit or all reachable
3833+
* chains have been validated.
3834+
*/
38263835
int nft_chain_validate(const struct nft_ctx *ctx, const struct nft_chain *chain)
38273836
{
38283837
struct nft_expr *expr, *last;
@@ -3844,6 +3853,9 @@ int nft_chain_validate(const struct nft_ctx *ctx, const struct nft_chain *chain)
38443853
if (!expr->ops->validate)
38453854
continue;
38463855

3856+
/* This may call nft_chain_validate() recursively,
3857+
* callers that do so must increment ctx->level.
3858+
*/
38473859
err = expr->ops->validate(ctx, expr, &data);
38483860
if (err < 0)
38493861
return err;
@@ -10809,150 +10821,6 @@ int nft_chain_validate_hooks(const struct nft_chain *chain,
1080910821
}
1081010822
EXPORT_SYMBOL_GPL(nft_chain_validate_hooks);
1081110823

10812-
/*
10813-
* Loop detection - walk through the ruleset beginning at the destination chain
10814-
* of a new jump until either the source chain is reached (loop) or all
10815-
* reachable chains have been traversed.
10816-
*
10817-
* The loop check is performed whenever a new jump verdict is added to an
10818-
* expression or verdict map or a verdict map is bound to a new chain.
10819-
*/
10820-
10821-
static int nf_tables_check_loops(const struct nft_ctx *ctx,
10822-
const struct nft_chain *chain);
10823-
10824-
static int nft_check_loops(const struct nft_ctx *ctx,
10825-
const struct nft_set_ext *ext)
10826-
{
10827-
const struct nft_data *data;
10828-
int ret;
10829-
10830-
data = nft_set_ext_data(ext);
10831-
switch (data->verdict.code) {
10832-
case NFT_JUMP:
10833-
case NFT_GOTO:
10834-
ret = nf_tables_check_loops(ctx, data->verdict.chain);
10835-
break;
10836-
default:
10837-
ret = 0;
10838-
break;
10839-
}
10840-
10841-
return ret;
10842-
}
10843-
10844-
static int nf_tables_loop_check_setelem(const struct nft_ctx *ctx,
10845-
struct nft_set *set,
10846-
const struct nft_set_iter *iter,
10847-
struct nft_elem_priv *elem_priv)
10848-
{
10849-
const struct nft_set_ext *ext = nft_set_elem_ext(set, elem_priv);
10850-
10851-
if (!nft_set_elem_active(ext, iter->genmask))
10852-
return 0;
10853-
10854-
if (nft_set_ext_exists(ext, NFT_SET_EXT_FLAGS) &&
10855-
*nft_set_ext_flags(ext) & NFT_SET_ELEM_INTERVAL_END)
10856-
return 0;
10857-
10858-
return nft_check_loops(ctx, ext);
10859-
}
10860-
10861-
static int nft_set_catchall_loops(const struct nft_ctx *ctx,
10862-
struct nft_set *set)
10863-
{
10864-
u8 genmask = nft_genmask_next(ctx->net);
10865-
struct nft_set_elem_catchall *catchall;
10866-
struct nft_set_ext *ext;
10867-
int ret = 0;
10868-
10869-
list_for_each_entry_rcu(catchall, &set->catchall_list, list) {
10870-
ext = nft_set_elem_ext(set, catchall->elem);
10871-
if (!nft_set_elem_active(ext, genmask))
10872-
continue;
10873-
10874-
ret = nft_check_loops(ctx, ext);
10875-
if (ret < 0)
10876-
return ret;
10877-
}
10878-
10879-
return ret;
10880-
}
10881-
10882-
static int nf_tables_check_loops(const struct nft_ctx *ctx,
10883-
const struct nft_chain *chain)
10884-
{
10885-
const struct nft_rule *rule;
10886-
const struct nft_expr *expr, *last;
10887-
struct nft_set *set;
10888-
struct nft_set_binding *binding;
10889-
struct nft_set_iter iter;
10890-
10891-
if (ctx->chain == chain)
10892-
return -ELOOP;
10893-
10894-
if (fatal_signal_pending(current))
10895-
return -EINTR;
10896-
10897-
list_for_each_entry(rule, &chain->rules, list) {
10898-
nft_rule_for_each_expr(expr, last, rule) {
10899-
struct nft_immediate_expr *priv;
10900-
const struct nft_data *data;
10901-
int err;
10902-
10903-
if (strcmp(expr->ops->type->name, "immediate"))
10904-
continue;
10905-
10906-
priv = nft_expr_priv(expr);
10907-
if (priv->dreg != NFT_REG_VERDICT)
10908-
continue;
10909-
10910-
data = &priv->data;
10911-
switch (data->verdict.code) {
10912-
case NFT_JUMP:
10913-
case NFT_GOTO:
10914-
err = nf_tables_check_loops(ctx,
10915-
data->verdict.chain);
10916-
if (err < 0)
10917-
return err;
10918-
break;
10919-
default:
10920-
break;
10921-
}
10922-
}
10923-
}
10924-
10925-
list_for_each_entry(set, &ctx->table->sets, list) {
10926-
if (!nft_is_active_next(ctx->net, set))
10927-
continue;
10928-
if (!(set->flags & NFT_SET_MAP) ||
10929-
set->dtype != NFT_DATA_VERDICT)
10930-
continue;
10931-
10932-
list_for_each_entry(binding, &set->bindings, list) {
10933-
if (!(binding->flags & NFT_SET_MAP) ||
10934-
binding->chain != chain)
10935-
continue;
10936-
10937-
iter.genmask = nft_genmask_next(ctx->net);
10938-
iter.type = NFT_ITER_UPDATE;
10939-
iter.skip = 0;
10940-
iter.count = 0;
10941-
iter.err = 0;
10942-
iter.fn = nf_tables_loop_check_setelem;
10943-
10944-
set->ops->walk(ctx, set, &iter);
10945-
if (!iter.err)
10946-
iter.err = nft_set_catchall_loops(ctx, set);
10947-
10948-
if (iter.err < 0)
10949-
return iter.err;
10950-
}
10951-
}
10952-
10953-
return 0;
10954-
}
10955-
1095610824
/**
1095710825
* nft_parse_u32_check - fetch u32 attribute and check for maximum value
1095810826
*
@@ -11065,7 +10933,7 @@ static int nft_validate_register_store(const struct nft_ctx *ctx,
1106510933
if (data != NULL &&
1106610934
(data->verdict.code == NFT_GOTO ||
1106710935
data->verdict.code == NFT_JUMP)) {
11068-
err = nf_tables_check_loops(ctx, data->verdict.chain);
10936+
err = nft_chain_validate(ctx, data->verdict.chain);
1106910937
if (err < 0)
1107010938
return err;
1107110939
}

0 commit comments

Comments
 (0)