[PATCH net] nexthop: account nexthop allocations to memcg

[PATCH net] nexthop: account nexthop allocations to memcg

[Yizhou Zhao]

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.

A QEMU reproduction with Docker confirmed the problem. A container
granted CAP_NET_ADMIN and limited to memory.max=128M could repeatedly
create resilient nexthops while memory.current stayed at 589824 and host
VmallocUsed rose from 67044 kB to 231860 kB.

Fix this by charging the nexthop creation-path allocations to the
caller's memcg with GFP_KERNEL_ACCOUNT / __GFP_ACCOUNT, including the
per-group stats allocation added later. With this change, the same
workload hits memcg OOM with GFP_KERNEL_ACCOUNT in the allocation path
and host VmallocUsed stays flat at 67044 kB, so memory.max constrains
the workload again.

Fixes: ab84be7e54fc ("net: Initial nexthop code")
Fixes: f4676ea74b85 ("net: nexthop: Add nexthop group entry stats")
Cc: stable@vger.kernel.org
Reported-by: Yizhou Zhao <zhaoyz24@mails.tsinghua.edu.cn>
Reported-by: Yuxiang Yang <yangyx22@mails.tsinghua.edu.cn>
Reported-by: Ao Wang <wangao@seu.edu.cn>
Reported-by: Xuewei Feng <fengxw06@126.com>
Reported-by: Qi Li <qli01@tsinghua.edu.cn>
Reported-by: Ke Xu <xuke@tsinghua.edu.cn>
Assisted-by: Claude-Code:GLM-5.2
Signed-off-by: Yizhou Zhao <zhaoyz24@mails.tsinghua.edu.cn>
---
diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 44fe75004cac..c8685c583579 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -169,7 +169,7 @@ static int nh_notifier_res_table_info_init(struct nh_notifier_info *info,
 	info->type = NH_NOTIFIER_INFO_TYPE_RES_TABLE;
 	size = struct_size(info->nh_res_table, nhs, num_nh_buckets);
 	info->nh_res_table = __vmalloc(size, GFP_KERNEL | __GFP_ZERO |
-				       __GFP_NOWARN);
+				       __GFP_NOWARN | __GFP_ACCOUNT);
 	if (!info->nh_res_table)
 		return -ENOMEM;
 
