Re: [PATCH RFC v2 8/8] selftests/dmabuf-heaps: Add dma-buf memcg accounting tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] selftests/dmabuf-heaps: missing skip check in test_xfer_charge_cgroupfd_self()
- [Medium] selftests/dmabuf-heaps: missing cgroup2 and memcg checks in open_self_cgroupfd()
- [Medium] selftests/dmabuf-heaps: vmtest.sh executes runner from unshared /tmp in guest
- [Medium] selftests/dmabuf-heaps: vmtest.sh runs vng/make before check_deps()
--

commit 0809a03b2fbc81e6f2b637ef5c0744b2c8ae99f1
Author: Albert Esteve <aesteve@redhat.com>

selftests/dmabuf-heaps: Add dma-buf memcg accounting tests

This commit adds selftests for dma-buf memcg accounting via charge_fd in
tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c and
tools/testing/selftests/cgroup/test_memcontrol.c, along with a virtme-ng
runner script in tools/testing/selftests/dmabuf-heaps/vmtest.sh.

> diff --git a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
> index 1d49df6719198..b1c58e5883f64 100644
> --- a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
> +++ b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c

[ ... ]

> @@ -496,6 +505,213 @@ static void test_alloc_no_fd_leak_on_efault(char *heap_name)

[ ... ]

> +static int open_self_cgroupfd(void)
> +{
> +	char cgroup_path[PATH_MAX];
> +	char buf[PATH_MAX];
> +	FILE *f;
> +
> +	f = fopen("/proc/self/cgroup", "r");
> +	if (!f)
> +		return -1;
> +
> +	cgroup_path[0] = '\0';
> +	while (fgets(buf, sizeof(buf), f)) {
> +		char *p = buf;
> +
> +		if (strncmp(p, "0::", 3) == 0) {
> +			p += 3;
> +			p[strcspn(p, "\n")] = '\0';
> +			snprintf(cgroup_path, sizeof(cgroup_path),
> +				 "/sys/fs/cgroup%s", p);
> +			break;
> +		}
> +	}
> +	fclose(f);
> +
> +	if (!cgroup_path[0])
> +		return -1;
> +
> +	return open(cgroup_path, O_RDONLY | O_DIRECTORY);
> +}
> +
> +/*
> + * Test: allocate charging the calling process's own cgroup via a cgroupfd.
> + */
> +static void test_alloc_cgroupfd_self(char *heap_name)
> +{
> +	int heap_fd = -1, cgroup_fd = -1, dmabuf_fd = -1, ret;
> +
> +	heap_fd = dmabuf_heap_open(heap_name);
> +
> +	if (!pidfd_alloc_supported(heap_fd)) {
> +		ksft_test_result_skip("charge_fd not supported by this kernel\n");
> +		goto out;
> +	}
> +
> +	cgroup_fd = open_self_cgroupfd();
> +	if (cgroup_fd < 0) {
> +		ksft_test_result_skip("cannot open self cgroupfd\n");
> +		goto out;
> +	}
> +
> +	ret = dmabuf_heap_alloc_pidfd(heap_fd, ONE_MEG, 0, cgroup_fd, &dmabuf_fd);
> +	ksft_test_result(!ret, "Allocation with cgroupfd %d\n", ret);
> +	if (dmabuf_fd >= 0)
> +		close(dmabuf_fd);
> +	close(cgroup_fd);
> +out:
> +	close(heap_fd);
> +}

[Severity: Medium]
Can open_self_cgroupfd() return a directory fd that causes
test_alloc_cgroupfd_self() and test_xfer_charge_cgroupfd_self() to report a
hard failure instead of skipping when cgroup v2 or the memory controller is
not active on that path?

