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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] selftests/dmabuf-heaps: charge_fd=0 and unskipped EOPNOTSUPP in XFER_CHARGE test
- [Medium] selftests/dmabuf-heaps: pidfd_alloc_supported() treats EOPNOTSUPP as supported
- [Medium] selftests/dmabuf-heaps: missing memory controller check in open_self_cgroupfd()
- [Medium] selftests/dmabuf-heaps: check_deps() runs before handle_build() in vmtest.sh
- [Low] selftests: cgroup: leak of child_cg on cg_create() failure in test_memcg_dmabuf()
--

commit b3c8a8b74499074e19a368c4979733bfe47e9d56
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 and
DMA_BUF_IOCTL_XFER_CHARGE in dmabuf-heaps and cgroup/test_memcontrol, along
with a virtme-ng runner script and Kconfig fragment.

> diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
> index 0ed82347044ed..3c95bcabee2bd 100644
> --- a/tools/testing/selftests/cgroup/test_memcontrol.c
> +++ b/tools/testing/selftests/cgroup/test_memcontrol.c
[ ... ]
> +static int test_memcg_dmabuf(const char *root)
> +{
[ ... ]
> +	parent = cg_name(root, "dmabuf_memcg_test");
> +	if (!parent)
> +		goto cleanup;
> +
> +	if (cg_create(parent))
> +		goto cleanup_parent;
> +
> +	if (cg_write(parent, "cgroup.subtree_control", "+memory"))
> +		goto cleanup_parent;
> +
> +	child_cg = cg_name(parent, "child");
> +	if (!child_cg)
> +		goto cleanup_parent;
> +
> +	if (cg_create(child_cg))
> +		goto cleanup_parent;
[ ... ]
> +cleanup_child:
> +	cg_destroy(child_cg);
> +	free(child_cg);
> +cleanup_parent:
> +	cg_destroy(parent);
> +	free(parent);
> +cleanup:
> +	return ret;
> +}

[Severity: Low]
Does test_memcg_dmabuf() leak the child_cg string buffer if
cg_create(child_cg) fails?

Jumping to cleanup_parent bypasses free(child_cg) under cleanup_child. Should
that error path free child_cg (or jump to cleanup_child) before cleaning up
parent?

> diff --git a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
> index 1d49df6719198..90ef14d2ecba0 100644
> --- a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
> +++ b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
[ ... ]
> +/*
> + * Probe whether the kernel honours charge_fd in DMA_HEAP_IOCTL_ALLOC.
> + */
> +static bool pidfd_alloc_supported(int heap_fd)
> +{
> +	int devnull_fd, dmabuf_fd = -1, ret;
> +
> +	devnull_fd = open("/dev/null", O_RDONLY);
> +	if (devnull_fd < 0)
> +		return false;
> +
> +	ret = dmabuf_heap_alloc_pidfd(heap_fd, ONE_MEG, 0, devnull_fd, &dmabuf_fd);
> +	if (dmabuf_fd >= 0) {
> +		close(dmabuf_fd);
> +		dmabuf_fd = -1;
> +	}
> +	close(devnull_fd);
> +	return ret < 0;
> +}

[Severity: Medium]
As reported by Sashiko on v3 [1], won't pidfd_alloc_supported() return true
when CONFIG_MEMCG is disabled?

When CONFIG_MEMCG=n, the inline stub in include/linux/memcontrol.h returns
-EOPNOTSUPP for any file descriptor:

static inline struct mem_cgroup *mem_cgroup_get_from_fd(int fd)
{
	return ERR_PTR(-EOPNOTSUPP);
}

In dma_heap_ioctl_allocate(), that causes DMA_HEAP_IOCTL_ALLOC to fail with
ret < 0 and errno == EOPNOTSUPP when passed devnull_fd, so
pidfd_alloc_supported() treats the feature as supported and the subsequent
tests fail with EOPNOTSUPP instead of skipping.

Should pidfd_alloc_supported() check ret < 0 && errno != EOPNOTSUPP?

[ ... ]
> +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);
> +}

[Severity: Medium]
As reported by Sashiko on v3 [2], what happens in test_alloc_cgroupfd_self()
and test_xfer_charge_cgroupfd_self() when the test process runs in a cgroup
where the memory controller is not enabled in its parent's
cgroup.subtree_control?

