1. Patchew
  2. linux
  3. View series

[PATCH] bpf: Fix excessive memory allocation in stack_map_alloc()

Yuntao Wang posted 1 patch 3 years, 10 months ago 
kernel/bpf/stackmap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH] bpf: Fix excessive memory allocation in stack_map_alloc()
Posted by Yuntao Wang 3 years, 10 months ago 
The 'n_buckets * (value_size + sizeof(struct stack_map_bucket))' part of
the allocated memory for 'smap' is never used, get rid of it.

Fixes: b936ca643ade ("bpf: rework memlock-based memory accounting for maps")
Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
Link: https://lore.kernel.org/bpf/20220407130423.798386-1-ytcoode@gmail.com
---
This is the modified version for 5.10, the original patch is:

[ Upstream commit b45043192b3e481304062938a6561da2ceea46a6 ]

It would be better if the new patch can be reviewed by someone else.

 kernel/bpf/stackmap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index 4575d2d60cb1..54fdcb78ad19 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -121,8 +121,8 @@ static struct bpf_map *stack_map_alloc(union bpf_attr *attr)
 		return ERR_PTR(-E2BIG);
 
 	cost = n_buckets * sizeof(struct stack_map_bucket *) + sizeof(*smap);
-	cost += n_buckets * (value_size + sizeof(struct stack_map_bucket));
-	err = bpf_map_charge_init(&mem, cost);
+	err = bpf_map_charge_init(&mem, cost + n_buckets *
+				  (value_size + sizeof(struct stack_map_bucket)));
 	if (err)
 		return ERR_PTR(err);
 
-- 
2.36.0
Re: [PATCH] bpf: Fix excessive memory allocation in stack_map_alloc()
Posted by Greg KH 3 years, 10 months ago 
On Wed, Jun 08, 2022 at 10:25:38PM +0800, Yuntao Wang wrote:
> The 'n_buckets * (value_size + sizeof(struct stack_map_bucket))' part of
> the allocated memory for 'smap' is never used, get rid of it.
> 
> Fixes: b936ca643ade ("bpf: rework memlock-based memory accounting for maps")
> Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
> Link: https://lore.kernel.org/bpf/20220407130423.798386-1-ytcoode@gmail.com
> ---
> This is the modified version for 5.10, the original patch is:
> 
> [ Upstream commit b45043192b3e481304062938a6561da2ceea46a6 ]
> 
> It would be better if the new patch can be reviewed by someone else.

What is wrong with the version that we have queued up in the 5.10-stable
review queue right now?



> 
>  kernel/bpf/stackmap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index 4575d2d60cb1..54fdcb78ad19 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c
> @@ -121,8 +121,8 @@ static struct bpf_map *stack_map_alloc(union bpf_attr *attr)
>  		return ERR_PTR(-E2BIG);
>  
>  	cost = n_buckets * sizeof(struct stack_map_bucket *) + sizeof(*smap);
> -	cost += n_buckets * (value_size + sizeof(struct stack_map_bucket));
> -	err = bpf_map_charge_init(&mem, cost);
> +	err = bpf_map_charge_init(&mem, cost + n_buckets *
> +				  (value_size + sizeof(struct stack_map_bucket)));

This differs from what we have queued up for 5.4.y and 5.10.y, why?
If you are going to modify the upstream version, you need to document in
great detail what you have changed and why you have changed it.

thanks,

greg k-h
Re: [PATCH] bpf: Fix excessive memory allocation in stack_map_alloc()
Posted by Yuntao Wang 3 years, 10 months ago 
On Wed, 8 Jun 2022 17:20:58 +0200, Greg KH wrote:
> On Wed, Jun 08, 2022 at 10:25:38PM +0800, Yuntao Wang wrote:
> > The 'n_buckets * (value_size + sizeof(struct stack_map_bucket))' part of
> > the allocated memory for 'smap' is never used, get rid of it.
> > 
> > Fixes: b936ca643ade ("bpf: rework memlock-based memory accounting for maps")
> > Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
> > Link: https://lore.kernel.org/bpf/20220407130423.798386-1-ytcoode@gmail.com
> > ---
> > This is the modified version for 5.10, the original patch is:
> > 
> > [ Upstream commit b45043192b3e481304062938a6561da2ceea46a6 ]
> > 
> > It would be better if the new patch can be reviewed by someone else.
> 
> What is wrong with the version that we have queued up in the 5.10-stable
> review queue right now?

Since the 5.10 branch doesn't have commit 370868107bf6, the upstream version
is not correct for it, I modified the original patch and wanted to backport
it to the 5.10 branch.

