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 <[email protected]> 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/[email protected]?part=4
