Re: [PATCH net 0/2] wireguard: memcg accounting and initiation race fix

Re: [PATCH net 0/2] wireguard: memcg accounting and initiation race fix

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net 1/2] wireguard: account peer and allowedips allocations to memcg

[netdev-bot+sashiko]

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

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

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).

- [High] Incomplete fix: every peer still creates uncharged memory and a
  PID.
- [Medium] Incomplete fix: per-device allocations in wg_newlink() are
  still not charged.
- [Medium] The commit leaves noise_keypair allocations uncharged, saying
  they 'happen from the handshake worker and are bounded to three per…

--

wireguard: account peer and allowedips allocations to memcg

This patch creates the wg_peer and allowedips_node slab caches with
SLAB_ACCOUNT. It also switches the per-peer dst_cache and the per-device
pubkey and index hashtables to GFP_KERNEL_ACCOUNT. The goal is to stop a
user with a user and network namespace from escaping its memory cgroup
limit by adding peers over WG_CMD_SET_DEVICE.

> The keypair allocations are left alone, as they happen from the handshake
> worker and are bounded to three per peer.

[Severity: Medium]
Does the per-peer bound make the keypair memory small enough to ignore?

struct noise_keypair embeds a noise_replay_counter with:

    unsigned long backtrack[COUNTER_BITS_TOTAL / BITS_PER_LONG];

COUNTER_BITS_TOTAL is 8192, so that is a 1KB bitmap. The struct ends up
around 1.2KB, which means keypair_create() is served from kmalloc-2k:

drivers/net/wireguard/noise.c:keypair_create() {
	struct noise_keypair *keypair = kzalloc_obj(*keypair);
    ...
}

add_new_keypair() keeps two keypairs in slots per peer, plus any that are
in flight or waiting on RCU. That is about 4KB or more of uncharged memory
per peer. The now-charged wg_peer object plus the per-CPU dst_cache come
to about 2KB.

The configuring user picks every peer public key, so it knows the matching
private keys. A UDP program in the same netns can send valid handshake
initiations for every peer to the listen port. The responder path then
allocates keypairs from the kworker:

wg_packet_handshake_receive_worker()
  wg_packet_send_handshake_response()
    wg_noise_handshake_begin_session()
      keypair_create()

Once the peer count is limited only by charged memory, could the
uncharged keypair memory still reach about 1-2x memory.max?

This runs in a worker, so GFP_KERNEL_ACCOUNT alone would charge root.
Would this need the configuring memcg to be recorded, with
set_active_memcg() used in the handshake path?

