On Tue, Mar 22, 2022 at 2:57 PM Martin KaFai Lau <kafai@xxxxxx> wrote:
>
> On Tue, Mar 22, 2022 at 02:15:13PM -0700, Joanne Koong wrote:
> > From: Joanne Koong <joannelkoong@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
Date: Tue, Mar 22, 2022 at 11:36 AM
Subject: [bpf-next:master] BUILD SUCCESS
e52b8f5bd3d2f7b2f4b98067db33bc2fdc125643
To: BPF build status <bpf@xxxxxxxxxxxxx>
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
> >
