Re: [PATCH 4/7] tracing: account ring buffer pages and trace_array metadata to memcg

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] tracing: alloc_cpu_data() mischarges global_trace and
  read-page swaps
- [Medium] tracing: alloc_cpu_data() memcg limit triggers
  trace_rb_cpu_prepare() WARN
- [Low] tracing: inaccurate snapshot size claim and inconsistent
  GFP_KERNEL_ACCOUNT use
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] tracing: memory leak in trace_array_create_systems() [1]
--

commit a11da3138f9fda850c1b05bae82b0be7e738b994
Author: Tao Cui <cuitao@kylinos.cn>

tracing: account ring buffer pages and trace_array metadata to memcg

This commit adds __GFP_ACCOUNT to ring buffer subbuffer allocations in
alloc_cpu_data() and switches trace_array allocations in
trace_array_create_systems() to GFP_KERNEL_ACCOUNT.

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index e760d2fe386fb..f925936378639 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -441,7 +441,13 @@ static struct buffer_data_page *alloc_cpu_data(int cpu, int order)
>  	 * gracefully without invoking oom-killer and the system is not
>  	 * destabilized.
>  	 */
> -	mflags = GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_COMP | __GFP_ZERO;
> +	/*
> +	 * The subbuffers are charged to the memcg of whoever caused the
> +	 * allocation: the creator of a trace instance, or root at boot for
> +	 * the global trace.
> +	 */
> +	mflags = GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_COMP | __GFP_ZERO |
> +		  __GFP_ACCOUNT;
>  
>  	page = alloc_pages_node(cpu_to_node(cpu), mflags, order);
>  	if (!page)

[Severity: Medium]
Does charging every alloc_cpu_data() call to current's memcg misattribute
global_trace subbuffers and swapped read pages?

At boot, tracer_alloc_buffers() keeps global_trace at its minimum size
(ring_buf_size = 1) when ring_buffer_expanded is false.  global_trace is
only expanded to trace_buf_size at runtime in tracing_update_buffers() or
tracing_set_tracer() when a task first enables an event, sets a tracer, or
loads a module using trace_printk().  With __GFP_ACCOUNT in
alloc_cpu_data(), those global_trace pages are charged to whichever cgroup
triggers the lazy expansion and stay pinned to that memcg because
global_trace is never freed.

In addition, ring_buffer_alloc_read_page() calls alloc_cpu_data() when
reading or splicing from trace_pipe_raw, and ring_buffer_read_page() swaps
that reader-allocated page directly into reader->page while handing the old
ring buffer page back to the reader to be freed:

kernel/trace/ring_buffer.c:ring_buffer_read_page() {
    ...
		dpage = reader->page;
		reader->page = data_page->data;
		local_set(&reader->write, 0);
		local_set(&reader->entries, 0);
		reader->read = 0;
		data_page->data = dpage;
    ...
}

A host process reading trace_pipe_raw from a container's instance will
replace the container's charged pages with uncharged pages and uncharge the
container as the old pages are freed, while a cgroup task reading
global_trace will swap pages charged to its own memcg into global_trace.
Similarly, trace_rb_cpu_prepare() during runtime CPU hotplug allocates
per-CPU buffers for all existing instances in the context of the task
onlining the CPU.

[Severity: Medium]
Can hitting the memcg limit in alloc_cpu_data() during instance creation
trigger the WARN(1) in trace_rb_cpu_prepare()?

When alloc_buffer() creates a ring buffer, it allocates the first CPU's
buffer directly and then calls
cpuhp_state_add_instance(CPUHP_TRACE_RB_PREPARE, &buffer->node), which runs
trace_rb_cpu_prepare() for each remaining present CPU:

kernel/trace/ring_buffer.c:trace_rb_cpu_prepare() {
    ...
	buffer->buffers[cpu] =
		rb_allocate_cpu_buffer(buffer, nr_pages, cpu);
	if (!buffer->buffers[cpu]) {
		WARN(1, "failed to allocate ring buffer on CPU %u\n",
		     cpu);
		return -ENOMEM;
	}
    ...
}

Because __rb_allocate_pages() only checks si_mem_available() against global
memory, a cgroup with enough headroom for the first CPU (~1.375 MB) that
hits its memcg limit on any subsequent CPU in trace_rb_cpu_prepare() will
trigger this WARN(1) and crash the host if panic_on_warn is set.

> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 08614af8fad1c..fd1cfd1f66fbe 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -8677,11 +8677,11 @@ trace_array_create_systems(const char *name, const char *systems,
>  	int ret;
>  
>  	ret = -ENOMEM;
> -	tr = kzalloc_obj(*tr);
> +	tr = kzalloc_obj(*tr, GFP_KERNEL_ACCOUNT);
>  	if (!tr)
>  		return ERR_PTR(ret);
>  
> -	tr->name = kstrdup(name, GFP_KERNEL);
> +	tr->name = kstrdup(name, GFP_KERNEL_ACCOUNT);
>  	if (!tr->name)
>  		goto out_free_tr;
>  
> @@ -8692,7 +8692,7 @@ trace_array_create_systems(const char *name, const char *systems,
>  		goto out_free_tr;
>  
>  	if (systems) {
> -		tr->system_names = kstrdup_const(systems, GFP_KERNEL);
> +		tr->system_names = kstrdup_const(systems, GFP_KERNEL_ACCOUNT);
>  		if (!tr->system_names)
>  			goto out_free_tr;
>  	}

[Severity: Low]
Should the alloc_cpumask_var() and zalloc_cpumask_var() calls right between
tr->name and tr->system_names, as well as alloc_cpu_page() and
alloc_cpu_buffer() in ring_buffer.c, also use GFP_KERNEL_ACCOUNT?

Also, the commit message says creating an instance immediately allocates a
full ~1.375 MB per-CPU snapshot buffer, but trace_allocate_snapshot() passes
size = 1 when allocate_snapshot is false (which is always the case after
boot), so snapshot_buffer only allocates the minimum pages until a snapshot
is armed.

[Severity: Medium]
This problem wasn't introduced by this patch, but in
trace_array_create_systems() [1], does the out_free_tr error path leak
tr->tracers when trace_array_create_dir() fails?

trace_array_create_dir() calls __update_tracer() -> add_tracer(), which
allocates struct tracers and struct tracer_flags entries on tr->tracers.  If
a later step in trace_array_create_dir() fails and jumps to out_free_tr,
nothing calls free_tracers(tr), leaking the entries already added to
tr->tracers.

[1] https://sashiko.dev/#/bug/linux-9449d18e-1233-4181-9199-09eaead7b576

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106131829.3228926-1-cui.tao@linux.dev?part=4