diff mbox series

[bpf-next,v1] bpf: bpf_local_storage_update fixes

Message ID 20220322211513.3994356-1-joannekoong@fb.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series [bpf-next,v1] bpf: bpf_local_storage_update fixes | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR fail PR summary
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2 this patch: 2
netdev/cc_maintainers warning 4 maintainers not CCed: john.fastabend@gmail.com netdev@vger.kernel.org yhs@fb.com songliubraving@fb.com
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 7 this patch: 7
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 25 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Joanne Koong March 22, 2022, 9:15 p.m. UTC
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(-)

Comments

Martin KaFai Lau March 22, 2022, 9:56 p.m. UTC | #1
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
>
Joanne Koong March 22, 2022, 10:38 p.m. UTC | #2
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
> >
KP Singh March 22, 2022, 10:54 p.m. UTC | #3
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
> > >
Martin KaFai Lau March 22, 2022, 11:07 p.m. UTC | #4
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
> > >
Joanne Koong March 22, 2022, 11:28 p.m. UTC | #5
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
> > > >
diff mbox series

Patch

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