Re: [PATCH net v4] 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 only partly closes the PACKET_ADD_MEMBERSHIP memcg
  bypass.

--

net/packet: account user-triggered allocations to memcg

This commit changes several AF_PACKET allocations whose size or count is
chosen by user space to use GFP_KERNEL_ACCOUNT, so they are charged to the
caller's memory cgroup. It covers the ring blocks and block array, the
rx_owner_map, the fanout group, and the packet_mclist entries created by
PACKET_ADD_MEMBERSHIP.

> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index cf6429f094772..ec6ab96c4f87c 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c

[ ... ]

> @@ -3773,7 +3774,7 @@ static int packet_mc_add(struct sock *sk, struct packet_mreq_max *mreq)
>  		goto done;
>  
>  	err = -ENOBUFS;
> -	i = kmalloc_obj(*i);
> +	i = kmalloc_obj(*i, GFP_KERNEL_ACCOUNT);
>  	if (i == NULL)
>  		goto done;
>  

[Severity: Medium]
Does this fully close the PACKET_ADD_MEMBERSHIP bypass described in the
commit message?

For PACKET_MR_MULTICAST and PACKET_MR_UNICAST, packet_mc_add() then calls
packet_dev_mc(), which adds the address to the device list:

packet_mc_add()
  packet_dev_mc()
    dev_mc_add() / dev_uc_add()
      __hw_addr_add_ex()
        __hw_addr_create()

Each distinct address gets a struct netdev_hw_addr, and that allocation
is not charged:

net/core/dev_addr_lists.c:__hw_addr_create() {
    ...
	ha = kmalloc(alloc_size, GFP_ATOMIC);
    ...
}

A struct netdev_hw_addr is about 104 bytes (kmalloc-128). That is larger
than the packet_mclist that is now charged (about 72 bytes,
kmalloc-cg-96). Neither packet_mc_add() nor dev_addr_lists.c limits how
many entries can be added.

On devices with ndo_set_rx_mode_async, netif_addr_lists_snapshot() ->
__hw_addr_list_snapshot() also makes two more GFP_ATOMIC copies of each
uc/mc entry. These copies stay in dev->rx_mode_addr_cache, which is only
flushed in free_netdev().

So a CAP_NET_RAW user in a user namespace can add many distinct
MR_MULTICAST or MR_UNICAST addresses on lo. Each one still pins more
uncharged memory than gets charged.

Would it make sense to also add __GFP_ACCOUNT to the allocation in
__hw_addr_add_ex(), which runs in process context?

The commit message says:

    100 sockets with 2000 PACKET_MR_PROMISC entries each
    on lo (45220 KB used, 124 KB charged) behave the same. With this change
    all three are stopped by the cgroup's OOM killer.

PACKET_MR_PROMISC only calls dev_set_promiscuity() and never reaches
__hw_addr_create(), so the reproducer does not exercise this path.
Should the commit message be narrowed, or should the device address
entries be covered as well?

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