@@ -533,7 +533,7 @@ static struct nexthop *nexthop_alloc(void)
 {
 	struct nexthop *nh;
 
-	nh = kzalloc_obj(struct nexthop);
+	nh = kzalloc_obj(struct nexthop, GFP_KERNEL_ACCOUNT);
 	if (nh) {
 		INIT_LIST_HEAD(&nh->fi_list);
 		INIT_LIST_HEAD(&nh->f6i_list);
@@ -548,7 +548,7 @@ static struct nh_group *nexthop_grp_alloc(u16 num_nh)
 {
 	struct nh_group *nhg;
 
-	nhg = kzalloc_flex(*nhg, nh_entries, num_nh);
+	nhg = kzalloc_flex(*nhg, nh_entries, num_nh, GFP_KERNEL_ACCOUNT);
 	if (nhg)
 		nhg->num_nh = num_nh;
 
@@ -565,7 +565,8 @@ nexthop_res_table_alloc(struct net *net, u32 nhg_id, struct nh_config *cfg)
 	unsigned long size;
 
 	size = struct_size(res_table, nh_buckets, num_nh_buckets);
-	res_table = __vmalloc(size, GFP_KERNEL | __GFP_ZERO | __GFP_NOWARN);
+	res_table = __vmalloc(size, GFP_KERNEL | __GFP_ZERO | __GFP_NOWARN |
+			      __GFP_ACCOUNT);
 	if (!res_table)
 		return NULL;
 
@@ -2799,7 +2800,8 @@ static struct nexthop *nexthop_create_group(struct net *net,
 			nhg->has_v4 = true;
 
 		nhg->nh_entries[i].stats =
-			netdev_alloc_pcpu_stats(struct nh_grp_entry_stats);
+			__netdev_alloc_pcpu_stats(struct nh_grp_entry_stats,
+						  GFP_KERNEL_ACCOUNT);
 		if (!nhg->nh_entries[i].stats) {
 			err = -ENOMEM;
 			nexthop_put(nhe);
@@ -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);
 	if (!nhi) {
 		kfree(nh);
 		return ERR_PTR(-ENOMEM);

--
2.47.3

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@mails.tsinghua.edu.cn>

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

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 1014ad1eb712722973916d53a1c5348624a80550
Author: Yizhou Zhao <zhaoyz24@mails.tsinghua.edu.cn>

nexthop: account nexthop allocations to memcg

This adds GFP_KERNEL_ACCOUNT / __GFP_ACCOUNT to the nexthop creation path
allocations (struct nexthop, struct nh_info, group storage, resilient
bucket table, per-group stats) and to the transient resilient-table
notifier buffer in nh_notifier_res_table_info_init().  The stated goal is
to make memory.max constrain nexthop creation by a CAP_NET_ADMIN task in
a container.

A couple of questions below about the notifier-table hunk and about the
tags.

>  nexthop: account nexthop allocations to memcg
>
>  Fixes: ab84be7e54fc ("net: Initial nexthop code")
>  Fixes: f4676ea74b85 ("net: nexthop: Add nexthop group entry stats")
>  Cc: stable@vger.kernel.org

[Severity: Low]
Are the Fixes: tags and the stable Cc appropriate here?  Switching
allocations to GFP_KERNEL_ACCOUNT does not fix an incorrect result, it
changes the success/failure profile of an existing uAPI operation.

After a stable backport, RTM_NEWNEXTHOP can start returning -ENOMEM
inside a memory.max-constrained cgroup on kernels where it previously
succeeded, which looks like a functional change for routing daemons
running in containers.

The operation still requires CAP_NET_ADMIN in the netns owner's user
namespace, so no privilege boundary is being closed - only accounting
accuracy changes.  Would net-next without the Fixes:/Cc: stable tags be a
better target, with the transient notifier hunk split out?

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

[Severity: Low]
The changelog says "transient notifier tables" in the plural, but only
one of the five transient notifier allocations in net/ipv4/nexthop.c is
converted.  These remain on plain GFP_KERNEL:

  nh_notifier_single_info_init()
	info->nh = kzalloc_obj(*info->nh);

  nh_notifier_mpath_info_init()
	info->nh_grp = kzalloc_flex(*info->nh_grp, nh_entries, num_nh);

  nh_notifier_res_bucket_info_init()
	info->nh_res_bucket = kzalloc_obj(*info->nh_res_bucket);

  nh_notifier_grp_hw_stats_init()
	info->nh_grp_hw_stats = kzalloc_flex(*info->nh_grp_hw_stats, stats,
					     nhg->num_nh);

Since nh_notifier_grp_info_init() dispatches to
nh_notifier_mpath_info_init() for hash_threshold (MPATH) groups, a group
created through the exact path described in the changelog gets no
notifier-table accounting at all, and call_nexthop_res_table_notifiers()
also builds an unaccounted num_nh-sized mpath table during resilient
group replace.

Could the policy be made all-or-nothing here, and the changelog adjusted
to match what the code does?

> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> index 44fe75004cac..c8685c583579 100644
> --- a/net/ipv4/nexthop.c
> +++ b/net/ipv4/nexthop.c
> @@ -169,7 +169,7 @@ static int nh_notifier_res_table_info_init(struct nh_notifier_info *info,
>  	info->type = NH_NOTIFIER_INFO_TYPE_RES_TABLE;
>  	size = struct_size(info->nh_res_table, nhs, num_nh_buckets);
>  	info->nh_res_table = __vmalloc(size, GFP_KERNEL | __GFP_ZERO |
> -				       __GFP_NOWARN);
> +				       __GFP_NOWARN | __GFP_ACCOUNT);
>  	if (!info->nh_res_table)
>  		return -ENOMEM;

[Severity: Medium]
Should this particular allocation be accounted at all?  It is not on the
nexthop creation path the changelog describes.  The buffer only exists to
hand resilient-group state to notifier listeners and is released before
the notifier call returns:

net/ipv4/nexthop.c:nh_notifier_grp_info_fini() {
	if (nhg->hash_threshold)
		kfree(info->nh_grp);
	else if (nhg->resilient)
		vfree(info->nh_res_table);
}

So it cannot accumulate, and charging it does not add containment.  Two
side effects come with it though.

First, whose memcg gets charged?  The notifier paths run in the context
of whoever triggers the notification, not the creator of the nexthop:

  remove_nexthop() -> call_nexthop_notifiers(NEXTHOP_EVENT_DEL)
	(also reached via nh_netdev_event -> nexthop_flush_dev)
  replace_nexthop_single() err_notify -> call_nexthop_notifiers(REPLACE)
  remove_nh_grp_entry() -> call_nexthop_notifiers(REPLACE)
  register_nexthop_notifier() / __unregister_nexthop_notifier()
	-> nexthops_dump() -> nh_notifier_info_init() for every
	   existing nexthop

Can a cgroup end up charged here for nexthops belonging to other
cgroups, for example when a driver module registers a nexthop notifier
and the replay loop walks every resilient nexthop in the netns?

Second, does this make notifier delivery fail on demand?  __GFP_ACCOUNT
is honoured for vmalloc pages, so with a memcg at memory.max this
__vmalloc() can now fail while the system is globally healthy, and
__GFP_NOWARN keeps it quiet.  The failure happens before the chain is
called:

net/ipv4/nexthop.c:call_nexthop_notifiers() {
	err = nh_notifier_info_init(&info, nh);
	if (err) {
		NL_SET_ERR_MSG(extack, "Failed to initialize nexthop notifier info");
		return err;
	}

and the callers do not stop on that error:

net/ipv4/nexthop.c:remove_nexthop() {
	call_nexthop_notifiers(net, NEXTHOP_EVENT_DEL, nh, NULL);

	/* remove from the tree */
	rb_erase(&nh->rb_node, &net->nexthop.rb_root);

The return value is discarded and the nexthop is erased and freed
anyway.  replace_nexthop_single()'s err_notify path also discards it,
remove_nh_grp_entry() only pr_err()s, and __unregister_nexthop_notifier()
ignores nexthops_dump()'s status while nexthops_dump() breaks out at the
first error, skipping DEL for all remaining nexthops.

The fail-silent error handling in those callers is pre-existing and not
introduced by this patch, but does this change turn it into something a
task can trigger at will from inside a constrained cgroup?  With an
offload listener such as mlxsw or netdevsim registered, that would leave
driver state for nexthops the kernel has already freed, and would let
register_nexthop_notifier() be made to fail for the whole netns.

Would keeping nh_notifier_res_table_info_init() on plain GFP_KERNEL and
accounting only the persistent objects (struct nexthop, struct nh_info,
struct nh_group, res_table, per-group stats) avoid all of this?

[ ... ]
-- 
pw-bot: cr