[PATCH bpf-next v2] bpf: Fix resetting logic for unreferenced kptrs

[PATCH bpf-next v2] bpf: Fix resetting logic for unreferenced kptrs

[Jules Irenge]

Sparse reported a warning at bpf_map_free_kptrs()

"warning: Using plain integer as NULL pointer"

During the process of fixing this warning,
it was discovered that the current code
erroneously writes to the pointer variable
instead of deferencing and writing to the actual kptr.
Hence, Sparse tool accidentally helped to 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 will not 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.

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
Changes in v2:
 - Make commit message clearer
 - Change commit headline
   from Fixes: 14a324f6a67e ("bpf: Wire up freeing of referenced kptr")
   to bpf: Fix resetting logic for unreferenced kptrs

 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

Re: [PATCH bpf-next v2] bpf: Fix resetting logic for unreferenced kptrs

[Kumar Kartikeya Dwivedi]

On Wed, 7 Sept 2022 at 17:24, Jules Irenge <jbi.octave@gmail.com> wrote:
>
> Sparse reported a warning at bpf_map_free_kptrs()
>
> "warning: Using plain integer as NULL pointer"
>
> During the process of fixing this warning,
> it was discovered that the current code
> erroneously writes to the pointer variable
> instead of deferencing and writing to the actual kptr.
> Hence, Sparse tool accidentally helped to 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 will not 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.
>

You're still missing the fixes tag right before your Signed-off-By.

Instead of

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>

It must be

Fixes: 14a324f6a67e ("bpf: Wire up freeing of referenced kptr")
Signed-off-by: Jules Irenge <jbi.octave@gmail.com>

Re: [PATCH bpf-next v2] bpf: Fix resetting logic for unreferenced kptrs

[Alexei Starovoitov]

On Wed, Sep 7, 2022 at 8:58 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> On Wed, 7 Sept 2022 at 17:24, Jules Irenge <jbi.octave@gmail.com> wrote:
> >
> > Sparse reported a warning at bpf_map_free_kptrs()
> >
> > "warning: Using plain integer as NULL pointer"
> >
> > During the process of fixing this warning,
> > it was discovered that the current code
> > erroneously writes to the pointer variable
> > instead of deferencing and writing to the actual kptr.
> > Hence, Sparse tool accidentally helped to 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 will not 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.
> >
>
> You're still missing the fixes tag right before your Signed-off-By.
>
> Instead of
>
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
>
> It must be
>
> Fixes: 14a324f6a67e ("bpf: Wire up freeing of referenced kptr")
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>

Added while applying.

Re: [PATCH bpf-next v2] bpf: Fix resetting logic for unreferenced kptrs

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Wed, 7 Sep 2022 16:24:20 +0100 you wrote:
> Sparse reported a warning at bpf_map_free_kptrs()
> 
> "warning: Using plain integer as NULL pointer"
> 
> During the process of fixing this warning,
> it was discovered that the current code
> erroneously writes to the pointer variable
> instead of deferencing and writing to the actual kptr.
> Hence, Sparse tool accidentally helped to uncover this problem.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v2] bpf: Fix resetting logic for unreferenced kptrs
    https://git.kernel.org/bpf/bpf-next/c/9fad7fe5b298

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html