From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f44.google.com (mail-wm1-f44.google.com [209.85.128.44]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6C7723D9552 for ; Fri, 14 Aug 2026 14:53:50 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.44 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786719232; cv=none; b=WawtxNtRFJ0J8E+BLK8sFn8NCeqIYOHAY935aTyDybPkLZW3LZ1NaasVqopoXZ1UNY3jdqOvkN5FmV26/RetHDV3V8BE+SppUAvJaj7MHlkv3LSbHNUt+zhrG7ajMsHMc8IuceeJW6WGAVdA+0k0Gx2NFA/FIfVOSxU7hy+KtWI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786719232; c=relaxed/simple; bh=mzk8aKbdFuG5Vu68J+c+zfEBpGwzivGgHtRkC0dkzrQ=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=TpuWFLvD0R4h45T8qjKlGGS5hUyREEm3PlZozbtTIPhiHJBdMt6eyIoGtMDnLa7ddlsAR5Dr6xDKiuSeRd6Ac8oDTX1OB5gfUY8sA7ISHbfJz7dmvc52O8+z/b1l+OThtQEU0svUpNVFY0Y+7stZnY0xkXltLF5xX9ivQ0Wc1qg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=YEamMCKW; arc=none smtp.client-ip=209.85.128.44 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="YEamMCKW" Received: by mail-wm1-f44.google.com with SMTP id 5b1f17b1804b1-4956242332dso11425995e9.2 for ; Fri, 14 Aug 2026 07:53:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20251104; t=1786719228; x=1787324028; darn=vger.kernel.org; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:from:to:cc:subject :date:message-id:reply-to:content-type; bh=RQImDO1MNrue3/f5xyKVW9ULG6rmGDlRWjNqJzFU5E8=; b=YEamMCKWRo8YPLoYLPBN/3wNdz1XdF0dThkQm4+rtt8AqFZbhS2fSKxklE19xCEAKa VjB8WVvG7DhGzSiXaObYr+GkRBQyiah0zrXW8UxxFeDZdG1WpNT331OX4Yptf4RXN6Y2 wfj31m2UG7G8D1sN+DhTllyyhOanR4rELEkXcEZM2pdsnw5bvyeVfeuKQ6ceWwL9nfU3 wjdZaUs4Tm1fbfBATDUHA7NPDVLgyNeladI3Knjfc4mF+Fy+s6QON/A0cG4oQEALojkQ qzlc82tpq10xD+h5qOAU7gofZjEwWE335MSuDJ/+oOp1iBRL1itih5GAb6l8R4zqwm3w WQ+g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1786719228; x=1787324028; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=RQImDO1MNrue3/f5xyKVW9ULG6rmGDlRWjNqJzFU5E8=; b=fEa3qh4T+apkFpgEgaHmGPXOEbZqfYB9U4JJ6f1XvohcP8MyuCqVqthZjfJCPvBRo5 +jQfYXNumd1q/YT9DpBau68uCgOOHH7B/lQDg+Ci9sa2Kt+UX73i/uCjdsbpdzqykpdt 9BbokK27jbeGMg9KN5Gdtk51pNWn2Kl2rTsIYeIQyJmvtTZ+v2MhxxXcJxw7bYSDXAU6 ysEsshQX8EsrhLjW0lHravGvFYks3hV3yVMn/8pMCvdnLB1XghS/Q3g5A4Ay1Ts/4yvX 6eh0fIDxTAm26DprtZT3vCBSDP3u/IXgwYrrHw/xLXyZ/T8+g+ZY2MNIhEQ7WGwysGzU 2jtw== X-Forwarded-Encrypted: i=1; AHgh+RpOpeNCnDUkO4meYYyg6h9eA1G4VALpyxKxvPa+2g4lEWamGKCjmocOCuwUV7wbJ9EgvjE79L9aqss9vsk=@vger.kernel.org X-Gm-Message-State: AOJu0YyFHmlM7RymDfZRFwUMsXoX02k3W5ofzarvEQsD1twu3A+rfedx 3s5pB9O2vYFBeNENYbR0H6Bjy/ueGJBBP+IfyqhTSnVG9iyD+NtlhsoqRrsY3kIG/A== X-Gm-Gg: AR+sD126GDVDNMK/6ODOH/qs71+bECbaDFH4+DWLjCWc57jfuUbau8U2pEax3NsiwuP LVaP98UFxOeas4taXRJRWYPlt7scBNfEhF+OMrNb3nFnzXhXmHs2E1hq2/QaxJPcJVUisP4UfH6 QgEUXUCwjaIj0QRTDSV8ex1ESQWfuCxY86H4mRNVgrKlqxZ24Sd45amVjl6/pJ3eY9qy0s53ajB fj2auyaFZnpZ++3xBSUSe6lPFlN1k7aw6/UqgwbdqLxHrLCIBPgDcWKSpFP/Iwih4rh0IGDazZz 1zZzkbYUyG68GxJKVEiKBmbTcTu71W5PuJdUeXsmJ3DZ/EP6uicplR7J7yYmg0nNf+1P/4Ust5q LMitQsawhAKjtGgbSrB5uDkij7NrqjOHZKNkm66KV3HEYG7daTrNG43A8EPsHaGlDCcf46ErLtI lWX8XBtOWWFfChQp2e8183yFGJHI36W8Un8/93uvVDkuKtlghdWFuE11zbctBRC5ZG6KVYcQa3U d9gAVKV89LllKynFQgyOAQBpQSrTGBJ X-Received: by 2002:a05:600c:19d1:b0:495:4e89:3f30 with SMTP id 5b1f17b1804b1-499879ae4f4mr84287635e9.15.1786719227894; Fri, 14 Aug 2026 07:53:47 -0700 (PDT) Received: from google.com (135.91.155.104.bc.googleusercontent.com. [104.155.91.135]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-49988b1d33asm60194785e9.10.2026.08.14.07.53.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 14 Aug 2026 07:53:47 -0700 (PDT) Date: Fri, 14 Aug 2026 15:53:43 +0100 From: Vincent Donnefort To: Steven Rostedt Cc: "Masami Hiramatsu (Google)" , Mathieu Desnoyers , linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org, kernel-team@android.com Subject: Re: [PATCH] ring-buffer: Fix race between ring_buffer_subbuf_order_set() and readers Message-ID: References: <178663776361.475864.7685868697103378735.stgit@devnote2> <178663777320.475864.4716637934003507750.stgit@devnote2> <20260814104208.28f47749@gandalf.local.home> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260814104208.28f47749@gandalf.local.home> On Fri, Aug 14, 2026 at 10:42:08AM -0400, Steven Rostedt wrote: > On Fri, 14 Aug 2026 01:16:13 +0900 > "Masami Hiramatsu (Google)" wrote: > > > From: Masami Hiramatsu (Google) > > > > When ring_buffer_subbuf_order_set() updates buffer->subbuf_order, it > > previously modified buffer->subbuf_order before clearing the cached > > per-CPU free_page entries. Furthermore, clearing cpu_buffer->free_page > > was done under cpu_buffer->reader_lock, whereas > > ring_buffer_alloc_read_page() protects cpu_buffer->free_page using > > arch_spin_lock(&cpu_buffer->lock). > > > > Because ring_buffer_alloc_read_page(), ring_buffer_free_read_page(), > > and ring_buffer_read_page() checked buffer->subbuf_order locklessly > > before accessing reader resources, a TOCTOU race allowed a concurrent > > reader to obtain, cache, or swap a page allocated under an outdated order > > while tagging bpage->order with the new order. This allowed undersized > > pages to be swapped into the ring buffer, leading to heap buffer overflows, > > or caused free_pages() to be called with an invalid order. > > > > Fix this by: > > 1. Flushing and freeing all per-CPU cached free_page entries under > > arch_spin_lock(&cpu_buffer->lock) using old_order before modifying > > buffer->subbuf_order. > > 2. Protecting bpage->order assignment under > > arch_spin_lock(&cpu_buffer->lock) in ring_buffer_alloc_read_page(). > > 3. Moving the buffer->subbuf_order validation inside > > arch_spin_lock(&cpu_buffer->lock) in ring_buffer_free_read_page(). > > 4. Re-validating buffer->subbuf_order inside reader_lock in > > ring_buffer_read_page(). > > 5. Protecting cpu_buffer->free_page extraction with > > arch_spin_lock(&cpu_buffer->lock) in ring_buffer_subbuf_order_set(). > > > > Fixes: 2808e31ec12e ("ring-buffer: Add interface for configuring trace sub buffer size") > > Assisted-by: Antigravity:gemini-3.6-flash > > Signed-off-by: Masami Hiramatsu (Google) > > --- > > > Please rebase on top of ring-buffer/for-next, as I added Vincent's patches to that. > > > kernel/trace/ring_buffer.c | 35 +++++++++++++++++++++++++++++------ > > 1 file changed, 29 insertions(+), 6 deletions(-) > > > > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c > > index 2667992f0aa2..6d180689ad59 100644 > > --- a/kernel/trace/ring_buffer.c > > +++ b/kernel/trace/ring_buffer.c > > @@ -6957,11 +6957,11 @@ ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu) > > if (!bpage) > > return ERR_PTR(-ENOMEM); > > > > - bpage->order = buffer->subbuf_order; > > cpu_buffer = buffer->buffers[cpu]; > > local_irq_save(flags); > > arch_spin_lock(&cpu_buffer->lock); > > > > + bpage->order = buffer->subbuf_order; > > This is still needed. I am not sure, the lock is per-cpu_buffer but subbuf_order. is global to trace_buffer? > > > if (cpu_buffer->free_page) { > > bpage->data = cpu_buffer->free_page; > > cpu_buffer->free_page = NULL; > > @@ -7010,13 +7010,13 @@ void ring_buffer_free_read_page(struct trace_buffer *buffer, int cpu, > > * is different from the subbuffer order of the buffer - > > * we can't reuse it > > */ > > - if (page_ref_count(page) > 1 || data_page->order != buffer->subbuf_order) > > + if (page_ref_count(page) > 1) > > goto out; > > > > local_irq_save(flags); > > arch_spin_lock(&cpu_buffer->lock); > > > > - if (!cpu_buffer->free_page) { > > + if (data_page->order == buffer->subbuf_order && !cpu_buffer->free_page) { > > Swap the order please. It has to check both to continue and if one fails it > will not continue. Checking for cpu_buffer->free_page to be NULL first is > the quicker check. And also the more likely one to fail. > > > cpu_buffer->free_page = dpage; > > dpage = NULL; > > } > > @@ -7094,15 +7094,15 @@ int ring_buffer_read_page(struct trace_buffer *buffer, > > if (!data_page || !data_page->data) > > return -1; > > > > - if (data_page->order != buffer->subbuf_order) > > - return -1; > > - > > dpage = data_page->data; > > if (!dpage) > > return -1; > > > > guard(raw_spinlock_irqsave)(&cpu_buffer->reader_lock); > > > > + if (data_page->order != buffer->subbuf_order) > > + return -1; > > + I have modified this as part of tracing: Fix subbuf resize races with trace_pipe_raw readers > > reader = rb_get_reader_page(cpu_buffer); > > if (!reader) > > return -1; > > @@ -7350,6 +7350,27 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order) > > /* Make sure all commits have finished */ > > synchronize_rcu(); > > > > + /* Flush any cached free_page allocated with old_order */ > > + for_each_buffer_cpu(buffer, cpu) { > > + struct buffer_data_page *old_free; > > + unsigned long flags; > > + > > + if (!cpumask_test_cpu(cpu, buffer->cpumask)) > > + continue; > > + > > + cpu_buffer = buffer->buffers[cpu]; > > + > > + local_irq_save(flags); > > + arch_spin_lock(&cpu_buffer->lock); > > + old_free = cpu_buffer->free_page; > > + cpu_buffer->free_page = NULL; > > + arch_spin_unlock(&cpu_buffer->lock); > > + local_irq_restore(flags); > > + > > + if (old_free) > > + free_pages((unsigned long)old_free, old_order); > > + } > > Honestly, this should be a separate patch. The first part of this patch is > data races with adding and freeing, but this is about changes to the size. > > > + > > buffer->subbuf_order = order; > > buffer->subbuf_size = psize - BUF_PAGE_HDR_SIZE; > > > > @@ -7431,8 +7452,10 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order) > > cpu_buffer->nr_pages = cpu_buffer->nr_pages_to_update; > > cpu_buffer->nr_pages_to_update = 0; > > > > + arch_spin_lock(&cpu_buffer->lock); > > old_free_data_page = cpu_buffer->free_page; > > cpu_buffer->free_page = NULL; > > + arch_spin_unlock(&cpu_buffer->lock); > > This is already fixed by Vincent (and I would have asked this to be a > separate patch too if it hadn't). > > -- Steve > > > > > rb_head_page_activate(cpu_buffer); > > > -- Vincent