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