From: Yuntao Wang <[email protected]>
[ Upstream commit b45043192b3e481304062938a6561da2ceea46a6 ]
The 'n_buckets * (value_size + sizeof(struct stack_map_bucket))' part of the
allocated memory for 'smap' is never used after the memlock accounting was
removed, thus get rid of it.
[ Note, Daniel:
Commit b936ca643ade ("bpf: rework memlock-based memory accounting for maps")
moved `cost += n_buckets * (value_size + sizeof(struct stack_map_bucket))`
up and therefore before the bpf_map_area_alloc() allocation, sigh. In a later
step commit c85d69135a91 ("bpf: move memory size checks to bpf_map_charge_init()"),
and the overflow checks of `cost >= U32_MAX - PAGE_SIZE` moved into
bpf_map_charge_init(). And then 370868107bf6 ("bpf: Eliminate rlimit-based
memory accounting for stackmap maps") finally removed the bpf_map_charge_init().
Anyway, the original code did the allocation same way as /after/ this fix. ]
Fixes: b936ca643ade ("bpf: rework memlock-based memory accounting for maps")
Signed-off-by: Yuntao Wang <[email protected]>
Signed-off-by: Daniel Borkmann <[email protected]>
Link: https://lore.kernel.org/bpf/[email protected]
Signed-off-by: Sasha Levin <[email protected]>
---
kernel/bpf/stackmap.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index 4575d2d60cb1..c19e669afba0 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -121,7 +121,6 @@ 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);
if (err)
return ERR_PTR(err);
--
2.35.1
Hi!
> The 'n_buckets * (value_size + sizeof(struct stack_map_bucket))' part of the
> allocated memory for 'smap' is never used after the memlock accounting was
> removed, thus get rid of it.
>
> [ Note, Daniel:
>
> Commit b936ca643ade ("bpf: rework memlock-based memory accounting for maps")
> moved `cost += n_buckets * (value_size + sizeof(struct stack_map_bucket))`
> up and therefore before the bpf_map_area_alloc() allocation, sigh. In a later
> step commit c85d69135a91 ("bpf: move memory size checks to bpf_map_charge_init()"),
> and the overflow checks of `cost >= U32_MAX - PAGE_SIZE` moved into
> bpf_map_charge_init(). And then 370868107bf6 ("bpf: Eliminate rlimit-based
> memory accounting for stackmap maps") finally removed the bpf_map_charge_init().
> Anyway, the original code did the allocation same way as /after/ this fix. ]
We don't have 370868107bf6 in 5.10. Can someone verify this is still
right think to do for 5.10?
Best regards,
Pavel
> +++ b/kernel/bpf/stackmap.c
> @@ -121,7 +121,6 @@ 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);
> if (err)
> return ERR_PTR(err);
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
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 <[email protected]>
Link: https://lore.kernel.org/bpf/[email protected]
---
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
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 <[email protected]>
> Link: https://lore.kernel.org/bpf/[email protected]
> ---
> 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
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 <[email protected]>
> > Link: https://lore.kernel.org/bpf/[email protected]
> > ---
> > 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
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 <[email protected]>
> > > Link: https://lore.kernel.org/bpf/[email protected]
> > > ---
> > > 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
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: <[email protected]> # 5.4.y
Cc: <[email protected]> # 5.10.y
Signed-off-by: Yuntao Wang <[email protected]>
---
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
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: <[email protected]> # 5.4.y
> Cc: <[email protected]> # 5.10.y
> Signed-off-by: Yuntao Wang <[email protected]>
> ---
> 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