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