Re: [PATCH v3 2/2] selftests: net: packetdrill: add test for negative sk_forward_alloc in tcp_wmem_schedule()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 3 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] When cgroup v2 or a usable memory controller is missing,
  tcp_wmem_schedule_negative_fwd_alloc.pkt reports FAIL instead of SKIP.
- [Medium] Step 4 only gets EAGAIN if the per-CPU memcg_stock holds no
  leftover charge for pkt_${PPID} when write(4, ..., 16384) charges…
- [Medium] Between the init command (line 23, `echo $PPID >
  pkt_${PPID}/cgroup.procs`) and the move-back command (lines 38-41),…
- [Low] Test teardown does not mirror setup.

--

selftests: net: packetdrill: add test for negative sk_forward_alloc in tcp_wmem_schedule()

This adds a packetdrill test that pushes sk->sk_forward_alloc negative
through tcp_collapse() while sk_wmem_queued is above sysctl_tcp_wmem[0].
It then checks that a non-blocking write() under memcg pressure fails
with EAGAIN rather than EFAULT. It also adds CONFIG_CGROUPS and
CONFIG_MEMCG to the packetdrill config, and a PAGE_SIZE based skip to
ksft_runner.sh.

> diff --git a/tools/testing/selftests/net/packetdrill/tcp_wmem_schedule_negative_fwd_alloc.pkt b/tools/testing/selftests/net/packetdrill/tcp_wmem_schedule_negative_fwd_alloc.pkt
> new file mode 100644
> index 0000000000000..f8b7386fe77e2
> --- /dev/null
> +++ b/tools/testing/selftests/net/packetdrill/tcp_wmem_schedule_negative_fwd_alloc.pkt
> @@ -0,0 +1,108 @@

[ ... ]

> +`set -e
> +./defaults.sh
> +ip link set dev tun0 mtu 65535
> +sysctl -q net.ipv4.tcp_rmem="4096 131072 6291456"
> +sysctl -q net.ipv4.tcp_wmem="4096 16384 4194304"
> +cgroot=$(mount -t cgroup2 | head -1 | awk '{print $3}')
> +[ -n "$cgroot" ]
> +echo +memory > "${cgroot}/cgroup.subtree_control"

[Severity: Medium]
What happens with this init command on a system where cgroup2 is not
mounted, or where the memory controller can't be enabled here? Because of
set -e, the init command aborts if any of these steps fails:

  - no cgroup2 mount, so [ -n "$cgroot" ] fails
  - memory is bound to cgroup v1 in a hybrid setup, CONFIG_MEMCG=n, or
    cgroup_disable=memory, so the cgroup.subtree_control write fails
  - the first cgroup2 mount is a non-root cgroup that holds tasks, which
    is common in containers, so enabling +memory fails with EBUSY

packetdrill then exits non-zero, and ksft_runner.sh counts that as a
failure:

	unshare -n packetdrill ${ip_args[$ip_version]} ${optargs[@]} $script > /dev/null \
		&& ktap_test_pass $ip_version || $failfunc $ip_version

The result is three failures (ipv4, ipv6, ipv4-mapped-ipv6) instead of a
skip. The only new prerequisite check in ksft_runner.sh is for PAGE_SIZE.

Could a matching skip be added for a missing cgroup2 memory controller?

> +rmdir "${cgroot}/pkt_${PPID}" 2>/dev/null || true
> +mkdir "${cgroot}/pkt_${PPID}"
> +echo $PPID > "${cgroot}/pkt_${PPID}/cgroup.procs"
> +`

[ ... ]

> +// Move packetdrill back to its original cgroup so subsequent packetdrill allocations are not charged to pkt_${PPID}
> ++0.000 `cgroot=$(mount -t cgroup2 | head -1 | awk '{print $3}')
> +runner_pid=$(awk '/^PPid:/ {print $2}' /proc/$PPID/status)
> +orig_cg=$(sed -n 's/^0:://p' /proc/$runner_pid/cgroup)
> +echo $PPID > "${cgroot}${orig_cg}/cgroup.procs"`

[Severity: Medium]
Does moving packetdrill back also move the memory charged while it ran in
pkt_${PPID}?

Between the init command's cgroup.procs write and this move-back, any
anonymous pages packetdrill touches for the first time (heap growth for
injected or sniffed packets, or stack) are charged to pkt_${PPID}. Cgroup
v2 does not move charges when a task migrates, so those pages stay
charged to pkt_${PPID}.

In step 4 the memory.high write is a blocking echo, so memory_high_write()
keeps calling:

		reclaimed = try_to_free_mem_cgroup_pages(memcg, nr_pages - high,
					GFP_KERNEL, MEMCG_RECLAIM_MAY_SWAP, NULL);

Socket memory can't be reclaimed, so the memcg never gets under high=0,
and reclaim continues until it stops making progress. If swap or zswap is
available, packetdrill's leftover anon pages can be swapped out at this
point.

