linux-kernel - Re: [PATCH bpf-next] Fixes: 14a324f6a67e ("bpf: Wire up freeing of referenced kptr")

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP01T76nG7wQ-WFdjtYr5v-KutAOTRJ_zME-nV6HYUwwYW=z6Q@mail.gmail.com>
Date:   Sun, 4 Sep 2022 23:10:18 +0200
From:   Kumar Kartikeya Dwivedi <memxor@...il.com>
To:     Jules Irenge <jbi.octave@...il.com>
Cc:     ast@...nel.org, linux-kernel@...r.kernel.org, bpf@...r.kernel.org,
        daniel@...earbox.net, martin.lau@...ux.dev,
        Elana.Copperman@...ileye.com
Subject: Re: [PATCH bpf-next] Fixes: 14a324f6a67e ("bpf: Wire up freeing of
 referenced kptr")

On Sun, 4 Sept 2022 at 12:28, Jules Irenge <jbi.octave@...il.com> wrote:
>
> This patch fixes a warning generated by Sparse
>
> "warning: Using plain integer as NULL pointer"
>
> by replacing p with *p in the WRITE_ONCE() macro
>
> This enables the pointer to be cleared on map value delete,
> hence clearing the warning.
>
> Signed-off-by: Jules Irenge <jbi.octave@...il.com>
> ---

1. Always bump the version number when resending a patch
2. Fixes: ... tag needs to come before your SoB.
3. The headline can instead be [PATCH bpf-next vN] bpf: Fix resetting
logic for unreferenced kptrs
where N is your version number.
4. Commit message needs more work. Apart from what you have specified
about sparse, it can be:

During the process of fixing this warning, it was discovered that the
current code erroneously writes to the pointer variable instead
dereferencing and writing to the actual kptr. Hence, the sparse tool
accidently helped tp uncover this problem. Fix this by doing
WRITE_ONCE(*p, 0) instead of WRITE_ONCE(p, 0). Note that the effect of
this bug is that unreferenced kptrs won't be cleared during
check_and_free_fields. It is not a problem if the clearing is not done
during map_free stage, as there is nothing to free for them.

5. There is no requirement to Cc linux-kernel ML for BPF patches.

>  kernel/bpf/syscall.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 27760627370d..f798acd43a28 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -598,7 +598,7 @@ void bpf_map_free_kptrs(struct bpf_map *map, void *map_value)
>                 if (off_desc->type == BPF_KPTR_UNREF) {
>                         u64 *p = (u64 *)btf_id_ptr;
>
> -                       WRITE_ONCE(p, 0);
> +                       WRITE_ONCE(*p, 0);
>                         continue;
>                 }
>                 old_ptr = xchg(btf_id_ptr, 0);
> --
> 2.35.1
>