* [PATCH bpf-next v1] bpf: bpf_local_storage_update fixes
@ 2022-03-22 21:15 Joanne Koong
2022-03-22 21:56 ` Martin KaFai Lau
0 siblings, 1 reply; 6+ messages in thread
From: Joanne Koong @ 2022-03-22 21:15 UTC (permalink / raw)
To: bpf; +Cc: ast, kafai, kpsingh, daniel, andrii, Joanne Koong, kernel test robot
From: Joanne Koong <joannelkoong@gmail.com>
This fixes two things in bpf_local_storage_update:
1) A memory leak where if bpf_selem_alloc is called right before we
acquire the spinlock and we hit the case where we can copy the new
value into old_sdata directly, we need to free the selem allocation
and uncharge the memory before we return. This was reported by the
kernel test robot.
2) A charge leak where if bpf_selem_alloc is called right before we
acquire the spinlock and we hit the case where old_sdata exists and we
need to unlink the old selem, we need to make sure the old selem gets
uncharged.
Fixes: b00fa38a9c1c ("bpf: Enable non-atomic allocations in local storage")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
kernel/bpf/bpf_local_storage.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
index 01aa2b51ec4d..2d33af0368ba 100644
--- a/kernel/bpf/bpf_local_storage.c
+++ b/kernel/bpf/bpf_local_storage.c
@@ -435,8 +435,12 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
if (old_sdata && (map_flags & BPF_F_LOCK)) {
copy_map_value_locked(&smap->map, old_sdata->data, value,
false);
- selem = SELEM(old_sdata);
- goto unlock;
+ raw_spin_unlock_irqrestore(&local_storage->lock, flags);
+ if (selem) {
+ mem_uncharge(smap, owner, smap->elem_size);
+ kfree(selem);
+ }
+ return old_sdata;
}
if (gfp_flags != GFP_KERNEL) {
@@ -466,10 +470,9 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
if (old_sdata) {
bpf_selem_unlink_map(SELEM(old_sdata));
bpf_selem_unlink_storage_nolock(local_storage, SELEM(old_sdata),
- false);
+ gfp_flags == GFP_KERNEL);
}
-unlock:
raw_spin_unlock_irqrestore(&local_storage->lock, flags);
return SDATA(selem);
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v1] bpf: bpf_local_storage_update fixes
2022-03-22 21:15 [PATCH bpf-next v1] bpf: bpf_local_storage_update fixes Joanne Koong
@ 2022-03-22 21:56 ` Martin KaFai Lau
2022-03-22 22:38 ` Joanne Koong
0 siblings, 1 reply; 6+ messages in thread
From: Martin KaFai Lau @ 2022-03-22 21:56 UTC (permalink / raw)
To: Joanne Koong
Cc: bpf, ast, kpsingh, daniel, andrii, Joanne Koong, kernel test robot
On Tue, Mar 22, 2022 at 02:15:13PM -0700, Joanne Koong wrote:
> From: Joanne Koong <joannelkoong@gmail.com>
>
> This fixes two things in bpf_local_storage_update:
>
> 1) A memory leak where if bpf_selem_alloc is called right before we
> acquire the spinlock and we hit the case where we can copy the new
> value into old_sdata directly, we need to free the selem allocation
> and uncharge the memory before we return. This was reported by the
> kernel test robot.
>
> 2) A charge leak where if bpf_selem_alloc is called right before we
> acquire the spinlock and we hit the case where old_sdata exists and we
> need to unlink the old selem, we need to make sure the old selem gets
> uncharged.
>
> Fixes: b00fa38a9c1c ("bpf: Enable non-atomic allocations in local storage")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
> kernel/bpf/bpf_local_storage.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
> index 01aa2b51ec4d..2d33af0368ba 100644
> --- a/kernel/bpf/bpf_local_storage.c
> +++ b/kernel/bpf/bpf_local_storage.c
> @@ -435,8 +435,12 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
> if (old_sdata && (map_flags & BPF_F_LOCK)) {
> copy_map_value_locked(&smap->map, old_sdata->data, value,
> false);
> - selem = SELEM(old_sdata);
> - goto unlock;
> + raw_spin_unlock_irqrestore(&local_storage->lock, flags);
> + if (selem) {
There is an earlier test ensures GFP_KERNEL can only
be used with BPF_NOEXIST.
The check_flags() before this should have error out.
Can you share a pointer to the report from kernel test robot?
> + mem_uncharge(smap, owner, smap->elem_size);
> + kfree(selem);
> + }
> + return old_sdata;
> }
>
> if (gfp_flags != GFP_KERNEL) {
> @@ -466,10 +470,9 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
> if (old_sdata) {
> bpf_selem_unlink_map(SELEM(old_sdata));
> bpf_selem_unlink_storage_nolock(local_storage, SELEM(old_sdata),
> - false);
> + gfp_flags == GFP_KERNEL);
> }
>
> -unlock:
> raw_spin_unlock_irqrestore(&local_storage->lock, flags);
> return SDATA(selem);
>
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v1] bpf: bpf_local_storage_update fixes
2022-03-22 21:56 ` Martin KaFai Lau
@ 2022-03-22 22:38 ` Joanne Koong
2022-03-22 22:54 ` KP Singh
2022-03-22 23:07 ` Martin KaFai Lau
0 siblings, 2 replies; 6+ messages in thread
From: Joanne Koong @ 2022-03-22 22:38 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Joanne Koong, bpf, Alexei Starovoitov, KP Singh, Daniel Borkmann,
Andrii Nakryiko, kernel test robot
On Tue, Mar 22, 2022 at 2:57 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Tue, Mar 22, 2022 at 02:15:13PM -0700, Joanne Koong wrote:
> > From: Joanne Koong <joannelkoong@gmail.com>
> >
> > This fixes two things in bpf_local_storage_update:
> >
> > 1) A memory leak where if bpf_selem_alloc is called right before we
> > acquire the spinlock and we hit the case where we can copy the new
> > value into old_sdata directly, we need to free the selem allocation
> > and uncharge the memory before we return. This was reported by the
> > kernel test robot.
> >
> > 2) A charge leak where if bpf_selem_alloc is called right before we
> > acquire the spinlock and we hit the case where old_sdata exists and we
> > need to unlink the old selem, we need to make sure the old selem gets
> > uncharged.
> >
> > Fixes: b00fa38a9c1c ("bpf: Enable non-atomic allocations in local storage")
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > ---
> > kernel/bpf/bpf_local_storage.c | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
> > index 01aa2b51ec4d..2d33af0368ba 100644
> > --- a/kernel/bpf/bpf_local_storage.c
> > +++ b/kernel/bpf/bpf_local_storage.c
> > @@ -435,8 +435,12 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
> > if (old_sdata && (map_flags & BPF_F_LOCK)) {
> > copy_map_value_locked(&smap->map, old_sdata->data, value,
> > false);
> > - selem = SELEM(old_sdata);
> > - goto unlock;
> > + raw_spin_unlock_irqrestore(&local_storage->lock, flags);
> > + if (selem) {
> There is an earlier test ensures GFP_KERNEL can only
> be used with BPF_NOEXIST.
>
I agree, we currently will never run into this case (since the
GFP_KERNEL case will error out if old_sdata exists), but my thinking
was that maybe in the future it may not always hold that GFP_KERNEL
will always be coupled with BPF_NOEXIST, so this change would
defensively protect against that.
> The check_flags() before this should have error out.
>
> Can you share a pointer to the report from kernel test robot?
>
I'm unable to find a link to the report, so I will copy/paste the contents:
From: kernel test robot <lkp@intel.com>
Date: Tue, Mar 22, 2022 at 11:36 AM
Subject: [bpf-next:master] BUILD SUCCESS
e52b8f5bd3d2f7b2f4b98067db33bc2fdc125643
To: BPF build status <bpf@iogearbox.net>
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git
master
branch HEAD: e52b8f5bd3d2f7b2f4b98067db33bc2fdc125643 selftests/bpf:
Fix kprobe_multi test.
Unverified Warning (likely false positive, please contact us if interested):
kernel/bpf/bpf_local_storage.c:473:2: warning: Potential leak of
memory pointed to by 'selem' [clang-analyzer-unix.Malloc]
Warning ids grouped by kconfigs:
clang_recent_errors
`-- i386-randconfig-c001
`-- kernel-bpf-bpf_local_storage.c:warning:Potential-leak-of-memory-pointed-to-by-selem-clang-analyzer-unix.Malloc
elapsed time: 723m
> > + mem_uncharge(smap, owner, smap->elem_size);
> > + kfree(selem);
> > + }
> > + return old_sdata;
> > }
> >
> > if (gfp_flags != GFP_KERNEL) {
> > @@ -466,10 +470,9 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
> > if (old_sdata) {
> > bpf_selem_unlink_map(SELEM(old_sdata));
> > bpf_selem_unlink_storage_nolock(local_storage, SELEM(old_sdata),
> > - false);
> > + gfp_flags == GFP_KERNEL);
> > }
> >
> > -unlock:
> > raw_spin_unlock_irqrestore(&local_storage->lock, flags);
> > return SDATA(selem);
> >
> > --
> > 2.30.2
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v1] bpf: bpf_local_storage_update fixes
2022-03-22 22:38 ` Joanne Koong
@ 2022-03-22 22:54 ` KP Singh
2022-03-22 23:07 ` Martin KaFai Lau
1 sibling, 0 replies; 6+ messages in thread
From: KP Singh @ 2022-03-22 22:54 UTC (permalink / raw)
To: Joanne Koong
Cc: Martin KaFai Lau, Joanne Koong, bpf, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, kernel test robot
On Tue, Mar 22, 2022 at 11:38 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Tue, Mar 22, 2022 at 2:57 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > On Tue, Mar 22, 2022 at 02:15:13PM -0700, Joanne Koong wrote:
> > > From: Joanne Koong <joannelkoong@gmail.com>
> > >
> > > This fixes two things in bpf_local_storage_update:
> > >
> > > 1) A memory leak where if bpf_selem_alloc is called right before we
> > > acquire the spinlock and we hit the case where we can copy the new
> > > value into old_sdata directly, we need to free the selem allocation
> > > and uncharge the memory before we return. This was reported by the
> > > kernel test robot.
> > >
> > > 2) A charge leak where if bpf_selem_alloc is called right before we
> > > acquire the spinlock and we hit the case where old_sdata exists and we
> > > need to unlink the old selem, we need to make sure the old selem gets
> > > uncharged.
> > >
> > > Fixes: b00fa38a9c1c ("bpf: Enable non-atomic allocations in local storage")
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > ---
> > > kernel/bpf/bpf_local_storage.c | 11 +++++++----
> > > 1 file changed, 7 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
> > > index 01aa2b51ec4d..2d33af0368ba 100644
> > > --- a/kernel/bpf/bpf_local_storage.c
> > > +++ b/kernel/bpf/bpf_local_storage.c
> > > @@ -435,8 +435,12 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
> > > if (old_sdata && (map_flags & BPF_F_LOCK)) {
> > > copy_map_value_locked(&smap->map, old_sdata->data, value,
> > > false);
> > > - selem = SELEM(old_sdata);
> > > - goto unlock;
> > > + raw_spin_unlock_irqrestore(&local_storage->lock, flags);
> > > + if (selem) {
> > There is an earlier test ensures GFP_KERNEL can only
> > be used with BPF_NOEXIST.
> >
>
> I agree, we currently will never run into this case (since the
> GFP_KERNEL case will error out if old_sdata exists), but my thinking
> was that maybe in the future it may not always hold that GFP_KERNEL
> will always be coupled with BPF_NOEXIST, so this change would
> defensively protect against that.
Didn't we discuss that we should not do this?
"The GFP_KERNEL here is only
calling from the bpf helper side and it is always done with BPF_NOEXIST
because the bpf helper has already done a lookup,
so it should always charge success first and then alloc."
Right?
I think we should add some comments about this in the code.
Also, on a side note (as it's from an older commit),
the flags logic seems to be getting more and more
complicated.
/* BPF_EXIST and BPF_NOEXIST cannot be both set */
if (unlikely((map_flags & ~BPF_F_LOCK) > BPF_EXIST) ||
/* BPF_F_LOCK can only be used in a value with spin_lock */
unlikely((map_flags & BPF_F_LOCK) &&
!map_value_has_spin_lock(&smap->map)))
return ERR_PTR(-EINVAL);
if (gfp_flags == GFP_KERNEL && (map_flags & ~BPF_F_LOCK) != BPF_NOEXIST)
return ERR_PTR(-EINVAL);
esp. stuff like
unlikely((map_flags & ~BPF_F_LOCK) > BPF_EXIST
I think we should be more explicit in what we are checking so that
it's easier to read.
>
> > The check_flags() before this should have error out.
> >
> > Can you share a pointer to the report from kernel test robot?
> >
> I'm unable to find a link to the report, so I will copy/paste the contents:
>
> From: kernel test robot <lkp@intel.com>
> Date: Tue, Mar 22, 2022 at 11:36 AM
> Subject: [bpf-next:master] BUILD SUCCESS
> e52b8f5bd3d2f7b2f4b98067db33bc2fdc125643
> To: BPF build status <bpf@iogearbox.net>
>
>
> tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git
> master
> branch HEAD: e52b8f5bd3d2f7b2f4b98067db33bc2fdc125643 selftests/bpf:
> Fix kprobe_multi test.
>
> Unverified Warning (likely false positive, please contact us if interested):
It's indeed a false positive then.
>
> kernel/bpf/bpf_local_storage.c:473:2: warning: Potential leak of
> memory pointed to by 'selem' [clang-analyzer-unix.Malloc]
>
> Warning ids grouped by kconfigs:
>
> clang_recent_errors
> `-- i386-randconfig-c001
> `-- kernel-bpf-bpf_local_storage.c:warning:Potential-leak-of-memory-pointed-to-by-selem-clang-analyzer-unix.Malloc
>
> elapsed time: 723m
>
> > > + mem_uncharge(smap, owner, smap->elem_size);
> > > + kfree(selem);
> > > + }
> > > + return old_sdata;
> > > }
> > >
> > > if (gfp_flags != GFP_KERNEL) {
> > > @@ -466,10 +470,9 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
> > > if (old_sdata) {
> > > bpf_selem_unlink_map(SELEM(old_sdata));
> > > bpf_selem_unlink_storage_nolock(local_storage, SELEM(old_sdata),
> > > - false);
> > > + gfp_flags == GFP_KERNEL);
> > > }
> > >
> > > -unlock:
> > > raw_spin_unlock_irqrestore(&local_storage->lock, flags);
> > > return SDATA(selem);
> > >
> > > --
> > > 2.30.2
> > >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v1] bpf: bpf_local_storage_update fixes
2022-03-22 22:38 ` Joanne Koong
2022-03-22 22:54 ` KP Singh
@ 2022-03-22 23:07 ` Martin KaFai Lau
2022-03-22 23:28 ` Joanne Koong
1 sibling, 1 reply; 6+ messages in thread
From: Martin KaFai Lau @ 2022-03-22 23:07 UTC (permalink / raw)
To: Joanne Koong
Cc: Joanne Koong, bpf, Alexei Starovoitov, KP Singh, Daniel Borkmann,
Andrii Nakryiko, kernel test robot
On Tue, Mar 22, 2022 at 03:38:29PM -0700, Joanne Koong wrote:
> On Tue, Mar 22, 2022 at 2:57 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > On Tue, Mar 22, 2022 at 02:15:13PM -0700, Joanne Koong wrote:
> > > From: Joanne Koong <joannelkoong@gmail.com>
> > >
> > > This fixes two things in bpf_local_storage_update:
> > >
> > > 1) A memory leak where if bpf_selem_alloc is called right before we
> > > acquire the spinlock and we hit the case where we can copy the new
> > > value into old_sdata directly, we need to free the selem allocation
> > > and uncharge the memory before we return. This was reported by the
> > > kernel test robot.
> > >
> > > 2) A charge leak where if bpf_selem_alloc is called right before we
> > > acquire the spinlock and we hit the case where old_sdata exists and we
> > > need to unlink the old selem, we need to make sure the old selem gets
> > > uncharged.
> > >
> > > Fixes: b00fa38a9c1c ("bpf: Enable non-atomic allocations in local storage")
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > ---
> > > kernel/bpf/bpf_local_storage.c | 11 +++++++----
> > > 1 file changed, 7 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
> > > index 01aa2b51ec4d..2d33af0368ba 100644
> > > --- a/kernel/bpf/bpf_local_storage.c
> > > +++ b/kernel/bpf/bpf_local_storage.c
> > > @@ -435,8 +435,12 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
> > > if (old_sdata && (map_flags & BPF_F_LOCK)) {
> > > copy_map_value_locked(&smap->map, old_sdata->data, value,
> > > false);
> > > - selem = SELEM(old_sdata);
> > > - goto unlock;
> > > + raw_spin_unlock_irqrestore(&local_storage->lock, flags);
> > > + if (selem) {
> > There is an earlier test ensures GFP_KERNEL can only
> > be used with BPF_NOEXIST.
> >
>
> I agree, we currently will never run into this case (since the
> GFP_KERNEL case will error out if old_sdata exists), but my thinking
> was that maybe in the future it may not always hold that GFP_KERNEL
> will always be coupled with BPF_NOEXIST, so this change would
> defensively protect against that.
Then the earlier GFP_KERNEL and BPF_NOEXIST check is enough
to guard the future change without considering other implications first.
It is better to fail early for the cases that it does not support (like
the existing GFP_KERNEL and BPF_NOEXIST check).
or make it truely work for all other cases (if there is a use case).
> > The check_flags() before this should have error out.
> >
> > Can you share a pointer to the report from kernel test robot?
> >
> I'm unable to find a link to the report, so I will copy/paste the contents:
>
> From: kernel test robot <lkp@intel.com>
> Date: Tue, Mar 22, 2022 at 11:36 AM
> Subject: [bpf-next:master] BUILD SUCCESS
> e52b8f5bd3d2f7b2f4b98067db33bc2fdc125643
> To: BPF build status <bpf@iogearbox.net>
>
>
> tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git
> master
> branch HEAD: e52b8f5bd3d2f7b2f4b98067db33bc2fdc125643 selftests/bpf:
> Fix kprobe_multi test.
>
> Unverified Warning (likely false positive, please contact us if interested):
It is indeed a false positive. I am not very convinced this needs
to be silenced in the expense of adding unneeded logic in
this function and makes it harder to read.
>
> kernel/bpf/bpf_local_storage.c:473:2: warning: Potential leak of
> memory pointed to by 'selem' [clang-analyzer-unix.Malloc]
>
> Warning ids grouped by kconfigs:
>
> clang_recent_errors
> `-- i386-randconfig-c001
> `-- kernel-bpf-bpf_local_storage.c:warning:Potential-leak-of-memory-pointed-to-by-selem-clang-analyzer-unix.Malloc
>
> elapsed time: 723m
>
> > > + mem_uncharge(smap, owner, smap->elem_size);
> > > + kfree(selem);
> > > + }
> > > + return old_sdata;
> > > }
> > >
> > > if (gfp_flags != GFP_KERNEL) {
> > > @@ -466,10 +470,9 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
> > > if (old_sdata) {
> > > bpf_selem_unlink_map(SELEM(old_sdata));
> > > bpf_selem_unlink_storage_nolock(local_storage, SELEM(old_sdata),
> > > - false);
> > > + gfp_flags == GFP_KERNEL);
> > > }
> > >
> > > -unlock:
> > > raw_spin_unlock_irqrestore(&local_storage->lock, flags);
> > > return SDATA(selem);
> > >
> > > --
> > > 2.30.2
> > >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v1] bpf: bpf_local_storage_update fixes
2022-03-22 23:07 ` Martin KaFai Lau
@ 2022-03-22 23:28 ` Joanne Koong
0 siblings, 0 replies; 6+ messages in thread
From: Joanne Koong @ 2022-03-22 23:28 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Joanne Koong, bpf, Alexei Starovoitov, KP Singh, Daniel Borkmann,
Andrii Nakryiko, kernel test robot
On Tue, Mar 22, 2022 at 4:07 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Tue, Mar 22, 2022 at 03:38:29PM -0700, Joanne Koong wrote:
> > On Tue, Mar 22, 2022 at 2:57 PM Martin KaFai Lau <kafai@fb.com> wrote:
> > >
> > > On Tue, Mar 22, 2022 at 02:15:13PM -0700, Joanne Koong wrote:
> > > > From: Joanne Koong <joannelkoong@gmail.com>
> > > >
> > > > This fixes two things in bpf_local_storage_update:
> > > >
> > > > 1) A memory leak where if bpf_selem_alloc is called right before we
> > > > acquire the spinlock and we hit the case where we can copy the new
> > > > value into old_sdata directly, we need to free the selem allocation
> > > > and uncharge the memory before we return. This was reported by the
> > > > kernel test robot.
> > > >
> > > > 2) A charge leak where if bpf_selem_alloc is called right before we
> > > > acquire the spinlock and we hit the case where old_sdata exists and we
> > > > need to unlink the old selem, we need to make sure the old selem gets
> > > > uncharged.
> > > >
> > > > Fixes: b00fa38a9c1c ("bpf: Enable non-atomic allocations in local storage")
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > ---
> > > > kernel/bpf/bpf_local_storage.c | 11 +++++++----
> > > > 1 file changed, 7 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
> > > > index 01aa2b51ec4d..2d33af0368ba 100644
> > > > --- a/kernel/bpf/bpf_local_storage.c
> > > > +++ b/kernel/bpf/bpf_local_storage.c
> > > > @@ -435,8 +435,12 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
> > > > if (old_sdata && (map_flags & BPF_F_LOCK)) {
> > > > copy_map_value_locked(&smap->map, old_sdata->data, value,
> > > > false);
> > > > - selem = SELEM(old_sdata);
> > > > - goto unlock;
> > > > + raw_spin_unlock_irqrestore(&local_storage->lock, flags);
> > > > + if (selem) {
> > > There is an earlier test ensures GFP_KERNEL can only
> > > be used with BPF_NOEXIST.
> > >
> >
> > I agree, we currently will never run into this case (since the
> > GFP_KERNEL case will error out if old_sdata exists), but my thinking
> > was that maybe in the future it may not always hold that GFP_KERNEL
> > will always be coupled with BPF_NOEXIST, so this change would
> > defensively protect against that.
> Then the earlier GFP_KERNEL and BPF_NOEXIST check is enough
> to guard the future change without considering other implications first.
>
> It is better to fail early for the cases that it does not support (like
> the existing GFP_KERNEL and BPF_NOEXIST check).
>
> or make it truely work for all other cases (if there is a use case).
>
> > > The check_flags() before this should have error out.
> > >
> > > Can you share a pointer to the report from kernel test robot?
> > >
> > I'm unable to find a link to the report, so I will copy/paste the contents:
> >
> > From: kernel test robot <lkp@intel.com>
> > Date: Tue, Mar 22, 2022 at 11:36 AM
> > Subject: [bpf-next:master] BUILD SUCCESS
> > e52b8f5bd3d2f7b2f4b98067db33bc2fdc125643
> > To: BPF build status <bpf@iogearbox.net>
> >
> >
> > tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git
> > master
> > branch HEAD: e52b8f5bd3d2f7b2f4b98067db33bc2fdc125643 selftests/bpf:
> > Fix kprobe_multi test.
> >
> > Unverified Warning (likely false positive, please contact us if interested):
> It is indeed a false positive. I am not very convinced this needs
> to be silenced in the expense of adding unneeded logic in
> this function and makes it harder to read.
Sounds good, I will abandon this patch then. Thanks for your input,
Martin and KP.
>
> >
> > kernel/bpf/bpf_local_storage.c:473:2: warning: Potential leak of
> > memory pointed to by 'selem' [clang-analyzer-unix.Malloc]
> >
> > Warning ids grouped by kconfigs:
> >
> > clang_recent_errors
> > `-- i386-randconfig-c001
> > `-- kernel-bpf-bpf_local_storage.c:warning:Potential-leak-of-memory-pointed-to-by-selem-clang-analyzer-unix.Malloc
> >
> > elapsed time: 723m
> >
> > > > + mem_uncharge(smap, owner, smap->elem_size);
> > > > + kfree(selem);
> > > > + }
> > > > + return old_sdata;
> > > > }
> > > >
> > > > if (gfp_flags != GFP_KERNEL) {
> > > > @@ -466,10 +470,9 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
> > > > if (old_sdata) {
> > > > bpf_selem_unlink_map(SELEM(old_sdata));
> > > > bpf_selem_unlink_storage_nolock(local_storage, SELEM(old_sdata),
> > > > - false);
> > > > + gfp_flags == GFP_KERNEL);
> > > > }
> > > >
> > > > -unlock:
> > > > raw_spin_unlock_irqrestore(&local_storage->lock, flags);
> > > > return SDATA(selem);
> > > >
> > > > --
> > > > 2.30.2
> > > >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-22 23:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-22 21:15 [PATCH bpf-next v1] bpf: bpf_local_storage_update fixes Joanne Koong
2022-03-22 21:56 ` Martin KaFai Lau
2022-03-22 22:38 ` Joanne Koong
2022-03-22 22:54 ` KP Singh
2022-03-22 23:07 ` Martin KaFai Lau
2022-03-22 23:28 ` Joanne Koong
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).