* [PATCH nf-next 1/3] netfilter: nf_tables: merge nft_rules_old structure and end of ruleblob marker
2023-04-11 14:29 [PATCH nf-next 0/3] netfilter: nf_tables: shrink jump stack size Florian Westphal
@ 2023-04-11 14:29 ` Florian Westphal
2023-04-11 14:29 ` [PATCH nf-next 2/3] netfilter: nf_tables: don't store address of last rule on jump Florian Westphal
2023-04-11 14:29 ` [PATCH nf-next 3/3] netfilter: nf_tables: don't store chain address " Florian Westphal
2 siblings, 0 replies; 4+ messages in thread
From: Florian Westphal @ 2023-04-11 14:29 UTC (permalink / raw)
To: netfilter-devel; +Cc: Florian Westphal
In order to free the rules in a chain via call_rcu, the rule array used
to stash a rcu_head and space for a pointer at the end of the rule array.
When the current nft_rule_dp blob format got added in
2c865a8a28a1 ("netfilter: nf_tables: add rule blob layout"), this results
in a double-trailer:
size (unsigned long)
struct nft_rule_dp
struct nft_expr
...
struct nft_rule_dp
struct nft_expr
...
struct nft_rule_dp (is_last=1) // Trailer
The trailer, struct nft_rule_dp (is_last=1), is not accounted for in size,
so it can be located via start_addr + size.
Because the rcu_head is stored after 'start+size' as well this means the
is_last trailer is *aliased* to the rcu_head (struct nft_rules_old).
This is harmless, because at this time the nft_do_chain function never
evaluates/accesses the trailer, it only checks the address boundary:
for (; rule < last_rule; rule = nft_rule_next(rule)) {
...
But this way the last_rule address has to be stashed in the jump
structure to restore it after returning from a chain.
nft_do_chain stack usage has become way too big, so put it on a diet.
Without this patch is impossible to use
for (; !rule->is_last; rule = nft_rule_next(rule)) {
... because on free, the needed update of the rcu_head will clobber the
nft_rule_dp is_last bit.
Furthermore, also stash the chain pointer in the trailer, this allows
to recover the original chain structure from nf_tables_trace infra
without a need to place them in the jump struct.
After this patch it is trivial to diet the jump stack structure,
done in the next two patches.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/netfilter/nf_tables_api.c | 55 +++++++++++++++++------------------
1 file changed, 27 insertions(+), 28 deletions(-)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 6004d4b24451..f89fa158ead9 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2110,38 +2110,41 @@ static void nft_chain_release_hook(struct nft_chain_hook *hook)
module_put(hook->type->owner);
}
-struct nft_rules_old {
+struct nft_rule_dp_last {
+ struct nft_rule_dp end; /* end of nft_rule_blob marker */
struct rcu_head h;
struct nft_rule_blob *blob;
+ const struct nft_chain *chain; /* for tracing */
};
-static void nft_last_rule(struct nft_rule_blob *blob, const void *ptr)
+static void nft_last_rule(const struct nft_chain *chain, const void *ptr)
{
- struct nft_rule_dp *prule;
+ struct nft_rule_dp_last *lrule;
+
+ BUILD_BUG_ON(offsetof(struct nft_rule_dp_last, end) != 0);
- prule = (struct nft_rule_dp *)ptr;
- prule->is_last = 1;
+ lrule = (struct nft_rule_dp_last *)ptr;
+ lrule->end.is_last = 1;
+ lrule->chain = chain;
/* blob size does not include the trailer rule */
}
-static struct nft_rule_blob *nf_tables_chain_alloc_rules(unsigned int size)
+static struct nft_rule_blob *nf_tables_chain_alloc_rules(const struct nft_chain *chain,
+ unsigned int size)
{
struct nft_rule_blob *blob;
- /* size must include room for the last rule */
- if (size < offsetof(struct nft_rule_dp, data))
- return NULL;
-
- size += sizeof(struct nft_rule_blob) + sizeof(struct nft_rules_old);
if (size > INT_MAX)
return NULL;
+ size += sizeof(struct nft_rule_blob) + sizeof(struct nft_rule_dp_last);
+
blob = kvmalloc(size, GFP_KERNEL_ACCOUNT);
if (!blob)
return NULL;
blob->size = 0;
- nft_last_rule(blob, blob->data);
+ nft_last_rule(chain, blob->data);
return blob;
}
@@ -2220,7 +2223,6 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 genmask,
struct nft_rule_blob *blob;
struct nft_trans *trans;
struct nft_chain *chain;
- unsigned int data_size;
int err;
if (table->use == UINT_MAX)
@@ -2308,8 +2310,7 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 genmask,
chain->udlen = nla_len(nla[NFTA_CHAIN_USERDATA]);
}
- data_size = offsetof(struct nft_rule_dp, data); /* last rule */
- blob = nf_tables_chain_alloc_rules(data_size);
+ blob = nf_tables_chain_alloc_rules(chain, 0);
if (!blob) {
err = -ENOMEM;
goto err_destroy_chain;
@@ -8764,9 +8765,8 @@ static int nf_tables_commit_chain_prepare(struct net *net, struct nft_chain *cha
return -ENOMEM;
}
}
- data_size += offsetof(struct nft_rule_dp, data); /* last rule */
- chain->blob_next = nf_tables_chain_alloc_rules(data_size);
+ chain->blob_next = nf_tables_chain_alloc_rules(chain, data_size);
if (!chain->blob_next)
return -ENOMEM;
@@ -8811,12 +8811,11 @@ static int nf_tables_commit_chain_prepare(struct net *net, struct nft_chain *cha
chain->blob_next->size += (unsigned long)(data - (void *)prule);
}
- prule = (struct nft_rule_dp *)data;
- data += offsetof(struct nft_rule_dp, data);
if (WARN_ON_ONCE(data > data_boundary))
return -ENOMEM;
- nft_last_rule(chain->blob_next, prule);
+ prule = (struct nft_rule_dp *)data;
+ nft_last_rule(chain, prule);
return 0;
}
@@ -8837,22 +8836,22 @@ static void nf_tables_commit_chain_prepare_cancel(struct net *net)
}
}
-static void __nf_tables_commit_chain_free_rules_old(struct rcu_head *h)
+static void __nf_tables_commit_chain_free_rules(struct rcu_head *h)
{
- struct nft_rules_old *o = container_of(h, struct nft_rules_old, h);
+ struct nft_rule_dp_last *l = container_of(h, struct nft_rule_dp_last, h);
- kvfree(o->blob);
+ kvfree(l->blob);
}
static void nf_tables_commit_chain_free_rules_old(struct nft_rule_blob *blob)
{
- struct nft_rules_old *old;
+ struct nft_rule_dp_last *last;
- /* rcu_head is after end marker */
- old = (void *)blob + sizeof(*blob) + blob->size;
- old->blob = blob;
+ /* last rule trailer is after end marker */
+ last = (void *)blob + sizeof(*blob) + blob->size;
+ last->blob = blob;
- call_rcu(&old->h, __nf_tables_commit_chain_free_rules_old);
+ call_rcu(&last->h, __nf_tables_commit_chain_free_rules);
}
static void nf_tables_commit_chain(struct net *net, struct nft_chain *chain)
--
2.39.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH nf-next 2/3] netfilter: nf_tables: don't store address of last rule on jump
2023-04-11 14:29 [PATCH nf-next 0/3] netfilter: nf_tables: shrink jump stack size Florian Westphal
2023-04-11 14:29 ` [PATCH nf-next 1/3] netfilter: nf_tables: merge nft_rules_old structure and end of ruleblob marker Florian Westphal
@ 2023-04-11 14:29 ` Florian Westphal
2023-04-11 14:29 ` [PATCH nf-next 3/3] netfilter: nf_tables: don't store chain address " Florian Westphal
2 siblings, 0 replies; 4+ messages in thread
From: Florian Westphal @ 2023-04-11 14:29 UTC (permalink / raw)
To: netfilter-devel; +Cc: Florian Westphal
Walk the rule headers until the trailer one (last_bit flag set) instead
of stopping at last_rule address.
This avoids the need to store the address when jumping to another chain.
This cuts size of jumpstack array by one third, on 64bit from
384 to 256 bytes. Still, stack usage is still quite large:
scripts/stackusage:
nf_tables_core.c:258 nft_do_chain 496 static
Next patch will also remove chain pointer.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/netfilter/nf_tables_core.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/net/netfilter/nf_tables_core.c b/net/netfilter/nf_tables_core.c
index 6ecd0ba2e546..ec3bab751092 100644
--- a/net/netfilter/nf_tables_core.c
+++ b/net/netfilter/nf_tables_core.c
@@ -205,7 +205,6 @@ static noinline void nft_update_chain_stats(const struct nft_chain *chain,
struct nft_jumpstack {
const struct nft_chain *chain;
const struct nft_rule_dp *rule;
- const struct nft_rule_dp *last_rule;
};
static void expr_call_ops_eval(const struct nft_expr *expr,
@@ -259,9 +258,9 @@ unsigned int
nft_do_chain(struct nft_pktinfo *pkt, void *priv)
{
const struct nft_chain *chain = priv, *basechain = chain;
- const struct nft_rule_dp *rule, *last_rule;
const struct net *net = nft_net(pkt);
const struct nft_expr *expr, *last;
+ const struct nft_rule_dp *rule;
struct nft_regs regs = {};
unsigned int stackptr = 0;
struct nft_jumpstack jumpstack[NFT_JUMP_STACK_SIZE];
@@ -279,10 +278,9 @@ nft_do_chain(struct nft_pktinfo *pkt, void *priv)
blob = rcu_dereference(chain->blob_gen_0);
rule = (struct nft_rule_dp *)blob->data;
- last_rule = (void *)blob->data + blob->size;
next_rule:
regs.verdict.code = NFT_CONTINUE;
- for (; rule < last_rule; rule = nft_rule_next(rule)) {
+ for (; !rule->is_last ; rule = nft_rule_next(rule)) {
nft_rule_dp_for_each_expr(expr, last, rule) {
if (expr->ops == &nft_cmp_fast_ops)
nft_cmp_fast_eval(expr, ®s);
@@ -327,7 +325,6 @@ nft_do_chain(struct nft_pktinfo *pkt, void *priv)
return NF_DROP;
jumpstack[stackptr].chain = chain;
jumpstack[stackptr].rule = nft_rule_next(rule);
- jumpstack[stackptr].last_rule = last_rule;
stackptr++;
fallthrough;
case NFT_GOTO:
@@ -344,7 +341,6 @@ nft_do_chain(struct nft_pktinfo *pkt, void *priv)
stackptr--;
chain = jumpstack[stackptr].chain;
rule = jumpstack[stackptr].rule;
- last_rule = jumpstack[stackptr].last_rule;
goto next_rule;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH nf-next 3/3] netfilter: nf_tables: don't store chain address on jump
2023-04-11 14:29 [PATCH nf-next 0/3] netfilter: nf_tables: shrink jump stack size Florian Westphal
2023-04-11 14:29 ` [PATCH nf-next 1/3] netfilter: nf_tables: merge nft_rules_old structure and end of ruleblob marker Florian Westphal
2023-04-11 14:29 ` [PATCH nf-next 2/3] netfilter: nf_tables: don't store address of last rule on jump Florian Westphal
@ 2023-04-11 14:29 ` Florian Westphal
2 siblings, 0 replies; 4+ messages in thread
From: Florian Westphal @ 2023-04-11 14:29 UTC (permalink / raw)
To: netfilter-devel; +Cc: Florian Westphal
Now that the rule trailer/end marker and the rcu head reside in the
same structure, we no longer need to save/restore the chain pointer
when performing/returning from a jump.
We can simply let the trace infra walk the evaluated rule until it
hits the end marker and then fetch the chain pointer from there.
When the rule is NULL (policy tracing), then chain and basechain
pointers were already identical, so just use the basechain.
This cuts size of jumpstack in half, from 256 to 128 bytes in 64bit,
scripts/stackusage says:
nf_tables_core.c:251 nft_do_chain 328 static
Signed-off-by: Florian Westphal <fw@strlen.de>
---
include/net/netfilter/nf_tables.h | 14 ++++++++++++--
net/netfilter/nf_tables_api.c | 7 -------
net/netfilter/nf_tables_core.c | 21 ++++++---------------
net/netfilter/nf_tables_trace.c | 30 ++++++++++++++++++++++++++----
4 files changed, 44 insertions(+), 28 deletions(-)
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 9430128aae99..7b9f141120a1 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1046,6 +1046,18 @@ struct nft_rule_dp {
__attribute__((aligned(__alignof__(struct nft_expr))));
};
+struct nft_rule_dp_last {
+ struct nft_rule_dp end; /* end of nft_rule_blob marker */
+ struct rcu_head h; /* call_rcu head */
+ struct nft_rule_blob *blob; /* ptr to free via call_rcu */
+ const struct nft_chain *chain; /* for nftables tracing */
+};
+
+static inline const struct nft_rule_dp *nft_rule_next(const struct nft_rule_dp *rule)
+{
+ return (void *)rule + sizeof(*rule) + rule->dlen;
+}
+
struct nft_rule_blob {
unsigned long size;
unsigned char data[]
@@ -1392,7 +1404,6 @@ void nft_unregister_flowtable_type(struct nf_flowtable_type *type);
* @packet_dumped: packet headers sent in a previous traceinfo message
* @pkt: pktinfo currently processed
* @basechain: base chain currently processed
- * @chain: chain currently processed
* @rule: rule that was evaluated
* @verdict: verdict given by rule
*/
@@ -1404,7 +1415,6 @@ struct nft_traceinfo {
u32 skbid;
const struct nft_pktinfo *pkt;
const struct nft_base_chain *basechain;
- const struct nft_chain *chain;
const struct nft_rule_dp *rule;
const struct nft_verdict *verdict;
};
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index f89fa158ead9..fbc699bf0918 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2110,13 +2110,6 @@ static void nft_chain_release_hook(struct nft_chain_hook *hook)
module_put(hook->type->owner);
}
-struct nft_rule_dp_last {
- struct nft_rule_dp end; /* end of nft_rule_blob marker */
- struct rcu_head h;
- struct nft_rule_blob *blob;
- const struct nft_chain *chain; /* for tracing */
-};
-
static void nft_last_rule(const struct nft_chain *chain, const void *ptr)
{
struct nft_rule_dp_last *lrule;
diff --git a/net/netfilter/nf_tables_core.c b/net/netfilter/nf_tables_core.c
index ec3bab751092..89c05b64c2a2 100644
--- a/net/netfilter/nf_tables_core.c
+++ b/net/netfilter/nf_tables_core.c
@@ -42,13 +42,11 @@ static inline void nf_skip_indirect_calls_enable(void) { }
#endif
static noinline void __nft_trace_packet(struct nft_traceinfo *info,
- const struct nft_chain *chain,
enum nft_trace_types type)
{
if (!info->trace || !info->nf_trace)
return;
- info->chain = chain;
info->type = type;
nft_trace_notify(info);
@@ -56,14 +54,13 @@ static noinline void __nft_trace_packet(struct nft_traceinfo *info,
static inline void nft_trace_packet(const struct nft_pktinfo *pkt,
struct nft_traceinfo *info,
- const struct nft_chain *chain,
const struct nft_rule_dp *rule,
enum nft_trace_types type)
{
if (static_branch_unlikely(&nft_trace_enabled)) {
info->nf_trace = pkt->skb->nf_trace;
info->rule = rule;
- __nft_trace_packet(info, chain, type);
+ __nft_trace_packet(info, type);
}
}
@@ -111,7 +108,6 @@ static void nft_cmp16_fast_eval(const struct nft_expr *expr,
}
static noinline void __nft_trace_verdict(struct nft_traceinfo *info,
- const struct nft_chain *chain,
const struct nft_regs *regs)
{
enum nft_trace_types type;
@@ -133,17 +129,16 @@ static noinline void __nft_trace_verdict(struct nft_traceinfo *info,
break;
}
- __nft_trace_packet(info, chain, type);
+ __nft_trace_packet(info, type);
}
static inline void nft_trace_verdict(struct nft_traceinfo *info,
- const struct nft_chain *chain,
const struct nft_rule_dp *rule,
const struct nft_regs *regs)
{
if (static_branch_unlikely(&nft_trace_enabled)) {
info->rule = rule;
- __nft_trace_verdict(info, chain, regs);
+ __nft_trace_verdict(info, regs);
}
}
@@ -203,7 +198,6 @@ static noinline void nft_update_chain_stats(const struct nft_chain *chain,
}
struct nft_jumpstack {
- const struct nft_chain *chain;
const struct nft_rule_dp *rule;
};
@@ -247,7 +241,6 @@ static void expr_call_ops_eval(const struct nft_expr *expr,
#define nft_rule_expr_first(rule) (struct nft_expr *)&rule->data[0]
#define nft_rule_expr_next(expr) ((void *)expr) + expr->ops->size
#define nft_rule_expr_last(rule) (struct nft_expr *)&rule->data[rule->dlen]
-#define nft_rule_next(rule) (void *)rule + sizeof(*rule) + rule->dlen
#define nft_rule_dp_for_each_expr(expr, last, rule) \
for ((expr) = nft_rule_expr_first(rule), (last) = nft_rule_expr_last(rule); \
@@ -302,14 +295,14 @@ nft_do_chain(struct nft_pktinfo *pkt, void *priv)
nft_trace_copy_nftrace(pkt, &info);
continue;
case NFT_CONTINUE:
- nft_trace_packet(pkt, &info, chain, rule,
+ nft_trace_packet(pkt, &info, rule,
NFT_TRACETYPE_RULE);
continue;
}
break;
}
- nft_trace_verdict(&info, chain, rule, ®s);
+ nft_trace_verdict(&info, rule, ®s);
switch (regs.verdict.code & NF_VERDICT_MASK) {
case NF_ACCEPT:
@@ -323,7 +316,6 @@ nft_do_chain(struct nft_pktinfo *pkt, void *priv)
case NFT_JUMP:
if (WARN_ON_ONCE(stackptr >= NFT_JUMP_STACK_SIZE))
return NF_DROP;
- jumpstack[stackptr].chain = chain;
jumpstack[stackptr].rule = nft_rule_next(rule);
stackptr++;
fallthrough;
@@ -339,12 +331,11 @@ nft_do_chain(struct nft_pktinfo *pkt, void *priv)
if (stackptr > 0) {
stackptr--;
- chain = jumpstack[stackptr].chain;
rule = jumpstack[stackptr].rule;
goto next_rule;
}
- nft_trace_packet(pkt, &info, basechain, NULL, NFT_TRACETYPE_POLICY);
+ nft_trace_packet(pkt, &info, NULL, NFT_TRACETYPE_POLICY);
if (static_branch_unlikely(&nft_counters_enabled))
nft_update_chain_stats(basechain, pkt);
diff --git a/net/netfilter/nf_tables_trace.c b/net/netfilter/nf_tables_trace.c
index 1163ba9c1401..3d9b83d84a84 100644
--- a/net/netfilter/nf_tables_trace.c
+++ b/net/netfilter/nf_tables_trace.c
@@ -164,9 +164,29 @@ static bool nft_trace_have_verdict_chain(struct nft_traceinfo *info)
return true;
}
+static const struct nft_chain *nft_trace_get_chain(const struct nft_traceinfo *info)
+{
+ const struct nft_rule_dp *rule = info->rule;
+ const struct nft_rule_dp_last *last;
+
+ if (!rule)
+ return &info->basechain->chain;
+
+ while (!rule->is_last)
+ rule = nft_rule_next(rule);
+
+ last = (const struct nft_rule_dp_last *)rule;
+
+ if (WARN_ON_ONCE(!last->chain))
+ return &info->basechain->chain;
+
+ return last->chain;
+}
+
void nft_trace_notify(struct nft_traceinfo *info)
{
const struct nft_pktinfo *pkt = info->pkt;
+ const struct nft_chain *chain;
struct nlmsghdr *nlh;
struct sk_buff *skb;
unsigned int size;
@@ -176,9 +196,11 @@ void nft_trace_notify(struct nft_traceinfo *info)
if (!nfnetlink_has_listeners(nft_net(pkt), NFNLGRP_NFTRACE))
return;
+ chain = nft_trace_get_chain(info);
+
size = nlmsg_total_size(sizeof(struct nfgenmsg)) +
- nla_total_size(strlen(info->chain->table->name)) +
- nla_total_size(strlen(info->chain->name)) +
+ nla_total_size(strlen(chain->table->name)) +
+ nla_total_size(strlen(chain->name)) +
nla_total_size_64bit(sizeof(__be64)) + /* rule handle */
nla_total_size(sizeof(__be32)) + /* trace type */
nla_total_size(0) + /* VERDICT, nested */
@@ -217,10 +239,10 @@ void nft_trace_notify(struct nft_traceinfo *info)
if (nla_put_u32(skb, NFTA_TRACE_ID, info->skbid))
goto nla_put_failure;
- if (nla_put_string(skb, NFTA_TRACE_CHAIN, info->chain->name))
+ if (nla_put_string(skb, NFTA_TRACE_CHAIN, chain->name))
goto nla_put_failure;
- if (nla_put_string(skb, NFTA_TRACE_TABLE, info->chain->table->name))
+ if (nla_put_string(skb, NFTA_TRACE_TABLE, chain->table->name))
goto nla_put_failure;
if (nf_trace_fill_rule_info(skb, info))
--
2.39.2
^ permalink raw reply related [flat|nested] 4+ messages in thread