All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/1] tracing: fix bug in rb_per_cpu_empty() that might cause deadloop.
@ 2021-07-21  3:50 Haoran Luo
  2021-07-21 13:12 ` Steven Rostedt
  0 siblings, 1 reply; 2+ messages in thread
From: Haoran Luo @ 2021-07-21  3:50 UTC (permalink / raw)
  To: rostedt, mingo; +Cc: www, linux-trace-devel, security

The "rb_per_cpu_empty()" misinterpret the condition (as not-empty) when
"head_page" and "commit_page" of "struct ring_buffer_per_cpu" points to
the same buffer page, whose "buffer_data_page" is empty and "read" field
is non-zero.

An error scenario could be constructed as followed (kernel perspective):

1. All pages in the buffer has been accessed by reader(s) so that all of
them will have non-zero "read" field.

2. Read and clear all buffer pages so that "rb_num_of_entries()" will
return 0 rendering there's no more data to read. It is also required
that the "read_page", "commit_page" and "tail_page" points to the same
page, while "head_page" is the next page of them.

3. Invoke "ring_buffer_lock_reserve()" with large enough "length"
so that it shot pass the end of current tail buffer page. Now the
"head_page", "commit_page" and "tail_page" points to the same page.

4. Discard current event with "ring_buffer_discard_commit()", so that
"head_page", "commit_page" and "tail_page" points to a page whose buffer
data page is now empty.

When the error scenario has been constructed, "tracing_read_pipe" will
be trapped inside a deadloop: "trace_empty()" returns 0 since
"rb_per_cpu_empty()" returns 0 when it hits the CPU containing such
constructed ring buffer. Then "trace_find_next_entry_inc()" always
return NULL since "rb_num_of_entries()" reports there's no more entry
to read. Finally "trace_seq_to_user()" returns "-EBUSY" spanking
"tracing_read_pipe" back to the start of the "waitagain" loop.

I've also written a proof-of-concept script to construct the scenario
and trigger the bug automatically, you can use it to trace and validate
my reasoning above:

  https://github.com/aegistudio/RingBufferDetonator.git

Tests has been carried out on linux kernel 5.14-rc2
(2734d6c1b1a089fb593ef6a23d4b70903526fe0c), my fixed version
of kernel (for testing whether my update fixes the bug) and
some older kernels (for range of affected kernels). Test result is
also attached to the proof-of-concept repository.

Signed-off-by: Haoran Luo <www@aegistudio.net>
---
 kernel/trace/ring_buffer.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index d1463eac11a3..fb9129029a32 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -3880,10 +3880,30 @@ static bool rb_per_cpu_empty(struct ring_buffer_per_cpu *cpu_buffer)
 	if (unlikely(!head))
 		return true;
 
-	return reader->read == rb_page_commit(reader) &&
-		(commit == reader ||
-		 (commit == head &&
-		  head->read == rb_page_commit(commit)));
+	/* Reader should exhaust content in reader page */
+	if (reader->read != rb_page_commit(reader))
+		return false;
+
+	/*
+	 * If writers are committing on the reader page, knowing all
+	 * committed content has been read, the ring buffer is empty.
+	 */
+	if (commit == reader)
+		return true;
+
+	/*
+	 * If writers are committing on the head page, we just need
+	 * to care about there're committed data, and the reader will
+	 * swap reader page with head page when it is to read data.
+	 */
+	if (commit == head)
+		return rb_page_commit(commit) == 0;
+
+	/*
+	 * Writers are committing on page other than reader page and
+	 * head page, there should always be content to read.
+	 */
+	return false;
 }
 
 /**
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH v2 1/1] tracing: fix bug in rb_per_cpu_empty() that might cause deadloop.
  2021-07-21  3:50 [PATCH v2 1/1] tracing: fix bug in rb_per_cpu_empty() that might cause deadloop Haoran Luo
@ 2021-07-21 13:12 ` Steven Rostedt
  0 siblings, 0 replies; 2+ messages in thread
From: Steven Rostedt @ 2021-07-21 13:12 UTC (permalink / raw)
  To: Haoran Luo; +Cc: mingo, linux-trace-devel, security

On Wed, 21 Jul 2021 03:50:16 +0000
Haoran Luo <www@aegistudio.net> wrote:

> +	/*
> +	 * If writers are committing on the head page, we just need
> +	 * to care about there're committed data, and the reader will
> +	 * swap reader page with head page when it is to read data.
> +	 */
> +	if (commit == head)
> +		return rb_page_commit(commit) == 0;
> +
> +	/*
> +	 * Writers are committing on page other than reader page and
> +	 * head page, there should always be content to read.
> +	 */
> +	return false;
>  }
>  

I like the patch, but could you swap the above two operations. I find
that return conditional in the if condition rather ugly. That is, can
you instead have:


	/*
	 * If writers are committing on a page other than the reader
	 * page and head page, there should always be content to read.
	 */
	if (commit != head)
		return false;

	/*
	 * Writers are committing on the head page, we just need
	 * to care about there're committed data, and the reader will
	 * swap reader page with head page when it is to read data.
	 */
	return rb_page_commit(commit) == 0;

-- Steve

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-07-21 13:12 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-21  3:50 [PATCH v2 1/1] tracing: fix bug in rb_per_cpu_empty() that might cause deadloop Haoran Luo
2021-07-21 13:12 ` Steven Rostedt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.