On Tue, Mar 22, 2022 at 4:07 PM Martin KaFai Lau <kafai@xxxxxx> 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@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.
> 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@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):
> 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
> > > >
