Re: [PATCH net] net/sched: account classifier filter 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.
---
net/sched: account classifier filter allocations to memcg

This adds GFP_KERNEL_ACCOUNT to the filter objects, per-CPU counters and
per-filter aux allocations in the tc classifier *_change() paths, plus the
shared exts->actions array in tcf_exts_init_ex(). It also adds an
idr_remove() to the cls_basic percpu-allocation failure path.

> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 4e6a2812a4f32..be63b347640ba 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -3364,7 +3364,8 @@ int tcf_exts_init_ex(struct tcf_exts *exts, struct net *net, int action,
>  	 * This reference might be taken later from tcf_exts_get_net().
>  	 */
>  	exts->net = net;
> -	exts->actions = kzalloc_objs(struct tc_action *, TCA_ACT_MAX_PRIO);
> +	exts->actions = kzalloc_objs(struct tc_action *, TCA_ACT_MAX_PRIO,
> +				     GFP_KERNEL_ACCOUNT);
>  	if (!exts->actions)
>  		return -ENOMEM;
>  #endif
> diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
> index 492cd9ce8d46f..e2a94ba9fba76 100644
> --- a/net/sched/cls_basic.c
> +++ b/net/sched/cls_basic.c
> @@ -193,7 +193,7 @@ static int basic_change(struct net *net, struct sk_buff *in_skb,
>  			return -EINVAL;
>  	}
>  
> -	fnew = kzalloc_obj(*fnew);
> +	fnew = kzalloc_obj(*fnew, GFP_KERNEL_ACCOUNT);
>  	if (!fnew)
>  		return -ENOBUFS;
>  

[Severity: Medium]
The changelog says this "closes" the bypass that lets "unprivileged users
to pin kernel memory outside memcg charging". Is that accurate when the
larger, user-sized allocations on the very same basic_change() request are
still uncharged?

basic_change() charges fnew and fnew->pf here, then reaches the ematch
tree through basic_set_parms():

net/sched/cls_basic.c:basic_set_parms()
    err = tcf_em_tree_validate(tp, tb[TCA_BASIC_EMATCHES], &f->ematches);

net/sched/ematch.c:tcf_em_tree_validate()
    matches_len = tree_hdr->nmatches * sizeof(*em);

    tree->matches = kzalloc(matches_len, GFP_KERNEL);

and per-ematch payloads in tcf_em_validate():

    void *v = kmemdup(data, data_len, GFP_KERNEL);

nmatches and data_len come from the netlink message, so the uncharged part
of a single filter can be much larger than the charged part. The same
tcf_em_tree_validate() path is used by flow_change() and
cls_cgroup_change(), whose filter objects this patch also charges.

Should the u32 hash tables be converted too? u32_change() charges the
knode, but the TCA_U32_DIVISOR branch in the same function still uses the
default gfp for the up-to-257-pointer table that is published in
tp_c->hlist:

net/sched/cls_u32.c:u32_change()
	ht = kzalloc_flex(*ht, ht, divisor + 1);
	if (ht == NULL)
		return -ENOBUFS;

The same object type is allocated as root_ht in u32_init(), also
uncharged.

There are a few other siblings on these paths that keep plain GFP_KERNEL:

- fw_change() allocates head = kzalloc_obj(*head) (fw_head with its
  256-entry hash table) a few lines above the fw_filter allocation this
  patch converts.
- route4_init() allocates route4_head, which carries table[256], while
  route4_filter and route4_bucket in the same file are converted.
- fl_init() allocates the cls_fl_head plus the rhashtable that grows with
  each inserted filter, and fl_tmplt_create() allocates the template
  object; both stay uncharged while fnew and newmask are charged.
- tcf_proto_create(), tcf_chain_create() and tcf_block_create() in
  cls_api.c, and the IDR nodes for the handles being charged
  (idr_alloc_u32(..., GFP_KERNEL) in basic_change() and u32_change()).

With the boundary running through the middle of one netlink request, a
memcg-limited task can still see slab grow faster than memory.current,
which is the signal the changelog uses as the reproducer. Was leaving
these out intentional, or is the intent to convert them in a follow-up?

> @@ -212,9 +212,11 @@ static int basic_change(struct net *net, struct sk_buff *in_skb,
>  	if (err)
>  		goto errout;
>  	fnew->handle = handle;
> -	fnew->pf = alloc_percpu(struct tc_basic_pcnt);
> +	fnew->pf = alloc_percpu_gfp(struct tc_basic_pcnt, GFP_KERNEL_ACCOUNT);
>  	if (!fnew->pf) {
>  		err = -ENOMEM;
> +		if (!fold)
> +			idr_remove(&head->handle_idr, fnew->handle);
>  		goto errout;
>  	}
>  

[ ... ]

> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index c297d7dbcf916..ac6d0fa5a40e3 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -1114,15 +1114,16 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
>  		goto erridr;
>  	}
>  
> -	n = kzalloc_flex(*n, sel.keys, s->nkeys);
> +	n = kzalloc_flex(*n, sel.keys, s->nkeys, GFP_KERNEL_ACCOUNT);
>  	if (n == NULL) {
>  		err = -ENOBUFS;
>  		goto erridr;
>  	}
>  

[ ... ]