On Tue, Mar 22, 2022 at 11:38 PM Joanne Koong <joannelkoong@xxxxxxxxx> 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.
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@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'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
> > >
