[bpf-next,libbpf] remove unnecessary null check in kernel_supports()

Commit Message

Eduard Zingerman Jan. 31, 2024, 9:26 p.m. UTC

After recent changes, Coverity complained about inconsistent null
checks in kernel_supports() function:

    kernel_supports(const struct bpf_object *obj, ...)
    ...
    // var_compare_op: Comparing obj to null implies that obj might be null
    if (obj && obj->gen_loader)
        return true;

    // var_deref_op: Dereferencing null pointer obj
    if (obj->token_fd)
        return feat_supported(obj->feat_cache, feat_id);
    ...

- The original null check was introduced by commit [0],
  which introduced a call `kernel_supports(NULL, ...)`
  in function bump_rlimit_memlock();
- This call was refactored to use `feat_supported(NULL, ...)`
  in commit [1].

Looking at all places where kernel_supports() is called:
- there is either `obj->...` access before the call;
- or `obj` comes from `prog->obj` expression, where `prog` comes from
  enumeration of programs in `obj`;
- or `obj` comes from `prog->obj`, where `prog` is a parameter to one
  of the API functions:
  - bpf_program__attach_kprobe_opts;
  - bpf_program__attach_kprobe;
  - bpf_program__attach_ksyscall.

Assuming correct API usage, it appears that `obj` can never be null
when passed to kernel_supports(). Silence the Coverity warning by
removing redundant null check.

[0] e542f2c4cd16 ("libbpf: Auto-bump RLIMIT_MEMLOCK if kernel needs it for BPF")
[1] d6dd1d49367a ("libbpf: Further decouple feature checking logic from bpf_object")

Signed-off-by: Eduard Zingerman <[e-mail]>
---
 tools/lib/bpf/libbpf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Daniel Borkmann Feb. 1, 2024, 10:28 a.m. UTC | #1

On 1/31/24 10:26 PM, Eduard Zingerman wrote:
> After recent changes, Coverity complained about inconsistent null
> checks in kernel_supports() function:
> 
>      kernel_supports(const struct bpf_object *obj, ...)
>      ...
>      // var_compare_op: Comparing obj to null implies that obj might be null
>      if (obj && obj->gen_loader)
>          return true;
> 
>      // var_deref_op: Dereferencing null pointer obj
>      if (obj->token_fd)
>          return feat_supported(obj->feat_cache, feat_id);
>      ...
> 
> - The original null check was introduced by commit [0],
>    which introduced a call `kernel_supports(NULL, ...)`
>    in function bump_rlimit_memlock();
> - This call was refactored to use `feat_supported(NULL, ...)`
>    in commit [1].
> 
> Looking at all places where kernel_supports() is called:
> - there is either `obj->...` access before the call;
> - or `obj` comes from `prog->obj` expression, where `prog` comes from
>    enumeration of programs in `obj`;
> - or `obj` comes from `prog->obj`, where `prog` is a parameter to one
>    of the API functions:
>    - bpf_program__attach_kprobe_opts;
>    - bpf_program__attach_kprobe;
>    - bpf_program__attach_ksyscall.
> 
> Assuming correct API usage, it appears that `obj` can never be null
> when passed to kernel_supports(). Silence the Coverity warning by
> removing redundant null check.
> 
> [0] e542f2c4cd16 ("libbpf: Auto-bump RLIMIT_MEMLOCK if kernel needs it for BPF")
> [1] d6dd1d49367a ("libbpf: Further decouple feature checking logic from bpf_object")
> 
> Signed-off-by: Eduard Zingerman <[e-mail]>

lgtm, also on the latter the struct bpf_program is only defined in libbpf.c, and
you can only use libbpf.h APIs to retrieve a pointer of this type from the obj.

[e-mail] Feb. 1, 2024, 10:30 a.m. UTC | #2

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <[e-mail]>:

On Wed, 31 Jan 2024 23:26:15 +0200 you wrote:
> After recent changes, Coverity complained about inconsistent null
> checks in kernel_supports() function:
> 
>     kernel_supports(const struct bpf_object *obj, ...)
>     ...
>     // var_compare_op: Comparing obj to null implies that obj might be null
>     if (obj && obj->gen_loader)
>         return true;
> 
> [...]

Here is the summary with links:
  - [bpf-next,libbpf] remove unnecessary null check in kernel_supports()
    https://git.kernel.org/bpf/bpf-next/c/8263b3382d8c

You are awesome, thank you!

Patch

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index db65ea59a05a..f6953d7faff1 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4661,7 +4661,7 @@  bpf_object__probe_loading(struct bpf_object *obj)
 
 bool kernel_supports(const struct bpf_object *obj, enum kern_feature_id feat_id)
 {
-	if (obj && obj->gen_loader)
+	if (obj->gen_loader)
 		/* To generate loader program assume the latest kernel
 		 * to avoid doing extra prog_load, map_create syscalls.
 		 */