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

[Yizhou Zhao]

Hi Jakub,

Thank you for both reviews. This is the reply for the second review.

> On Jul 30, 2026, at 08:39, Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
> 
> 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@xxxxxxxxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx
> 
> [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?

I agree that the fix changes the point at which RTM_NEWNEXTHOP can fail
under memory.max. However, I view that failure as the intended enforcement
of memory.max: the current uncharged allocations let a cgroup retain kernel
memory outside its configured hard limit.

For that reason, I had considered this a bug fix suitable for net, and
possibly stable once the patches are properly split and validated. Do you
prefer net-next because of the breadth and regression risk of the allocation
changes, rather than the -ENOMEM behavior itself? I will follow your
guidance on the target and tags.

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

Agreed. The partial notifier conversion and the corresponding changelog
wording are both incorrect.

After reviewing the lifetime and call contexts, I will make the policy here
"none": I will drop the notifier hunk rather than convert the remaining
notifier allocations or split it into a separate patch. These buffers are
temporary notification payloads, freed after the callback, rather than
persistent nexthop state. They can also be allocated during notifier replay
or deletion, where GFP_KERNEL_ACCOUNT would charge the current context
rather than the nexthop creator and could make notification delivery fail.

I will remove "transient notifier tables" from the commit message. The
revised patch will describe and account only persistent nexthop allocations.

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

I agree that this allocation should remain on plain GFP_KERNEL.

__GFP_ACCOUNT would charge the vmalloc backing pages to the current memcg,
not to the cgroup which created the resilient nexthop. Therefore notifier
replay or deletion could charge an unrelated cgroup while walking existing
nexthops in the netns.

It could also make nh_notifier_info_init() fail before the notifier chain is
called when the current cgroup is at memory.max. Since some callers discard
that error and continue, this could skip notification for an object which is
then removed or otherwise changed.

I will remove this hunk rather than account the other notifier allocations,
and remove the notifier-table wording from the changelog. The revised patch
will account only persistent nexthop state, for which the allocation context
and lifetime are directly attributable to the nexthop operation.

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

Please let me know if further changes are needed.

Thanks,
Yizhou