In open_self_cgroupfd(), the 0:: entry from /proc/self/cgroup is appended to a
hardcoded /sys/fs/cgroup prefix without checking that /sys/fs/cgroup is a
cgroup2 mount. On systems using a hybrid hierarchy where /sys/fs/cgroup is a
tmpfs and unified cgroup v2 is mounted at /sys/fs/cgroup/unified, opening
/sys/fs/cgroup/ succeeds on tmpfs, and the kernel's cgroup_get_from_fd() then
rejects charge_fd with EBADF.

Furthermore, unlike the pidfd path in dma_heap_ioctl_allocate() and
dma_buf_xfer_charge() which resolves the effective memcg via
get_mem_cgroup_from_mm(task->mm), the cgroupfd path calls
cgroup_css(cgrp, &memory_cgrp_subsys) directly on the target cgroup and
returns ENOENT if the memory controller is not enabled for that specific
cgroup (for example, when the test runs in a non-root cgroup whose parent has
not enabled +memory in cgroup.subtree_control).

Should open_self_cgroupfd() verify that the resolved directory is on a
cgroup2 filesystem with the memory controller enabled, or should the callers
skip when the ioctl fails with ENOENT or EBADF?

> +
> +/*
> + * Test: transfer a dma-buf's charge to self cgroup via DMA_BUF_IOCTL_XFER_CHARGE.
> + */
> +static void test_xfer_charge_cgroupfd_self(char *heap_name)
> +{
> +	int heap_fd = -1, cgroup_fd = -1, dmabuf_fd = -1, ret;
> +	struct dma_buf_transfer_charge xfer = { 0 };
> +
> +	heap_fd = dmabuf_heap_open(heap_name);
> +
> +	ret = dmabuf_heap_alloc(heap_fd, ONE_MEG, 0, &dmabuf_fd);
> +	if (ret) {
> +		ksft_test_result_fail("Allocation failed %d\n", ret);
> +		goto out;
> +	}
> +
> +	cgroup_fd = open_self_cgroupfd();
> +	if (cgroup_fd < 0) {
> +		ksft_test_result_skip("cannot open self cgroupfd\n");
> +		close(dmabuf_fd);
> +		goto out;
> +	}
> +
> +	xfer.charge_fd = cgroup_fd;
> +	ret = ioctl(dmabuf_fd, DMA_BUF_IOCTL_XFER_CHARGE, &xfer);
> +	ksft_test_result(!ret, "XFER_CHARGE to self cgroupfd %d\n", ret);
> +
> +	close(cgroup_fd);
> +	close(dmabuf_fd);
>  out:
>  	close(heap_fd);
>  }

[Severity: Medium]
Unlike test_alloc_pidfd_self(), test_alloc_pidfd_child(), and
test_alloc_cgroupfd_self(), test_xfer_charge_cgroupfd_self() does not call
pidfd_alloc_supported(heap_fd) or check for errno == ENOTTY when
ioctl(dmabuf_fd, DMA_BUF_IOCTL_XFER_CHARGE, &xfer) fails.

On an unpatched kernel where dmabuf_heap_alloc() and open_self_cgroupfd()
both succeed, won't ioctl(dmabuf_fd, DMA_BUF_IOCTL_XFER_CHARGE, &xfer) return
-1 with errno == ENOTTY and cause ksft_test_result(!ret, ...) to report a
failure instead of skipping?

[ ... ]

> diff --git a/tools/testing/selftests/dmabuf-heaps/vmtest.sh b/tools/testing/selftests/dmabuf-heaps/vmtest.sh
> new file mode 100755
> index 0000000000000..434857b765b84
> --- /dev/null
> +++ b/tools/testing/selftests/dmabuf-heaps/vmtest.sh
> @@ -0,0 +1,205 @@

[ ... ]

> +readonly DMABUF_HEAP_TEST="${SCRIPT_DIR}"/dmabuf-heap
> +readonly MEMCONTROL_TEST="${CGROUP_DIR}"/test_memcontrol
> +readonly TMP_DIR=$(mktemp -d /tmp/dmabuf-vmtest.XXXXXXXX)

[ ... ]

