| Message ID | 20190906151242.1115282-1-arnd@arndb.de |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [net-next] netfilter: nf_tables: avoid excessive stack usage | expand |
Hi Arnd, On Fri, Sep 06, 2019 at 05:12:30PM +0200, Arnd Bergmann wrote: > The nft_offload_ctx structure is much too large to put on the > stack: > > net/netfilter/nf_tables_offload.c:31:23: error: stack frame size of 1200 bytes in function 'nft_flow_rule_create' [-Werror,-Wframe-larger-than=] > > Use dynamic allocation here, as we do elsewhere in the same > function. > > Fixes: c9626a2cbdb2 ("netfilter: nf_tables: add hardware offload support") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > Since we only really care about two members of the structure, an > alternative would be a larger rewrite, but that is probably too > late for v5.4. Thanks for this patch. I'm attaching a patch to reduce this structure size a bit. Do you think this alternative patch is ok until this alternative rewrite happens? Anyway I agree we should to get this structure away from the stack, even after this is still large, so your patch (or a variant of it) will be useful sooner than later I think.
On Sat, Sep 7, 2019 at 8:07 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > Hi Arnd, > > On Fri, Sep 06, 2019 at 05:12:30PM +0200, Arnd Bergmann wrote: > > The nft_offload_ctx structure is much too large to put on the > > stack: > > > > net/netfilter/nf_tables_offload.c:31:23: error: stack frame size of 1200 bytes in function 'nft_flow_rule_create' [-Werror,-Wframe-larger-than=] > > > > Use dynamic allocation here, as we do elsewhere in the same > > function. > > > > Fixes: c9626a2cbdb2 ("netfilter: nf_tables: add hardware offload support") > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > > Since we only really care about two members of the structure, an > > alternative would be a larger rewrite, but that is probably too > > late for v5.4. > > Thanks for this patch. > > I'm attaching a patch to reduce this structure size a bit. Do you > think this alternative patch is ok until this alternative rewrite > happens? I haven't tried it yet, but it looks like that would save 8 of the 48 bytes in each for each of the 24 registers (12 bytes on m68k or i386, which only use 4 byte alignment for nft_data), so this wouldn't make too much difference. > Anyway I agree we should to get this structure away from the > stack, even after this is still large, so your patch (or a variant of > it) will be useful sooner than later I think. What I was thinking for a possible smaller fix would be to not pass the ctx into the expr->ops->offload callback but only pass the 'dep' member. Since I've never seen this code before, I have no idea if that would be an improvement in the end. Arnd
On Sat, Sep 07, 2019 at 08:41:22PM +0200, Arnd Bergmann wrote: > On Sat, Sep 7, 2019 at 8:07 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > > Hi Arnd, > > > > On Fri, Sep 06, 2019 at 05:12:30PM +0200, Arnd Bergmann wrote: > > > The nft_offload_ctx structure is much too large to put on the > > > stack: > > > > > > net/netfilter/nf_tables_offload.c:31:23: error: stack frame size of 1200 bytes in function 'nft_flow_rule_create' [-Werror,-Wframe-larger-than=] > > > > > > Use dynamic allocation here, as we do elsewhere in the same > > > function. > > > > > > Fixes: c9626a2cbdb2 ("netfilter: nf_tables: add hardware offload support") > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > --- > > > Since we only really care about two members of the structure, an > > > alternative would be a larger rewrite, but that is probably too > > > late for v5.4. > > > > Thanks for this patch. > > > > I'm attaching a patch to reduce this structure size a bit. Do you > > think this alternative patch is ok until this alternative rewrite > > happens? > > I haven't tried it yet, but it looks like that would save 8 of the > 48 bytes in each for each of the 24 registers (12 bytes on m68k > or i386, which only use 4 byte alignment for nft_data), so > this wouldn't make too much difference. I'll take your patch as is. > > Anyway I agree we should to get this structure away from the > > stack, even after this is still large, so your patch (or a variant of > > it) will be useful sooner than later I think. > > What I was thinking for a possible smaller fix would be to not > pass the ctx into the expr->ops->offload callback but > only pass the 'dep' member. Since I've never seen this code > before, I have no idea if that would be an improvement > in the end. We might need this more fields of this context structure, this code is very new, still under development, let's revisit this later. Thanks.
diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c index 3c2725ade61b..c94331aae552 100644 --- a/net/netfilter/nf_tables_offload.c +++ b/net/netfilter/nf_tables_offload.c @@ -30,15 +30,13 @@ static struct nft_flow_rule *nft_flow_rule_alloc(int num_actions) struct nft_flow_rule *nft_flow_rule_create(const struct nft_rule *rule) { - struct nft_offload_ctx ctx = { - .dep = { - .type = NFT_OFFLOAD_DEP_UNSPEC, - }, - }; + struct nft_offload_ctx *ctx; + struct nft_flow_rule *flow; int num_actions = 0, err; struct nft_expr *expr; + expr = nft_expr_first(rule); while (expr->ops && expr != nft_expr_last(rule)) { if (expr->ops->offload_flags & NFT_OFFLOAD_F_ACTION) @@ -52,21 +50,31 @@ struct nft_flow_rule *nft_flow_rule_create(const struct nft_rule *rule) return ERR_PTR(-ENOMEM); expr = nft_expr_first(rule); + + ctx = kzalloc(sizeof(struct nft_offload_ctx), GFP_KERNEL); + if (!ctx) { + err = -ENOMEM; + goto err_out; + } + ctx->dep.type = NFT_OFFLOAD_DEP_UNSPEC; + while (expr->ops && expr != nft_expr_last(rule)) { if (!expr->ops->offload) { err = -EOPNOTSUPP; goto err_out; } - err = expr->ops->offload(&ctx, flow, expr); + err = expr->ops->offload(ctx, flow, expr); if (err < 0) goto err_out; expr = nft_expr_next(expr); } - flow->proto = ctx.dep.l3num; + flow->proto = ctx->dep.l3num; + kfree(ctx); return flow; err_out: + kfree(ctx); nft_flow_rule_destroy(flow); return ERR_PTR(err);
The nft_offload_ctx structure is much too large to put on the stack: net/netfilter/nf_tables_offload.c:31:23: error: stack frame size of 1200 bytes in function 'nft_flow_rule_create' [-Werror,-Wframe-larger-than=] Use dynamic allocation here, as we do elsewhere in the same function. Fixes: c9626a2cbdb2 ("netfilter: nf_tables: add hardware offload support") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- Since we only really care about two members of the structure, an alternative would be a larger rewrite, but that is probably too late for v5.4. --- net/netfilter/nf_tables_offload.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)