memory.max is then set to 0. When packetdrill touches one of those pages
again, mem_cgroup_swapin_charge_folio() charges the memcg recorded in the
swap entry, as long as that memcg is still online:

	memcg = mem_cgroup_from_private_id(id);
	if (!memcg || !css_tryget_online(&memcg->css))
		memcg = get_mem_cgroup_from_mm(mm);

pkt_${PPID} stays online until the final rmdir. try_charge_memcg() can't
reclaim the socket memory, and the memcg OOM killer finds no tasks in
pkt_${PPID}. The fault would return VM_FAULT_OOM and be retried forever.

Can packetdrill livelock here? If the kselftest timeout then kills the
runner, the packetdrill child could be left spinning. Its cleanup command
would never run, and pkt_${PPID} would be left behind.

This depends on packetdrill touching new anonymous pages during that
window, and on swap or zswap being present.

[ ... ]

> +// 4. Drain per-CPU memcg stock via memory.high=0, cap memory.max=0 (O_NONBLOCK
> +//    avoids synchronous reclaim/OOM), and expect -EAGAIN on a 16 KB non-blocking
> +//    write. Because copy (16 KB) minus sk_forward_alloc (<= -16.4 KB) exceeds
> +//    PAGE_ALLOC_COSTLY_ORDER (32 KB), try_charge_memcg() fails without invoking
> +//    the memcg OOM killer.
> ++0.010 `cgroot=$(mount -t cgroup2 | head -1 | awk '{print $3}')
> +echo 0 > "${cgroot}/pkt_${PPID}/memory.high"
> +printf 0 | dd of="${cgroot}/pkt_${PPID}/memory.max" oflag=nonblock status=none`
> ++0.000 fcntl(4, F_SETFL, O_RDWR|O_NONBLOCK) = 0
> ++0.000 write(4, ..., 16384) = -1 EAGAIN (Resource temporarily unavailable)

[Severity: Medium]
Can this write() succeed because of leftover per-CPU memcg stock?

The comment relies on memory.high=0 to drain the stock, but
drain_all_stock() is best-effort. It returns right away if another drain
is running anywhere on the system:

mm/memcontrol.c:drain_all_stock() {
    ...
	/* If someone's already draining, avoid adding running more workers. */
	if (!mutex_trylock(&percpu_charge_mutex))
		return;
    ...
}

For remote CPUs it only queues the drain work and does not wait for it.

The memory.max write uses oflag=nonblock, so memory_max_write() takes the
early exit and never drains:

	if (of->file->f_flags & O_NONBLOCK)
		goto out;

The receive-side charges from step 3 go through try_charge_memcg() with a
MEMCG_CHARGE_BATCH batch, and refill_stock() leaves the surplus on the
charging CPU.

When write() charges memory through
tcp_wmem_schedule()->sk_wmem_schedule()->__sk_mem_raise_allocated()->
mem_cgroup_sk_charge()->try_charge_memcg(), the stock is consumed before
memory.max is checked:

retry:
	if (consume_stock(memcg, nr_pages))
		return 0;

consume_stock() accepts any nr_pages up to MEMCG_CHARGE_BATCH, and the
charge here is about 9 pages.

If the stock was not fully drained, the charge would succeed even with
memory.max=0, and write() would return 16384 instead of -1 EAGAIN. Could
this make the test flaky when other memcg activity is running on the
system?

> +
> ++0.000 close(4) = 0
> ++0.000 close(3) = 0
> +
> +// Untimed cleanup command executed by packetdrill on both pass and early failure.
> +`cgroot=$(mount -t cgroup2 | head -1 | awk '{print $3}')
> +[ -n "$cgroot" ] || exit 0
> +runner_pid=$(awk '/^PPid:/ {print $2}' /proc/$PPID/status)
> +orig_cg=$(sed -n 's/^0:://p' /proc/$runner_pid/cgroup)
> +echo $PPID > "${cgroot}${orig_cg}/cgroup.procs" 2>/dev/null || true
> +echo $$ > "${cgroot}${orig_cg}/cgroup.procs" 2>/dev/null || true
> +rmdir "${cgroot}/pkt_${PPID}"`

[Severity: Low]
The init command writes +memory to the root cgroup.subtree_control, but
this cleanup never restores the previous state. Is it intended that this
system-wide change stays in place after the test, whether it passes or
fails?

pkt_${PPID} is also removed only by this cleanup command, which never runs
if packetdrill is killed from outside, for example by the kselftest
timeout. The rmdir at the start of the init command only removes a
directory left by the same PID:

	rmdir "${cgroot}/pkt_${PPID}" 2>/dev/null || true

ksft_runner.sh starts a new unshare -n packetdrill for each of the three
IP versions, and each one gets a new PID. Can stale pkt_* cgroups pile up
across killed runs?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021185447.2914274-1-tavip%40google.com