> +build_selftests() {
> +	make -C "${KERNEL_CHECKOUT}" headers_install \
> +		INSTALL_HDR_PATH="${TMP_DIR}/usr" -j"$(nproc)"
> +
> +	local khdr="-isystem ${TMP_DIR}/usr/include"
> +
> +	if ! make -C "${SCRIPT_DIR}" KHDR_INCLUDES="${khdr}" -j"$(nproc)"; then
> +		die "failed to build dmabuf-heaps selftests"
> +	fi
> +
> +	if ! make -C "${CGROUP_DIR}" KHDR_INCLUDES="${khdr}" \
> +		"${MEMCONTROL_TEST}" -j"$(nproc)"; then
> +		die "failed to build cgroup/test_memcontrol selftest"
> +	fi
> +}

[ ... ]

> +make_runner() {
> +	# virtme-ng shares the host filesystem, so TMP_DIR is accessible
> +	# inside the VM at the same absolute path.
> +	cat > "${TMP_DIR}/run_tests.sh" <<-EOF

[ ... ]

> +	EOF
> +	chmod +x "${TMP_DIR}/run_tests.sh"
> +}
> +
> +run_vm() {
> +	local verbose_opt=""
> +	local kernel_opt=""
> +
> +	${VERBOSE} && verbose_opt="--verbose"
> +
> +	# If we are running from within the kernel source tree, use the kernel
> +	# source tree as the kernel to boot, otherwise use the running kernel.
> +	if [[ "$(realpath "$(pwd)")" == "${KERNEL_CHECKOUT}"* ]]; then
> +		kernel_opt="${KERNEL_CHECKOUT}"
> +	fi
> +
> +	vng --run ${kernel_opt} ${verbose_opt} --user root --memory 512M \
> +		--exec "${TMP_DIR}/run_tests.sh"
> +}

[Severity: Medium]
Will "${TMP_DIR}/run_tests.sh" be visible inside the guest VM when TMP_DIR is
created under /tmp?

When virtme-ng boots the guest, virtme-init mounts a fresh tmpfs over /tmp
inside the VM, which masks the host's /tmp directory unless TMP_DIR is
explicitly shared via --rodir / --rwdir or created under a shared directory
such as SCRIPT_DIR. Won't vng --run --exec "${TMP_DIR}/run_tests.sh" fail in
the guest because /tmp/dmabuf-vmtest.XXXXXXXX/run_tests.sh does not exist
there?

> +
> +while getopts :hvbH:p: o
> +do
> +	case $o in
> +	v) VERBOSE=true;;
> +	b) BUILD=true;;
> +	H) BUILD_HOST=$OPTARG;;
> +	p) BUILD_HOST_PODMAN_CONTAINER_NAME=$OPTARG;;
> +	h|*) usage;;
> +	esac
> +done
> +shift $((OPTIND-1))
> +
> +trap cleanup EXIT
> +
> +check_vng
> +handle_build
> +check_deps
> +make_runner
> +
> +echo "Booting VM and running tests..."
> +run_vm

[Severity: Medium]
Should check_deps() verify vng and make before check_vng() and handle_build()
are called?

Right now, if vng or make is not installed, check_vng() runs vng --version
first and prints a command-not-found error plus an untested-version warning,
and if -b is passed, handle_build() fails via die() with KSFT_FAIL instead of
exiting with KSFT_SKIP in check_deps().

Also, in handle_build(), the script uses pushd "${KERNEL_CHECKOUT}" and popd
to build the kernel, whereas run_vm() only sets kernel_opt="${KERNEL_CHECKOUT}"
when $(pwd) is inside KERNEL_CHECKOUT. If vmtest.sh -b is invoked from
outside KERNEL_CHECKOUT, won't run_vm() ignore the freshly built kernel and
boot the running host kernel instead? And in build_selftests(), should the
make headers_install command also be checked for failure before building the
tests?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-v2_20230123_tjmercier_google_com-v2-0-ef6ee2dd8c5c@redhat.com?part=8