Re: [PATCH bpf-next v2 2/2] selftests/bpf: Add test for map-stored struct file kptrs

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [Medium] In `copy_file`, closing an invalid file descriptor overwrites the original error code.
--

commit 975d24d408cdf10dcbd5bc80513a92bbedd7dc6f
Author: Justin Suess <utilityemal77@gmail.com>

selftests/bpf: Add test for map-stored struct file kptrs

This commit adds tests for map-stored struct file referenced kptrs. It
validates that the struct file reference remains valid when the original
file descriptor is closed, and checks that unreferenced struct file
pointers are rejected by the verifier.

> diff --git a/tools/testing/selftests/bpf/prog_tests/refcounted_kptr_file.c b/tools/testing/selftests/bpf/prog_tests/refcounted_kptr_file.c
> new file mode 100644
> index 00000000000000..1f2586a7af9365
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/refcounted_kptr_file.c

[ ... ]

> +static int copy_file(const char *src_path, const char *dst_path, mode_t mode)
> +{
> +	char buf[4096];
> +	int src_fd = -1, dst_fd = -1;
> +	int err = 0;
> +
> +	src_fd = open(src_path, O_RDONLY | O_CLOEXEC);
> +	if (src_fd < 0)
> +		return -errno;
> +
> +	dst_fd = open(dst_path, O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC,
> +		      mode & 0777);
> +	if (dst_fd < 0) {
> +		err = -errno;
> +		goto out;
> +	}

[ ... ]

> +out:
> +	err = close(src_fd) ?: err;
> +	err = close(dst_fd) ?: err;

If opening the destination file fails, dst_fd is left as -1, and the code jumps
here with err set to -errno. Would calling close(-1) here fail and return -1,
causing the ?: operator to overwrite the meaningful error code in err with -1?

Should the file descriptors be guarded with a >= 0 check before calling
close() to prevent masking the true cause of the failure?

> +	return err;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260423152239.4140627-1-utilityemal77@gmail.com?part=2