> diff --git a/drivers/net/wireguard/peer.c b/drivers/net/wireguard/peer.c
> index 1cb502a932e07..05b80521c7061 100644
> --- a/drivers/net/wireguard/peer.c
> +++ b/drivers/net/wireguard/peer.c
> @@ -33,7 +33,7 @@ struct wg_peer *wg_peer_create(struct wg_device *wg,
>  	peer = kmem_cache_zalloc(peer_cache, GFP_KERNEL);
>  	if (unlikely(!peer))
>  		return ERR_PTR(ret);
> -	if (unlikely(dst_cache_init(&peer->endpoint_cache, GFP_KERNEL)))
> +	if (unlikely(dst_cache_init(&peer->endpoint_cache, GFP_KERNEL_ACCOUNT)))
>  		goto err;
>  
>  	peer->device = wg;

[Severity: High]
The peer slab and dst_cache are now charged. Does each peer still pin a
larger amount of uncharged memory through its NAPI kthread?

wg_newlink() turns on threaded NAPI for every wireguard device:

	netif_threaded_enable(dev);
	ret = register_netdevice(dev);

Later in wg_peer_create(), each peer does:

	netif_napi_add(wg->dev, &peer->napi, wg_packet_rx_poll);

Peers have no napi->config, so napi_get_threaded_config() returns
dev->threaded. That spawns one kthread per peer:

wg_peer_create()
  netif_napi_add()
    netif_napi_add_weight_locked()
      napi_kthread_create()
        kthread_run(napi_threaded_poll, n, "napi/%s-%d", ...)

kthreadd forks the thread. Its task_struct and kernel stack (about
20-30KB) are therefore charged to the root memcg. The PID is also taken
outside the caller's pids cgroup and RLIMIT_NPROC.

Each peer charges only about 2KB for wg_peer plus dst_cache. Could a
cgroup with memory.max=64M still create tens of thousands of peers and pin
hundreds of MiB of uncharged memory? With a larger limit, could this also
use up pid_max for the whole system?

More GFP_KERNEL_ACCOUNT flags would not cover this, because kthreadd does
the allocation. Would this need a per-device or per-user cap on peers or
NAPI threads, or a design that does not spawn a kthread per peer?

The next patch in the series ("wireguard: noise: recheck replay and flood
under write lock on initiation") only changes the replay checks in
noise.c and does not affect this.

[ ... ]

> diff --git a/drivers/net/wireguard/peerlookup.c b/drivers/net/wireguard/peerlookup.c
> index 99651a86ac953..f333e906014ab 100644
> --- a/drivers/net/wireguard/peerlookup.c
> +++ b/drivers/net/wireguard/peerlookup.c

[ ... ]

> @@ -82,7 +82,7 @@ static struct hlist_head *index_bucket(struct index_hashtable *table,
>  
>  struct index_hashtable *wg_index_hashtable_alloc(void)
>  {
> -	struct index_hashtable *table = kvmalloc_obj(*table);
> +	struct index_hashtable *table = kvmalloc_obj(*table, GFP_KERNEL_ACCOUNT);
>  
>  	if (!table)
>  		return NULL;

[Severity: Medium]
The two per-device hashtables are now charged, and the commit message
says the number of devices has no limit. Are the other per-device
allocations in wg_newlink() still uncharged?

wg_newlink() calls wg_packet_queue_init() three times, and each call
does:

drivers/net/wireguard/queueing.c:wg_packet_queue_init() {
    ...
	ret = ptr_ring_init(&queue->ring, len, GFP_KERNEL);
    ...
	queue->worker = wg_packet_percpu_multicore_worker_alloc(function, queue);
    ...
}

ptr_ring_init() ends up in kvmalloc_array() with GFP_KERNEL. On 64-bit
that is 8KB + 8KB + 32KB per device (MAX_QUEUED_PACKETS and
MAX_QUEUED_INCOMING_HANDSHAKES).

The per-CPU worker arrays come from alloc_percpu() without
__GFP_ACCOUNT:

	struct multicore_worker __percpu *worker = alloc_percpu(struct multicore_worker);

wg_newlink() also creates three workqueues. packet_crypt_wq uses
WQ_MEM_RECLAIM, which adds a rescuer kthread forked by kthreadd:

	wg->packet_crypt_wq = alloc_workqueue("wg-crypt-%s",
			WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM | WQ_PERCPU, 0,
			dev->name);

Could a user with user and net namespaces call RTM_NEWLINK in a loop to
create wireguard devices? That would still allocate per-device memory
that never shows up in memory.current.

Should the ptr_ring and per-CPU worker allocations be charged as well?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105141006.95083-1-alex%40badcode.im

Re: [PATCH net 1/2] wireguard: account peer and allowedips allocations to memcg

[Jason A. Donenfeld]

On Thu, Nov 05, 2026 at 05:10:05PM +0300, Alexander Ildiyarov wrote:
> Peers, allowed IP trie nodes, the per-peer dst_cache and the per-device
> hashtables are all allocated in the context of the process that
> configures the device over netlink, but none of these allocations are
> charged to that process's memory cgroup: the wg_peer and allowedips_node
> caches are created without SLAB_ACCOUNT, and the hashtables and
> dst_cache use plain GFP_KERNEL.
> 
> The amount of memory is bounded only by MAX_PEERS_PER_DEVICE (2^20) per
> device, with no limit on the number of devices or on the number of
> allowed IPs per peer. Since WG_CMD_SET_DEVICE uses GENL_UNS_ADMIN_PERM,
> any user who can create a user and network namespace can do this, and
> thereby escape the memory limits of the cgroup it runs in.
> 
> For example, a process in a cgroup with memory.max=64M that adds 100000
> peers with 20 /128 allowed IPs each grows Slab by ~790 MiB, yet the
> cgroup's memory.current stays below 1 MiB and its memory limit is never
> hit.
> 
> Fix this by creating the two caches with SLAB_ACCOUNT and using
> GFP_KERNEL_ACCOUNT for the hashtables and the dst_cache. The keypair
> allocations are left alone, as they happen from the handshake worker and
> are bounded to three per peer.
 
I wasn't aware of this trick, I think mostly because most drivers don't
use this. Does any other part of the networking stack do this? Is this a
normal thing?

Re: [PATCH net 1/2] wireguard: account peer and allowedips allocations to memcg

[Jason A. Donenfeld]

Okay, I guess I was just looking for some judgement discussion, not
more output from the LLM. I'll look into this myself on the slow track
for a little bit.