Re: [PATCH 09/11] selftests: cgroup: add a vmtest script for memcg

On Fri, Aug 21, 2026 at 07:09:55PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Use of bash-specific `[[` command in SSH remote execution.
> - [Medium] The background `vng` process is orphaned and leaked on error paths before QEMU creates its PID file.
> --
> 
> --- Patch [9]: [PATCH 09/11] selftests: cgroup: add a vmtest script for memcg ---
> commit b0194f809e3b4d9bf07134eab98efb01f6db5c8d
> Author: Eric Chanudet <[email protected]>
> 
> selftests: cgroup: add a vmtest script for memcg
> 
> This patch introduces a vmtest-memcg.sh script to provide a repeatable VM
> workflow using virtme-ng for memcg tests. It boots a VM with CMA enabled in
> its kernel arguments, enables memory_cma_accounting, and runs the cgroup
> tests inside the guest.
> 
> Link: https://lore.kernel.org/all/[email protected]/
> 
> > diff --git a/tools/testing/selftests/cgroup/vmtest-memcg.sh b/tools/testing/selftests/cgroup/vmtest-memcg.sh
> > new file mode 100755
> > index 0000000000000..b19b967317849
> > --- /dev/null
> > +++ b/tools/testing/selftests/cgroup/vmtest-memcg.sh
> [ ... ]
> > +cleanup() {
> > +	if [[ -s "$QEMU_PIDFILE" ]]; then
> > +		pkill -SIGTERM -F "$QEMU_PIDFILE" >/dev/null 2>&1 || true
> > +	fi
> > +
> > +	if [[ -e "$QEMU_PIDFILE" ]]; then
> > +		rm -f "$QEMU_PIDFILE"
> > +	fi
> > +}
> 
> [Severity: Medium]
> Is there a chance the background vng process could be leaked here if the 
> script is interrupted or times out before QEMU creates the PID file?
> 
> If the QEMU PID file is not created in time during vm_start(), the script
> calls die() and triggers this cleanup trap. However, because QEMU_PIDFILE
> is empty or missing, cleanup() won't attempt to terminate anything.
> 
> Since bash doesn't automatically send SIGHUP to background jobs on a
> non-interactive exit, the background vng process might be left running
> indefinitely on the host.
> 

Ok, lets fallback on kill -HUP -$$ then.

> [ ... ]
> > +vm_start() {
> > +	local logfile=/dev/null
> > +	local verbose_opt=""
> > +	local kernel_opt=""
> > +
> [ ... ]
> > +	vng \
> > +		--run \
> > +		${kernel_opt} \
> > +		${verbose_opt} \
> > +		--qemu-opts="$QEMU_OPTS" \
> > +		--qemu="$(command -v "$QEMU")" \
> > +		--user root \
> > +		--ssh "$SSH_GUEST_PORT" \
> > +		--append "cma=64M" \
> > +		--rw &>"$logfile" &
> > +
> > +	local vng_pid=$!
> > +	local elapsed=0
> > +
> > +	while [[ ! -s "$QEMU_PIDFILE" ]]; do
> > +		kill -0 "$vng_pid" 2>/dev/null || \
> > +			die "vng exited early; failed to boot VM"
> > +		[[ "$elapsed" -ge "$WAIT_TOTAL" ]] && \
> > +			die "timed out waiting for VM boot"
> > +		sleep 1
> > +		elapsed=$((elapsed + 1))
> > +	done
> > +}
> 
> (This is where vng is launched and vng_pid is kept strictly as a local
> variable, which prevents the cleanup() function above from falling back to
> terminating it directly.)
> 
> [ ... ]
> > +check_guest_requirements() {
> > +	vm_ssh -- "grep -q memory /sys/fs/cgroup/cgroup.controllers" \
> > +		|| die "memory controller not available (CONFIG_MEMCG?)"
> > +	vm_ssh -- "[[ -e /dev/dma_heap/default_cma_region ]]" \
> > +		|| die "CMA heap not available (CONFIG_DMABUF_HEAPS_CMA? cma= cmdline?)"
> > +}
> 
> [Severity: Medium]
> Could this result in a false positive test failure depending on the guest's
> default shell?
> 
> This passes the bash-specific [[ command directly to SSH, which will execute 
> it using the remote user's default login shell. If the guest VM environment 
> defaults to a strict POSIX shell like dash or /bin/sh, the [[ syntax will 
> fail with a syntax or "not found" error, even if the CMA region exists.
> 
> Would it be safer to use standard POSIX [ -e ... ] instead?

Yes.

> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9
> 

-- 
Eric Chanudet