[PATCH] ring-buffer: fix error handling in ring_buffer_subbuf_order_set()

[PATCH] ring-buffer: fix error handling in ring_buffer_subbuf_order_set()

[Dmitry Antipov]

In 'ring_buffer_subbuf_order_set()', enlarge critical section to
ensure that error handling takes place with per-buffer mutex hold,
thus preventing list corruption and other concurrency-related issues.

Reported-by: syzbot+05d673e83ec640f0ced9@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=05d673e83ec640f0ced9
Fixes: f9b94daa542a8 ("ring-buffer: Set new size of the ring buffer sub page")
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 kernel/trace/ring_buffer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index e24509bd0af5..2028a24d6418 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -6908,9 +6908,6 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
 	buffer->subbuf_order = old_order;
 	buffer->subbuf_size = old_size;
 
-	atomic_dec(&buffer->record_disabled);
-	mutex_unlock(&buffer->mutex);
-
 	for_each_buffer_cpu(buffer, cpu) {
 		cpu_buffer = buffer->buffers[cpu];
 
@@ -6923,6 +6920,9 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
 		}
 	}
 
+	atomic_dec(&buffer->record_disabled);
+	mutex_unlock(&buffer->mutex);
+
 	return err;
 }
 EXPORT_SYMBOL_GPL(ring_buffer_subbuf_order_set);
-- 
2.49.0

Re: [PATCH] ring-buffer: fix error handling in ring_buffer_subbuf_order_set()

[Steven Rostedt]

On Fri,  6 Jun 2025 12:12:17 +0300
Dmitry Antipov <dmantipov@yandex.ru> wrote:

> In 'ring_buffer_subbuf_order_set()', enlarge critical section to
> ensure that error handling takes place with per-buffer mutex hold,
> thus preventing list corruption and other concurrency-related issues.
> 
> Reported-by: syzbot+05d673e83ec640f0ced9@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=05d673e83ec640f0ced9
> Fixes: f9b94daa542a8 ("ring-buffer: Set new size of the ring buffer sub page")
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
>  kernel/trace/ring_buffer.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index e24509bd0af5..2028a24d6418 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -6908,9 +6908,6 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
>  	buffer->subbuf_order = old_order;
>  	buffer->subbuf_size = old_size;
>  
> -	atomic_dec(&buffer->record_disabled);

There's no reason to move the record_disable. Enabling recording here is
fine, as the pages being freed have not been added to the ring buffer.

> -	mutex_unlock(&buffer->mutex);
> -
>  	for_each_buffer_cpu(buffer, cpu) {
>  		cpu_buffer = buffer->buffers[cpu];
>  
> @@ -6923,6 +6920,9 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
>  		}
>  	}
>  
> +	atomic_dec(&buffer->record_disabled);
> +	mutex_unlock(&buffer->mutex);

As this moves the mutex to the end, we can instead just remove the
mutex_unlock()s and replace the mutex() with:

	guard(mutex)(&buffer->mutex);

Care to send a v2?

-- Steve


> +
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(ring_buffer_subbuf_order_set);

[PATCH v2] ring-buffer: fix buffer locking in ring_buffer_subbuf_order_set()

[Dmitry Antipov]

In 'ring_buffer_subbuf_order_set()', prefer mutex guard over explicit
mutex operations to ensure that the buffer remains locked everywhere
where its innards may be changed, thus preventing per-CPU sub-buffer
lists corruptions and other concurrency-related issues.

Reported-by: syzbot+05d673e83ec640f0ced9@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=05d673e83ec640f0ced9
Fixes: f9b94daa542a8 ("ring-buffer: Set new size of the ring buffer sub page")
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v2: use mutex guard as suggested by Steven
---
 kernel/trace/ring_buffer.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index e24509bd0af5..00fc38d70e86 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -6795,7 +6795,7 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
 	old_size = buffer->subbuf_size;
 
 	/* prevent another thread from changing buffer sizes */
-	mutex_lock(&buffer->mutex);
+	guard(mutex)(&buffer->mutex);
 	atomic_inc(&buffer->record_disabled);
 
 	/* Make sure all commits have finished */
@@ -6900,7 +6900,6 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
 	}
 
 	atomic_dec(&buffer->record_disabled);
-	mutex_unlock(&buffer->mutex);
 
 	return 0;
 
@@ -6909,7 +6908,6 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
 	buffer->subbuf_size = old_size;
 
 	atomic_dec(&buffer->record_disabled);
-	mutex_unlock(&buffer->mutex);
 
 	for_each_buffer_cpu(buffer, cpu) {
 		cpu_buffer = buffer->buffers[cpu];
-- 
2.49.0

Re: [PATCH v2] ring-buffer: fix buffer locking in ring_buffer_subbuf_order_set()

[Masami Hiramatsu]

On Fri,  6 Jun 2025 14:22:42 +0300
Dmitry Antipov <dmantipov@yandex.ru> wrote:

> In 'ring_buffer_subbuf_order_set()', prefer mutex guard over explicit
> mutex operations to ensure that the buffer remains locked everywhere
> where its innards may be changed, thus preventing per-CPU sub-buffer
> lists corruptions and other concurrency-related issues.
> 
> Reported-by: syzbot+05d673e83ec640f0ced9@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=05d673e83ec640f0ced9
> Fixes: f9b94daa542a8 ("ring-buffer: Set new size of the ring buffer sub page")
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>

Looks good to me.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks,

> ---
> v2: use mutex guard as suggested by Steven
> ---
>  kernel/trace/ring_buffer.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index e24509bd0af5..00fc38d70e86 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -6795,7 +6795,7 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
>  	old_size = buffer->subbuf_size;
>  
>  	/* prevent another thread from changing buffer sizes */
> -	mutex_lock(&buffer->mutex);
> +	guard(mutex)(&buffer->mutex);
>  	atomic_inc(&buffer->record_disabled);
>  
>  	/* Make sure all commits have finished */
> @@ -6900,7 +6900,6 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
>  	}
>  
>  	atomic_dec(&buffer->record_disabled);
> -	mutex_unlock(&buffer->mutex);
>  
>  	return 0;
>  
> @@ -6909,7 +6908,6 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
>  	buffer->subbuf_size = old_size;
>  
>  	atomic_dec(&buffer->record_disabled);
> -	mutex_unlock(&buffer->mutex);
>  
>  	for_each_buffer_cpu(buffer, cpu) {
>  		cpu_buffer = buffer->buffers[cpu];
> -- 
> 2.49.0
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>