Re: [PATCH net v2] net/packet: account user-triggered allocations to memcg

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] The patch says it charges the per-socket memory a packet
  socket allocates for its user.

--

net/packet: account user-triggered allocations to memcg

This patch changes the allocations a packet socket makes for its user to
GFP_KERNEL_ACCOUNT, so the caller's memory cgroup is charged for them.
These are the ring blocks and block arrays, the fanout groups and rollover
state, the mclist entries and the per-CPU pending counter. The vmalloc
fallback for ring blocks becomes __vmalloc() with __GFP_ZERO.

> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 823e0911c910f..06386db4c8e8c 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1233,7 +1233,8 @@ static int packet_alloc_pending(struct packet_sock *po)
>  {
>  	po->rx_ring.pending_refcnt = NULL;
>  
> -	po->tx_ring.pending_refcnt = alloc_percpu(unsigned int);
> +	po->tx_ring.pending_refcnt = alloc_percpu_gfp(unsigned int,
> +						      GFP_KERNEL_ACCOUNT);
>  	if (unlikely(po->tx_ring.pending_refcnt == NULL))
>  		return -ENOBUFS;
>  

[Severity: Medium]

The pending counter on the socket() path is now charged. Is struct
packet_sock, allocated on the same path, still left uncharged?

The commit message says:

    struct packet_sock itself comes from sk_alloc(), and the
    netdev_hw_addr entries that PACKET_MR_MULTICAST and PACKET_MR_UNICAST
    add through dev_mc_add() and dev_uc_add() live in net/core; neither
    is changed here.

The v2 note also says the patch covers "every user-triggered allocation in
net/packet".

>From the code, af_packet itself seems to cause the missing charge, not
net/core. packet_init() registers the proto without a slab:

net/packet/af_packet.c:packet_init() {
    ...
	rc = proto_register(&packet_proto, 0);
    ...
}

So proto_register() never creates the SLAB_ACCOUNT kmem_cache for
packet_proto. packet_create() then passes plain GFP_KERNEL:

	sk = sk_alloc(net, PF_PACKET, GFP_KERNEL, &packet_proto, kern);

Because prot->slab is NULL, sk_prot_alloc() falls back to a plain kmalloc
with those flags:

net/core/sock.c:sk_prot_alloc() {
    ...
	} else
		sk = kmalloc(prot->obj_size, priority);
    ...
}

mem_cgroup_sk_alloc() runs afterwards. It only sets sk->sk_memcg and does
not charge the object itself.

packet_sock holds a struct sock, two packet_ring_buffer structs and a
cacheline-aligned prot_hook. That probably makes it the largest
per-socket allocation here.

A user with CAP_NET_RAW in its own user namespace can create one per fd.
The total is limited only by RLIMIT_NOFILE and process limits, not by
memory.max.

Would either proto_register(&packet_proto, 1) or GFP_KERNEL_ACCOUNT in the
sk_alloc() call in packet_create() close this gap? If packet_sock is left
out on purpose, could the commit message give the actual reason instead of
pointing to sk_alloc()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261030100504.23498-1-ahmadag1001%40gmail.com