Re: [PATCH net] nexthop: account nexthop allocations to memcg

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [1]: [PATCH net] nexthop: account nexthop allocations to memcg ---
commit 186a5b6499e65039b5037906ec518baac2833b1d
Author: Yizhou Zhao <zhaoyz24@xxxxxxxxxxxxxxxxxxxxx>

    nexthop: account nexthop allocations to memcg
    
    The nexthop creation path allocates struct nexthop, struct nh_info,
    group storage, resilient bucket tables, transient notifier tables and
    per-group stats without memcg accounting. As a result, a task with
    CAP_NET_ADMIN can create nexthops and resilient groups whose kernel
    memory is not charged to the task's memory cgroup, so memory.max does
    not constrain this part of the workload.

> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> index 44fe75004cacb..c8685c5835794 100644
> --- a/net/ipv4/nexthop.c
> +++ b/net/ipv4/nexthop.c
[ ... ]
> @@ -2943,7 +2945,7 @@ static struct nexthop *nexthop_create(struct net *net, struct nh_config *cfg,
>  	if (!nh)
>  		return ERR_PTR(-ENOMEM);
>  
> -	nhi = kzalloc_obj(*nhi);
> +	nhi = kzalloc_obj(*nhi, GFP_KERNEL_ACCOUNT);

[Severity: High]
This is a pre-existing issue, but does this patch miss some per-CPU
allocations in the nexthop creation path?

When creating IPv4 and IPv6 nexthops, nh_create_ipv6() and fib_nh_init()
explicitly pass GFP_KERNEL without __GFP_ACCOUNT down to the initialization
functions:

net/ipv4/nexthop.c:nh_create_ipv6() {
    ...
    err = fib6_nh_init(net, fib6_nh, &fib6_cfg, GFP_KERNEL, extack);
    ...
}

This results in unaccounted per-CPU allocations:

net/ipv6/route.c:fib6_nh_init() {
    ...
    fib6_nh->rt6i_pcpu = alloc_percpu_gfp(struct rt6_info *, gfp_flags);
    ...
}

A similar missing accounting happens for IPv4 in fib_nh_common_init():

net/ipv4/fib_semantics.c:fib_nh_common_init() {
    ...
    nhc->nhc_pcpu_rth_output = alloc_percpu_gfp(struct rtable __rcu *,
                                                gfp_flags);
    ...
}

Could an unprivileged user with CAP_NET_ADMIN still bypass memcg limits by
exhausting memory through these per-CPU allocations?


[Severity: High]
This is a pre-existing issue, but do we also need to account for Lightweight
Tunnel (LWT) state allocations?

During nexthop creation with encapsulation attributes, fib_nh_common_init()
triggers LWT state allocation:

net/ipv4/fib_semantics.c:fib_nh_common_init() {
    ...
    err = lwtunnel_build_state(net, encap_type, encap,
                               nhc->nhc_family, cfg, &lwtstate,
                               extack);
    ...
}

The allocation unconditionally uses GFP_ATOMIC without __GFP_ACCOUNT:

net/core/lwtunnel.c:lwtunnel_state_alloc() {
    ...
    lws = kzalloc(sizeof(*lws) + encap_len, GFP_ATOMIC);
    ...
}

Could a task in a container bypass memory.max limits by creating many nexthops
with encapsulation attributes?

>  	if (!nhi) {
>  		kfree(nh);
>  		return ERR_PTR(-ENOMEM);