In mm/memcontrol.c:mem_cgroup_get_from_fd(), resolving a cgroup directory fd
requires an active memory controller CSS on that cgroup and returns -ENOENT
otherwise:

	cgrp = cgroup_get_from_fd(fd);
	if (!IS_ERR(cgrp)) {
		rcu_read_lock();
		css = cgroup_css(cgrp, &memory_cgrp_subsys);
		if (!css || !css_tryget_online(css)) {
			rcu_read_unlock();
			cgroup_put(cgrp);
			return ERR_PTR(-ENOENT);
		}

Should open_self_cgroupfd() verify that the memory controller is enabled on
cgroup_path (or should the callers skip on ENOENT) so the tests skip rather
than fail when run outside vmtest.sh in a non-root cgroup without +memory?

[ ... ]
> +/*
> + * 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);
> +
> +	if (!pidfd_alloc_supported(heap_fd)) {
> +		ksft_test_result_skip("charge_fd not supported by this kernel\n");
> +		goto out;
> +	}
> +
> +	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);

[Severity: Medium]
As reported by Sashiko on v3 [3], won't test_xfer_charge_cgroupfd_self() fail
with EOPNOTSUPP under default kernel parameters (dma_heap.mem_accounting=0) or
on heaps that do not set exp_info.memcg (such as CMA heaps)?

Here the buffer is allocated with dmabuf_heap_alloc(), which passes
charge_fd = 0. In drivers/dma-buf/dma-heap.c:dma_heap_ioctl_allocate():

	if (heap_allocation->charge_fd) {
		memcg = mem_cgroup_get_from_fd(heap_allocation->charge_fd);
		if (IS_ERR(memcg))
			return ERR_CAST(memcg);
	}

	if (!memcg && mem_accounting)
		memcg = get_mem_cgroup_from_mm(current->mm);

When charge_fd is 0 and mem_accounting is false (the default), memcg is NULL,
and drivers/dma-buf/dma-buf.c:dma_buf_xfer_charge() returns -EOPNOTSUPP when
dmabuf->memcg is NULL:

	if (!dmabuf->memcg) {
		ret = -EOPNOTSUPP;
		goto out;
	}

Similarly, pidfd_alloc_supported() passes for all heaps because charge_fd is
validated in dma_heap_ioctl_allocate() before heap->ops->allocate(), so
non-system heaps reach DMA_BUF_IOCTL_XFER_CHARGE with dmabuf->memcg == NULL
and also return -EOPNOTSUPP.

Should test_xfer_charge_cgroupfd_self() allocate the buffer with a charge_fd
(e.g. via dmabuf_heap_alloc_pidfd()) and skip when DMA_BUF_IOCTL_XFER_CHARGE
fails with errno == EOPNOTSUPP?

> diff --git a/tools/testing/selftests/dmabuf-heaps/vmtest.sh b/tools/testing/selftests/dmabuf-heaps/vmtest.sh
> new file mode 100755
> index 0000000000000..3574b71ca83e8
> --- /dev/null
> +++ b/tools/testing/selftests/dmabuf-heaps/vmtest.sh
[ ... ]
> +check_deps() {
> +	for dep in vng make; do
> +		if [[ ! -x $(command -v "${dep}") ]]; then
> +			echo -e "skip:    dependency ${dep} not found!\n"
> +			exit "${KSFT_SKIP}"
> +		fi
> +	done
> +
> +	if ${SHELL_MODE}; then
> +		return
> +	fi
> +
> +	if [[ ! -x "${DMABUF_HEAP_TEST}" ]]; then
> +		printf "skip:    %s not found!" "${DMABUF_HEAP_TEST}"
> +		printf " Please build the kselftest dmabuf-heaps target (or use -b).\n"
> +		exit "${KSFT_SKIP}"
> +	fi
> +
> +	if [[ ! -x "${MEMCONTROL_TEST}" ]]; then
> +		printf "skip:    %s not found!" "${MEMCONTROL_TEST}"
> +		printf " Please build the kselftest cgroup target (or use -b).\n"
> +		exit "${KSFT_SKIP}"
> +	fi
> +}
[ ... ]
> +trap cleanup EXIT
> +
> +check_deps
> +check_vng
> +handle_build
> +make_runner

[Severity: Medium]
As reported by Sashiko on v3 [4], if vmtest.sh is invoked with -b on a clean
tree where the selftest binaries have not been built yet, won't check_deps()
exit with KSFT_SKIP before handle_build() has a chance to compile them?

Should handle_build() run before check_deps() (or should check_deps() skip
the binary existence checks when BUILD is true)?

[1] https://lore.kernel.org/r/20261027-v2_20230123_tjmercier_google_com-v3-8-78d2c4cb8647@redhat.com
[2] https://lore.kernel.org/r/20261027-v2_20230123_tjmercier_google_com-v3-8-78d2c4cb8647@redhat.com
[3] https://lore.kernel.org/r/20261027-v2_20230123_tjmercier_google_com-v3-8-78d2c4cb8647@redhat.com
[4] https://lore.kernel.org/r/20261027-v2_20230123_tjmercier_google_com-v3-8-78d2c4cb8647@redhat.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-v2_20230123_tjmercier_google_com-v4-0-1819a017000b@redhat.com?part=8