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

[Jakub Kicinski <kuba@kernel.org>]

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