* [PATCH net-next] netfilter: nf_tables: avoid excessive stack usage
@ 2019-09-06 15:12 Arnd Bergmann
2019-09-07 18:07 ` Pablo Neira Ayuso
0 siblings, 1 reply; 4+ messages in thread
From: Arnd Bergmann @ 2019-09-06 15:12 UTC (permalink / raw)
To: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal, David S. Miller
Cc: Arnd Bergmann, Jakub Kicinski, wenxu, netfilter-devel, coreteam,
netdev, linux-kernel
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(-)
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);
--
2.20.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net-next] netfilter: nf_tables: avoid excessive stack usage
2019-09-06 15:12 [PATCH net-next] netfilter: nf_tables: avoid excessive stack usage Arnd Bergmann
@ 2019-09-07 18:07 ` Pablo Neira Ayuso
2019-09-07 18:41 ` Arnd Bergmann
0 siblings, 1 reply; 4+ messages in thread
From: Pablo Neira Ayuso @ 2019-09-07 18:07 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jozsef Kadlecsik, Florian Westphal, David S. Miller,
Jakub Kicinski, wenxu, netfilter-devel, coreteam, netdev,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 998 bytes --]
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.
[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 485 bytes --]
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;
};
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net-next] netfilter: nf_tables: avoid excessive stack usage
2019-09-07 18:07 ` Pablo Neira Ayuso
@ 2019-09-07 18:41 ` Arnd Bergmann
2019-09-07 18:52 ` Pablo Neira Ayuso
0 siblings, 1 reply; 4+ messages in thread
From: Arnd Bergmann @ 2019-09-07 18:41 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Jozsef Kadlecsik, Florian Westphal, David S. Miller,
Jakub Kicinski, wenxu, netfilter-devel, coreteam, Networking,
linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next] netfilter: nf_tables: avoid excessive stack usage
2019-09-07 18:41 ` Arnd Bergmann
@ 2019-09-07 18:52 ` Pablo Neira Ayuso
0 siblings, 0 replies; 4+ messages in thread
From: Pablo Neira Ayuso @ 2019-09-07 18:52 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jozsef Kadlecsik, Florian Westphal, David S. Miller,
Jakub Kicinski, wenxu, netfilter-devel, coreteam, Networking,
linux-kernel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-09-07 18:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-06 15:12 [PATCH net-next] netfilter: nf_tables: avoid excessive stack usage Arnd Bergmann
2019-09-07 18:07 ` Pablo Neira Ayuso
2019-09-07 18:41 ` Arnd Bergmann
2019-09-07 18:52 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).