Re: [PATCH bpf-next v2 3/3] libbpf: fix feature probe failures for implicit BPF tokens

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Modifying `probe_fd()` to return negative error codes breaks feature sanitization by forcibly overriding missing features to `FEAT_SUPPORTED` if a BPF token is present.
- [Medium] `probe_fd()` returns a literal `-1` from direct syscall wrappers, causing `feat_supported()` to always report `-EPERM` and mask the true error code.
- [Medium] Missing curly braces in the BPF token rescue logic causes unconditional warning spam for all probe failures.
- [Medium] Several feature probes return `0` instead of propagating negative error codes on failure, silently bypassing the new BPF token rescue mechanism.
--

commit 0eceda08312936da19e1be0b6d87937f64f2f067
Author: Yuan Chen <chenyuan@kylinos.cn>
Subject: libbpf: fix feature probe failures for implicit BPF tokens

This commit modifies probe_fd() to return negative values instead of 0 on
failure. It also updates feat_supported() to mark features as supported if a
BPF token is present and the probe returns a negative error.

> diff --git a/tools/lib/bpf/features.c b/tools/lib/bpf/features.c
> index b7e388f99d0bb..f934452e52f01 100644
> --- a/tools/lib/bpf/features.c
> +++ b/tools/lib/bpf/features.c
> @@ -16,7 +16,14 @@ int probe_fd(int fd)
>  {
>  	if (fd >= 0)
>  		close(fd);
> -	return fd >= 0;
> +	/* Return 1 on success, negative error on failure, so
> +	 * feat_supported() can distinguish probe errors from
> +	 * genuine feature absence.  When a BPF token is present,
> +	 * a negative return triggers the rescue path that marks
> +	 * the feature as SUPPORTED (token creation itself proves
> +	 * the kernel BPF subsystem works).
> +	 */
> +	return fd >= 0 ? 1 : fd;

[Severity: Medium]
Does this return a literal -1 instead of the actual error code?

Several feature probes use internal direct syscall wrappers like
sys_bpf_prog_load() which return -1 on failure and leave the real error in
errno:

tools/lib/bpf/features.c:probe_kern_prog_name() {
    ...
	ret = sys_bpf_prog_load(&attr, attr_sz, PROG_LOAD_ATTEMPTS);
	return probe_fd(ret);
}

Since probe_fd() returns this -1 unmodified instead of -errno,
feat_supported() will pass -1 to errstr(). Because -1 equals -EPERM, does
this cause the error string to unconditionally resolve to "Operation not
permitted" and mask the true reason for the probe failure (like -EINVAL)?

[ ... ]

> @@ -725,9 +732,21 @@ bool feat_supported(struct kern_feature_cache *cache, enum kern_feature_id feat_
>  		} else if (ret == 0) {
>  			WRITE_ONCE(cache->res[feat_id], FEAT_MISSING);
>  		} else {
> +			/*
> +			 * A BPF token may restrict which program/map types
> +			 * are permitted, causing the probe to fail even
> +			 * though the kernel supports the feature.  When a
> +			 * token is present the probe is best-effort: BPF
> +			 * token creation itself proves the kernel has a
> +			 * working BPF subsystem.  Real BPF issues will be
> +			 * caught during actual program/map loading.
> +			 */
> +			if (cache->token_fd)
> +				WRITE_ONCE(cache->res[feat_id], FEAT_SUPPORTED);

[Severity: High]
Will this forcefully override genuine missing features to FEAT_SUPPORTED if
a token is present?

By returning < 0 from probe_fd(), genuine feature absences now fall into this
error-handling path. If a newer BPF feature is missing on an older kernel
that supports tokens, libbpf might incorrectly mark it as supported and skip
essential sanitizations (like stripping unsupported BTF tags). Does this
bypass the fallback logic and cause the final program load to fatally fail?

[Severity: Medium]
Are there existing probes that still return 0 on failure and bypass this
rescue mechanism?

For instance, probe_prog_bind_map() returns 0 if bpf_prog_load() fails:

tools/lib/bpf/features.c:probe_prog_bind_map() {
    ...
	prog = bpf_prog_load(BPF_PROG_TYPE_SOCKET_FILTER, NULL, "GPL", insns,
                         insn_cnt, &prog_opts);
	if (prog < 0) {
		close(map);
		return 0;
	}
    ...
}

Since these functions were not updated to propagate negative error codes, they
evaluate via the else if (ret == 0) branch and are immediately marked as
FEAT_MISSING. Does this leave unprivileged BPF token support broken for these
specific features?

> +			else
> +				WRITE_ONCE(cache->res[feat_id], FEAT_MISSING);
>  			pr_warn("Detection of kernel %s support failed: %s\n",
>  				feat->desc, errstr(ret));

[Severity: Medium]
Is the else block missing curly braces?

Without curly braces around the else block, the pr_warn() call executes
unconditionally for any negative return value. Combined with probe_fd() now
returning < 0 for all missing features, will this loudly spam warnings to the
console for normal feature absence checks?

> -			WRITE_ONCE(cache->res[feat_id], FEAT_MISSING);
>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604031033.806727-1-chenyuan_fl@163.com?part=3