Re: [PATCH vfs-7.5.bpf v2 3/3] selftests/bpf: Test bpf_path_getattr()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/bpf: missing CONFIG_OVERLAY_FS=y in config for test_overlay()
- [Low] selftests/bpf: missing bpf_path_getattr() prototype in bpf_kfuncs.h
--

commit 17972af984223b5b9a2708744af62ce15fa7e1b6
Author: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>

selftests/bpf: Test bpf_path_getattr()

This commit adds BPF selftests for the bpf_path_getattr() kfunc, comparing
its returned attributes with statx() across tmpfs and overlayfs and verifying
its BPF verifier restrictions.

> diff --git a/tools/testing/selftests/bpf/prog_tests/path_getattr.c b/tools/testing/selftests/bpf/prog_tests/path_getattr.c
> new file mode 100644
> index 0000000000000..6d8cabfd24c65
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/path_getattr.c
> @@ -0,0 +1,162 @@

[ ... ]

> +static void test_overlay(struct path_getattr *skel, const char *root)
> +{

[ ... ]

> +	snprintf(opts, sizeof(opts), "lowerdir=%s,upperdir=%s,workdir=%s,xino=off",
> +		 lower, upper, work);
> +	if (mount("overlay", merged, "overlay", 0, opts)) {
> +		if (errno == ENODEV || errno == EOPNOTSUPP) {
> +			printf("%s:SKIP: overlayfs unavailable: %s\n",
> +			       __func__, strerror(errno));
> +			test__skip();
> +			goto out_lower;
> +		}

[Severity: Medium]
Should CONFIG_OVERLAY_FS=y be added to tools/testing/selftests/bpf/config
so the overlay subtest in test_path_getattr() -> test_overlay() is not
skipped on standard BPF selftest kernels?

The shared tools/testing/selftests/bpf/config enables CONFIG_TMPFS=y and
CONFIG_FS_VERITY=y for filesystem kfunc tests, while CONFIG_OVERLAY_FS=y is
only present in tools/testing/selftests/bpf/config.aarch64. When running on
kernels built from the BPF selftest config on x86_64, s390x, riscv64, or
ppc64el, mount("overlay", ...) fails with ENODEV and skips test_overlay().

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/path_getattr.c b/tools/testing/selftests/bpf/progs/path_getattr.c
> new file mode 100644
> index 0000000000000..509cf13a00479
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/path_getattr.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <vmlinux.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +#include "bpf_kfuncs.h"

[ ... ]

> +SEC("lsm.s/inode_getattr")
> +int BPF_PROG(path_getattr, const struct path *path, int ret)
> +{
> +	struct kstat stat = {};
> +
> +	if (ret || (__u32)bpf_get_current_pid_tgid() != monitored_tid || calls)
> +		return ret;
> +	calls++;
> +	getattr_ret = bpf_path_getattr(path, request_mask, 0, &stat, sizeof(stat));

[Severity: Low]
Both tools/testing/selftests/bpf/progs/path_getattr.c and
tools/testing/selftests/bpf/progs/verifier_path_getattr.c include
bpf_kfuncs.h and call bpf_path_getattr(), which is only built into the
kernel when CONFIG_BPF_LSM=y.

All other filesystem kfuncs from fs/bpf_fs_kfuncs.c are declared with
__ksym __weak in tools/testing/selftests/bpf/bpf_kfuncs.h:

    extern int bpf_get_dentry_xattr(struct dentry *dentry, const char *name__str,
                                    struct bpf_dynptr *value_p) __ksym __weak;
    ...
    extern int bpf_inode_init_xattr(struct xattr *xattrs, int *xattr_count,
                                    const char *name__str,
                                    const struct bpf_dynptr *value_p) __ksym __weak;

Should bpf_path_getattr() also be declared in
tools/testing/selftests/bpf/bpf_kfuncs.h so compilation does not fail when
vmlinux.h is generated without CONFIG_BPF_LSM=y or compiled with
BPF_NO_KFUNC_PROTOTYPES?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102104655.70901-1-tasos.papagiannnis@gmail.com?part=3