> > 
> >  kernel/bpf/stackmap.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> > index 4575d2d60cb1..54fdcb78ad19 100644
> > --- a/kernel/bpf/stackmap.c
> > +++ b/kernel/bpf/stackmap.c
> > @@ -121,8 +121,8 @@ static struct bpf_map *stack_map_alloc(union bpf_attr *attr)
> >  		return ERR_PTR(-E2BIG);
> >  
> >  	cost = n_buckets * sizeof(struct stack_map_bucket *) + sizeof(*smap);
> > -	cost += n_buckets * (value_size + sizeof(struct stack_map_bucket));
> > -	err = bpf_map_charge_init(&mem, cost);
> > +	err = bpf_map_charge_init(&mem, cost + n_buckets *
> > +				  (value_size + sizeof(struct stack_map_bucket)));
> 
> This differs from what we have queued up for 5.4.y and 5.10.y, why?
> If you are going to modify the upstream version, you need to document in
> great detail what you have changed and why you have changed it.
> 
> thanks,
> 
> greg k-h
Re: [PATCH] bpf: Fix excessive memory allocation in stack_map_alloc()
Posted by Greg KH 3 years, 10 months ago 
On Thu, Jun 09, 2022 at 12:07:28AM +0800, Yuntao Wang wrote:
> On Wed, 8 Jun 2022 17:20:58 +0200, Greg KH wrote:
> > On Wed, Jun 08, 2022 at 10:25:38PM +0800, Yuntao Wang wrote:
> > > The 'n_buckets * (value_size + sizeof(struct stack_map_bucket))' part of
> > > the allocated memory for 'smap' is never used, get rid of it.
> > > 
> > > Fixes: b936ca643ade ("bpf: rework memlock-based memory accounting for maps")
> > > Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
> > > Link: https://lore.kernel.org/bpf/20220407130423.798386-1-ytcoode@gmail.com
> > > ---
> > > This is the modified version for 5.10, the original patch is:
> > > 
> > > [ Upstream commit b45043192b3e481304062938a6561da2ceea46a6 ]
> > > 
> > > It would be better if the new patch can be reviewed by someone else.
> > 
> > What is wrong with the version that we have queued up in the 5.10-stable
> > review queue right now?
> 
> Since the 5.10 branch doesn't have commit 370868107bf6, the upstream version
> is not correct for it, I modified the original patch and wanted to backport
> it to the 5.10 branch.

This does not apply to the 5.10 branch now, can you provide a working
version?

thanks,

greg k-h
[PATCH] bpf: Fix incorrect memory charge cost calculation in stack_map_alloc()
Posted by Yuntao Wang 3 years, 10 months ago 
commit b45043192b3e481304062938a6561da2ceea46a6 upstream.

This is a backport of the original upstream patch for 5.4/5.10.

The original upstream patch has been applied to 5.4/5.10 branches, which
simply removed the line:

  cost += n_buckets * (value_size + sizeof(struct stack_map_bucket));

This is correct for upstream branch but incorrect for 5.4/5.10 branches,
as the 5.4/5.10 branches do not have the commit 370868107bf6 ("bpf:
Eliminate rlimit-based memory accounting for stackmap maps"), so the
bpf_map_charge_init() function has not been removed.

Currently the bpf_map_charge_init() function in 5.4/5.10 branches takes a
wrong memory charge cost, the

  attr->max_entries * (sizeof(struct stack_map_bucket) + (u64)value_size))

part is missing, let's fix it.

Cc: <stable@vger.kernel.org> # 5.4.y
Cc: <stable@vger.kernel.org> # 5.10.y
Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
---
Note that the original upstream patch is currently applied to
linux-stable-rc/linux-5.4.y branch, not linux/linux-5.4.y, this patch
depends on that patch.

 kernel/bpf/stackmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index c19e669afba0..0c5bf98d5576 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -121,7 +121,8 @@ static struct bpf_map *stack_map_alloc(union bpf_attr *attr)
 		return ERR_PTR(-E2BIG);
 
 	cost = n_buckets * sizeof(struct stack_map_bucket *) + sizeof(*smap);
-	err = bpf_map_charge_init(&mem, cost);
+	err = bpf_map_charge_init(&mem, cost + attr->max_entries *
+			   (sizeof(struct stack_map_bucket) + (u64)value_size));
 	if (err)
 		return ERR_PTR(err);
 
-- 
2.36.0
Re: [PATCH] bpf: Fix incorrect memory charge cost calculation in stack_map_alloc()
Posted by Greg KH 3 years, 10 months ago 
On Tue, Jun 14, 2022 at 10:26:22PM +0800, Yuntao Wang wrote:
> commit b45043192b3e481304062938a6561da2ceea46a6 upstream.
> 
> This is a backport of the original upstream patch for 5.4/5.10.
> 
> The original upstream patch has been applied to 5.4/5.10 branches, which
> simply removed the line:
> 
>   cost += n_buckets * (value_size + sizeof(struct stack_map_bucket));
> 
> This is correct for upstream branch but incorrect for 5.4/5.10 branches,
> as the 5.4/5.10 branches do not have the commit 370868107bf6 ("bpf:
> Eliminate rlimit-based memory accounting for stackmap maps"), so the
> bpf_map_charge_init() function has not been removed.
> 
> Currently the bpf_map_charge_init() function in 5.4/5.10 branches takes a
> wrong memory charge cost, the
> 
>   attr->max_entries * (sizeof(struct stack_map_bucket) + (u64)value_size))
> 
> part is missing, let's fix it.
> 
> Cc: <stable@vger.kernel.org> # 5.4.y
> Cc: <stable@vger.kernel.org> # 5.10.y
> Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
> ---
> Note that the original upstream patch is currently applied to
> linux-stable-rc/linux-5.4.y branch, not linux/linux-5.4.y, this patch
> depends on that patch.

Now queued up, thanks.

greg k-h

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.