diff mbox series

[net-next] netfilter: nf_tables: avoid excessive stack usage

Message ID 20190906151242.1115282-1-arnd@arndb.de
State New
Headers show
Series [net-next] netfilter: nf_tables: avoid excessive stack usage | expand

Commit Message

Arnd Bergmann Sept. 6, 2019, 3:12 p.m. UTC 
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(-)

-- 
2.20.0

Comments

Pablo Neira Ayuso Sept. 7, 2019, 6:07 p.m. UTC | #1 
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.
diff --git a/include/net/netfilter/nf_tables_offload.h b/include/net/netfilter/nf_tables_offload.h
index db104665a9e4..cc44d29e9fd7 100644
--- a/include/net/netfilter/nf_tables_offload.h
+++ b/include/net/netfilter/nf_tables_offload.h
@@ -5,10 +5,10 @@
 #include <net/netfilter/nf_tables.h>
 
 struct nft_offload_reg {
-	u32		key;
-	u32		len;
-	u32		base_offset;
-	u32		offset;
+	u8		key;
+	u8		len;
+	u8		base_offset;
+	u8		offset;
 	struct nft_data data;
 	struct nft_data	mask;
 };
Arnd Bergmann Sept. 7, 2019, 6:41 p.m. UTC | #2 
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
Pablo Neira Ayuso Sept. 7, 2019, 6:52 p.m. UTC | #3 
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 mbox series

Patch

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