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.
>
> 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
> > >
