[PATCH] trace: Fix race in trace_open and buffer resize call

[PATCH] trace: Fix race in trace_open and buffer resize call

[Gaurav Kohli]

Below race can come, if trace_open and resize of
cpu buffer is running parallely on different cpus
CPUX                                CPUY
				    ring_buffer_resize
				    atomic_read(&buffer->resize_disabled)
tracing_open
tracing_reset_online_cpus
ring_buffer_reset_cpu
rb_reset_cpu
				    rb_update_pages
				    remove/insert pages
resetting pointer

This race can cause data abort or some times infinite loop in
rb_remove_pages and rb_insert_pages while checking pages
for sanity.

Take buffer lock to fix this.

Link: https://lkml.kernel.org/r/1601976833-24377-1-git-send-email-gkohli@codeaurora.org

Reported-by: Denis Efremov <efremov@linux.com>
Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org>

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 077877e..7283741 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -4448,6 +4448,8 @@ void ring_buffer_reset_cpu(struct ring_buffer *buffer, int cpu)
 
 	if (!cpumask_test_cpu(cpu, buffer->cpumask))
 		return;
+	/* prevent another thread from changing buffer sizes */
+	mutex_lock(&buffer->mutex);
 
 	atomic_inc(&buffer->resize_disabled);
 	atomic_inc(&cpu_buffer->record_disabled);
@@ -4471,6 +4473,8 @@ void ring_buffer_reset_cpu(struct ring_buffer *buffer, int cpu)
 
 	atomic_dec(&cpu_buffer->record_disabled);
 	atomic_dec(&buffer->resize_disabled);
+
+	mutex_unlock(&buffer->mutex);
 }
 EXPORT_SYMBOL_GPL(ring_buffer_reset_cpu);
 
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH] trace: Fix race in trace_open and buffer resize call

[Greg KH]

On Sun, Jan 24, 2021 at 03:26:33PM +0530, Gaurav Kohli wrote:
> Below race can come, if trace_open and resize of

<snip>

You forgot to cc: stable, and put the git id that this patch is
upstream...

Re: [PATCH] trace: Fix race in trace_open and buffer resize call

[Steven Rostedt]

On Sun, 24 Jan 2021 11:05:28 +0100
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Sun, Jan 24, 2021 at 03:26:33PM +0530, Gaurav Kohli wrote:
> > Below race can come, if trace_open and resize of  
> 
> <snip>
> 
> You forgot to cc: stable, and put the git id that this patch is
> upstream...

Hi Greg,

Sorry for the confusion. I wanted to give it a test before pushing it
officially to stable. Gaurav was just following my instructions. After I
get a chance to test it (tomorrow), I'll send it properly to stable.

Thanks,

-- Steve

[PATCH] trace: Fix race in trace_open and buffer resize call

[Gaurav Kohli]

Below race can come, if trace_open and resize of
cpu buffer is running parallely on different cpus
CPUX                                CPUY
				    ring_buffer_resize
				    atomic_read(&buffer->resize_disabled)
tracing_open
tracing_reset_online_cpus
ring_buffer_reset_cpu
rb_reset_cpu
				    rb_update_pages
				    remove/insert pages
resetting pointer
This race can cause data abort or some times infinte loop in 
rb_remove_pages and rb_insert_pages while checking pages 
for sanity.
Take ring buffer lock in trace_open to avoid resetting of cpu buffer.

Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org>

diff --git a/include/linux/ring_buffer.h b/include/linux/ring_buffer.h
index 136ea09..55f9115 100644
--- a/include/linux/ring_buffer.h
+++ b/include/linux/ring_buffer.h
@@ -163,6 +163,8 @@ bool ring_buffer_empty_cpu(struct trace_buffer *buffer, int cpu);
 
 void ring_buffer_record_disable(struct trace_buffer *buffer);
 void ring_buffer_record_enable(struct trace_buffer *buffer);
+void ring_buffer_mutex_acquire(struct trace_buffer *buffer);
+void ring_buffer_mutex_release(struct trace_buffer *buffer);
 void ring_buffer_record_off(struct trace_buffer *buffer);
 void ring_buffer_record_on(struct trace_buffer *buffer);
 bool ring_buffer_record_is_on(struct trace_buffer *buffer);
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 93ef0ab..638ec8f 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -3632,6 +3632,25 @@ void ring_buffer_record_enable(struct trace_buffer *buffer)
 EXPORT_SYMBOL_GPL(ring_buffer_record_enable);
 
 /**
+ * ring_buffer_mutex_acquire - prevent resetting of buffer
+ * during resize
+ */
+void ring_buffer_mutex_acquire(struct trace_buffer *buffer)
+{
+	mutex_lock(&buffer->mutex);
+}
+EXPORT_SYMBOL_GPL(ring_buffer_mutex_acquire);
+
+/**
+ * ring_buffer_mutex_release - prevent resetting of buffer
+ * during resize
+ */
+void ring_buffer_mutex_release(struct trace_buffer *buffer)
+{
+	mutex_unlock(&buffer->mutex);
+}
+EXPORT_SYMBOL_GPL(ring_buffer_mutex_release);
+/**
  * ring_buffer_record_off - stop all writes into the buffer
  * @buffer: The ring buffer to stop writes to.
  *
@@ -4918,6 +4937,8 @@ void ring_buffer_reset(struct trace_buffer *buffer)
 	struct ring_buffer_per_cpu *cpu_buffer;
 	int cpu;
 
+	/* prevent another thread from changing buffer sizes */
+	mutex_lock(&buffer->mutex);
 	for_each_buffer_cpu(buffer, cpu) {
 		cpu_buffer = buffer->buffers[cpu];
 
@@ -4936,6 +4957,7 @@ void ring_buffer_reset(struct trace_buffer *buffer)
 		atomic_dec(&cpu_buffer->record_disabled);
 		atomic_dec(&cpu_buffer->resize_disabled);
 	}
+	mutex_unlock(&buffer->mutex);
 }
 EXPORT_SYMBOL_GPL(ring_buffer_reset);
 
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index f40d850..392e9aa 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2006,6 +2006,8 @@ void tracing_reset_online_cpus(struct array_buffer *buf)
 	if (!buffer)
 		return;
 
+	ring_buffer_mutex_acquire(buffer);
+
 	ring_buffer_record_disable(buffer);
 
 	/* Make sure all commits have finished */
@@ -2016,6 +2018,8 @@ void tracing_reset_online_cpus(struct array_buffer *buf)
 	ring_buffer_reset_online_cpus(buffer);
 
 	ring_buffer_record_enable(buffer);
+
+	ring_buffer_mutex_release(buffer);
 }
 
 /* Must have trace_types_lock held */
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH] trace: Fix race in trace_open and buffer resize call

[Gaurav Kohli]

Hi Steven,

Please let us know, if below change looks good.
Or let us know some other way to solve this.

Thanks,
Gaurav



On 9/4/2020 11:39 AM, Gaurav Kohli wrote:
> Below race can come, if trace_open and resize of
> cpu buffer is running parallely on different cpus
> CPUX                                CPUY
> 				    ring_buffer_resize
> 				    atomic_read(&buffer->resize_disabled)
> tracing_open
> tracing_reset_online_cpus
> ring_buffer_reset_cpu
> rb_reset_cpu
> 				    rb_update_pages
> 				    remove/insert pages
> resetting pointer
> This race can cause data abort or some times infinte loop in
> rb_remove_pages and rb_insert_pages while checking pages
> for sanity.
> Take ring buffer lock in trace_open to avoid resetting of cpu buffer.
> 
> Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org>
> 
> diff --git a/include/linux/ring_buffer.h b/include/linux/ring_buffer.h
> index 136ea09..55f9115 100644
> --- a/include/linux/ring_buffer.h
> +++ b/include/linux/ring_buffer.h
> @@ -163,6 +163,8 @@ bool ring_buffer_empty_cpu(struct trace_buffer *buffer, int cpu);
>   
>   void ring_buffer_record_disable(struct trace_buffer *buffer);
>   void ring_buffer_record_enable(struct trace_buffer *buffer);
> +void ring_buffer_mutex_acquire(struct trace_buffer *buffer);
> +void ring_buffer_mutex_release(struct trace_buffer *buffer);
>   void ring_buffer_record_off(struct trace_buffer *buffer);
>   void ring_buffer_record_on(struct trace_buffer *buffer);
>   bool ring_buffer_record_is_on(struct trace_buffer *buffer);
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 93ef0ab..638ec8f 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -3632,6 +3632,25 @@ void ring_buffer_record_enable(struct trace_buffer *buffer)
>   EXPORT_SYMBOL_GPL(ring_buffer_record_enable);
>   
>   /**
> + * ring_buffer_mutex_acquire - prevent resetting of buffer
> + * during resize
> + */
> +void ring_buffer_mutex_acquire(struct trace_buffer *buffer)
> +{
> +	mutex_lock(&buffer->mutex);
> +}
> +EXPORT_SYMBOL_GPL(ring_buffer_mutex_acquire);
> +
> +/**
> + * ring_buffer_mutex_release - prevent resetting of buffer
> + * during resize
> + */
> +void ring_buffer_mutex_release(struct trace_buffer *buffer)
> +{
> +	mutex_unlock(&buffer->mutex);
> +}
> +EXPORT_SYMBOL_GPL(ring_buffer_mutex_release);
> +/**
>    * ring_buffer_record_off - stop all writes into the buffer
>    * @buffer: The ring buffer to stop writes to.
>    *
> @@ -4918,6 +4937,8 @@ void ring_buffer_reset(struct trace_buffer *buffer)
>   	struct ring_buffer_per_cpu *cpu_buffer;
>   	int cpu;
>   
> +	/* prevent another thread from changing buffer sizes */
> +	mutex_lock(&buffer->mutex);
>   	for_each_buffer_cpu(buffer, cpu) {
>   		cpu_buffer = buffer->buffers[cpu];
>   
> @@ -4936,6 +4957,7 @@ void ring_buffer_reset(struct trace_buffer *buffer)
>   		atomic_dec(&cpu_buffer->record_disabled);
>   		atomic_dec(&cpu_buffer->resize_disabled);
>   	}
> +	mutex_unlock(&buffer->mutex);
>   }
>   EXPORT_SYMBOL_GPL(ring_buffer_reset);
>   
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index f40d850..392e9aa 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2006,6 +2006,8 @@ void tracing_reset_online_cpus(struct array_buffer *buf)
>   	if (!buffer)
>   		return;
>   
> +	ring_buffer_mutex_acquire(buffer);
> +
>   	ring_buffer_record_disable(buffer);
>   
>   	/* Make sure all commits have finished */
> @@ -2016,6 +2018,8 @@ void tracing_reset_online_cpus(struct array_buffer *buf)
>   	ring_buffer_reset_online_cpus(buffer);
>   
>   	ring_buffer_record_enable(buffer);
> +
> +	ring_buffer_mutex_release(buffer);
>   }
>   
>   /* Must have trace_types_lock held */
> 

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH] trace: Fix race in trace_open and buffer resize call

[Steven Rostedt]

On Mon, 14 Sep 2020 10:00:50 +0530
Gaurav Kohli <gkohli@codeaurora.org> wrote:

> Hi Steven,
> 
> Please let us know, if below change looks good.
> Or let us know some other way to solve this.
> 
> Thanks,
> Gaurav
> 
> 

Hmm, for some reason, I don't see this in my INBOX, but it shows up in my
LKML folder. :-/


> 
> On 9/4/2020 11:39 AM, Gaurav Kohli wrote:
> > Below race can come, if trace_open and resize of
> > cpu buffer is running parallely on different cpus
> > CPUX                                CPUY
> > 				    ring_buffer_resize
> > 				    atomic_read(&buffer->resize_disabled)
> > tracing_open
> > tracing_reset_online_cpus
> > ring_buffer_reset_cpu
> > rb_reset_cpu
> > 				    rb_update_pages
> > 				    remove/insert pages
> > resetting pointer
> > This race can cause data abort or some times infinte loop in
> > rb_remove_pages and rb_insert_pages while checking pages
> > for sanity.
> > Take ring buffer lock in trace_open to avoid resetting of cpu buffer.
> > 
> > Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org>
> > 
> > diff --git a/include/linux/ring_buffer.h b/include/linux/ring_buffer.h
> > index 136ea09..55f9115 100644
> > --- a/include/linux/ring_buffer.h
> > +++ b/include/linux/ring_buffer.h
> > @@ -163,6 +163,8 @@ bool ring_buffer_empty_cpu(struct trace_buffer *buffer, int cpu);
> >   
> >   void ring_buffer_record_disable(struct trace_buffer *buffer);
> >   void ring_buffer_record_enable(struct trace_buffer *buffer);
> > +void ring_buffer_mutex_acquire(struct trace_buffer *buffer);
> > +void ring_buffer_mutex_release(struct trace_buffer *buffer);
> >   void ring_buffer_record_off(struct trace_buffer *buffer);
> >   void ring_buffer_record_on(struct trace_buffer *buffer);
> >   bool ring_buffer_record_is_on(struct trace_buffer *buffer);
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index 93ef0ab..638ec8f 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> > @@ -3632,6 +3632,25 @@ void ring_buffer_record_enable(struct trace_buffer *buffer)
> >   EXPORT_SYMBOL_GPL(ring_buffer_record_enable);
> >   
> >   /**
> > + * ring_buffer_mutex_acquire - prevent resetting of buffer
> > + * during resize
> > + */
> > +void ring_buffer_mutex_acquire(struct trace_buffer *buffer)
> > +{
> > +	mutex_lock(&buffer->mutex);
> > +}
> > +EXPORT_SYMBOL_GPL(ring_buffer_mutex_acquire);
> > +
> > +/**
> > + * ring_buffer_mutex_release - prevent resetting of buffer
> > + * during resize
> > + */
> > +void ring_buffer_mutex_release(struct trace_buffer *buffer)
> > +{
> > +	mutex_unlock(&buffer->mutex);
> > +}
> > +EXPORT_SYMBOL_GPL(ring_buffer_mutex_release);

I really do not like to export these.

> > +/**
> >    * ring_buffer_record_off - stop all writes into the buffer
> >    * @buffer: The ring buffer to stop writes to.
> >    *
> > @@ -4918,6 +4937,8 @@ void ring_buffer_reset(struct trace_buffer *buffer)
> >   	struct ring_buffer_per_cpu *cpu_buffer;
> >   	int cpu;
> >   
> > +	/* prevent another thread from changing buffer sizes */
> > +	mutex_lock(&buffer->mutex);
> >   	for_each_buffer_cpu(buffer, cpu) {
> >   		cpu_buffer = buffer->buffers[cpu];
> >   
> > @@ -4936,6 +4957,7 @@ void ring_buffer_reset(struct trace_buffer *buffer)
> >   		atomic_dec(&cpu_buffer->record_disabled);
> >   		atomic_dec(&cpu_buffer->resize_disabled);
> >   	}
> > +	mutex_unlock(&buffer->mutex);
> >   }
> >   EXPORT_SYMBOL_GPL(ring_buffer_reset);
> >   
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index f40d850..392e9aa 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -2006,6 +2006,8 @@ void tracing_reset_online_cpus(struct array_buffer *buf)
> >   	if (!buffer)
> >   		return;
> >   
> > +	ring_buffer_mutex_acquire(buffer);
> > +
> >   	ring_buffer_record_disable(buffer);

Hmm, why do we disable here as it gets disabled again in the call to
ring_buffer_reset_online_cpus()? Perhaps we don't need to disable the
buffer here. The only difference is that we have:

 buf->time_start = buffer_ftrace_now(buf, buf->cpu);

And that the above disables the entire buffer, whereas the reset only
resets individual ones.

But I don't think that will make any difference.

-- Steve


> >   
> >   	/* Make sure all commits have finished */
> > @@ -2016,6 +2018,8 @@ void tracing_reset_online_cpus(struct array_buffer *buf)
> >   	ring_buffer_reset_online_cpus(buffer);
> >   
> >   	ring_buffer_record_enable(buffer);
> > +
> > +	ring_buffer_mutex_release(buffer);
> >   }
> >   
> >   /* Must have trace_types_lock held */
> >   
> 

Re: [PATCH] trace: Fix race in trace_open and buffer resize call

[Gaurav Kohli]

Hi Steven,
thanks for reply.

On 9/14/2020 9:49 PM, Steven Rostedt wrote:
 > On Mon, 14 Sep 2020 10:00:50 +0530
 > Gaurav Kohli <gkohli@codeaurora.org> wrote:
 >
 >> Hi Steven,
 >>
 >> Please let us know, if below change looks good.
 >> Or let us know some other way to solve this.
 >>
 >> Thanks,
 >> Gaurav
 >>
 >>
 >
 > Hmm, for some reason, I don't see this in my INBOX, but it shows up in my
 > LKML folder. :-/
 >
 >


 >>> +void ring_buffer_mutex_release(struct trace_buffer *buffer)
 >>> +{
 >>> +    mutex_unlock(&buffer->mutex);
 >>> +}
 >>> +EXPORT_SYMBOL_GPL(ring_buffer_mutex_release);
 >
 > I really do not like to export these.
 >

Actually available reader lock is not helping 
here(&cpu_buffer->reader_lock), So i took ring buffer mutex lock to 
resolve this(this came on 4.19/5.4), in latest tip it is trace buffer 
lock. Due to this i have exported api.
 >>> +/**
 >>>     * ring_buffer_record_off - stop all writes into the buffer
 >>>     * @buffer: The ring buffer to stop writes to.
 >>>     *
 >>> @@ -4918,6 +4937,8 @@ void ring_buffer_reset(struct trace_buffer 
*buffer)
 >>>        struct ring_buffer_per_cpu *cpu_buffer;
 >>>        int cpu;
 >>>    +    /* prevent another thread from changing buffer sizes */
 >>> +    mutex_lock(&buffer->mutex);
 >>>        for_each_buffer_cpu(buffer, cpu) {
 >>>            cpu_buffer = buffer->buffers[cpu];
 >>>    @@ -4936,6 +4957,7 @@ void ring_buffer_reset(struct trace_buffer 
*buffer)
 >>>            atomic_dec(&cpu_buffer->record_disabled);
 >>>            atomic_dec(&cpu_buffer->resize_disabled);
 >>>        }
 >>> +    mutex_unlock(&buffer->mutex);
 >>>    }
 >>>    EXPORT_SYMBOL_GPL(ring_buffer_reset);
 >>>    diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
 >>> index f40d850..392e9aa 100644
 >>> --- a/kernel/trace/trace.c
 >>> +++ b/kernel/trace/trace.c
 >>> @@ -2006,6 +2006,8 @@ void tracing_reset_online_cpus(struct 
array_buffer *buf)
 >>>        if (!buffer)
 >>>            return;
 >>>    +    ring_buffer_mutex_acquire(buffer);
 >>> +
 >>>        ring_buffer_record_disable(buffer);
 >
 > Hmm, why do we disable here as it gets disabled again in the call to
 > ring_buffer_reset_online_cpus()? Perhaps we don't need to disable the
You mean cpu_buffer->reader_lock in reset_disabled_cpu_buffer?
Actually reader lock is already there but this is not helping if 
tracing_open and ring_buffer_resize are running parallel on different cpus.

We are seeing below race mainly during removal of extra pages:

                                             ring_buffer_resize
                                            //Below portion of code
                                            //not under any lock
                                             nr_pages_to_update < 0
                                            init_list_head(new_pages)
                                            rb_update_pages


ring_buffer_resize
tracing_open
tracing_reset_online_cpus
ring_buffer_reset_cpu
                                           cpu_buffer_reset done
                                           //now lock started

                                           warning(nr_removed)

We are seeing cases like cpu buffer got reset due to tracing open in 
other call, and then seeing issue in  rb_remove_pages.

Similar case can come during rb_insert_pages as well:

rb_insert_pages(struct ring_buffer_per_cpu *cpu_buffer)
{
         struct list_head *pages = &cpu_buffer->new_pages;
         int retries, success;
//before lock cpu buffer may get reset in another cpu, due to which we 
are seeing infinite loop cases as new_pages pointer got reset in 
rb_reset_cpu.

         raw_spin_lock_irq(&cpu_buffer->reader_lock);

 > buffer here. The only difference is that we have:
 >
 >   buf->time_start = buffer_ftrace_now(buf, buf->cpu);
 >
 > And that the above disables the entire buffer, whereas the reset only
 > resets individual ones.
 >
 > But I don't think that will make any difference.
 >
 > -- Steve
 >


-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH] trace: Fix race in trace_open and buffer resize call

[Steven Rostedt]

On Tue, 15 Sep 2020 10:38:03 +0530
Gaurav Kohli <gkohli@codeaurora.org> wrote:

> 
>  >>> +void ring_buffer_mutex_release(struct trace_buffer *buffer)
>  >>> +{
>  >>> +    mutex_unlock(&buffer->mutex);
>  >>> +}
>  >>> +EXPORT_SYMBOL_GPL(ring_buffer_mutex_release);  
>  >
>  > I really do not like to export these.
>  >  
> 
> Actually available reader lock is not helping 
> here(&cpu_buffer->reader_lock), So i took ring buffer mutex lock to 
> resolve this(this came on 4.19/5.4), in latest tip it is trace buffer 
> lock. Due to this i have exported api.

I'm saying, why don't you take the buffer->mutex in the
ring_buffer_reset_online_cpus() function? And remove all the protection in
tracing_reset_online_cpus()?

void tracing_reset_online_cpus(struct array_buffer *buf)
{
	struct trace_buffer *buffer = buf->buffer;

	if (!buffer)
		return;

	buf->time_start = buffer_ftrace_now(buf, buf->cpu);

	ring_buffer_reset_online_cpus(buffer);
}

The reset_online_cpus() is already doing the synchronization, we don't need
to do it twice.

I believe commit b23d7a5f4a07 ("ring-buffer: speed up buffer resets by
avoiding synchronize_rcu for each CPU") made the synchronization in
tracing_reset_online_cpus() obsolete.

-- Steve

Re: [PATCH] trace: Fix race in trace_open and buffer resize call

[Gaurav Kohli]

On 9/15/2020 6:53 PM, Steven Rostedt wrote:
> On Tue, 15 Sep 2020 10:38:03 +0530
> Gaurav Kohli <gkohli@codeaurora.org> wrote:
> 
>>
>>   >>> +void ring_buffer_mutex_release(struct trace_buffer *buffer)
>>   >>> +{
>>   >>> +    mutex_unlock(&buffer->mutex);
>>   >>> +}
>>   >>> +EXPORT_SYMBOL_GPL(ring_buffer_mutex_release);
>>   >
>>   > I really do not like to export these.
>>   >
>>
>> Actually available reader lock is not helping
>> here(&cpu_buffer->reader_lock), So i took ring buffer mutex lock to
>> resolve this(this came on 4.19/5.4), in latest tip it is trace buffer
>> lock. Due to this i have exported api.
> 
> I'm saying, why don't you take the buffer->mutex in the
> ring_buffer_reset_online_cpus() function? And remove all the protection in
> tracing_reset_online_cpus()?

Yes, got your point. then we can avoid export. Actually we are seeing 
issue in older kernel like 4.19/4.14/5.4 and there below patch was not 
present in stable branches:

ommit b23d7a5f4a07 ("ring-buffer: speed up buffer resets by
 > avoiding synchronize_rcu for each CPU")

Actually i have also thought to take mutex lock in ring_buffer_reset_cpu
while doing individual cpu reset, but this could cause another problem:

Different cpu buffer may have different state, so i have taken lock in 
tracing_reset_online_cpus.
>
> void tracing_reset_online_cpus(struct array_buffer *buf)
> {
> 	struct trace_buffer *buffer = buf->buffer;
> 
> 	if (!buffer)
> 		return;
> 
> 	buf->time_start = buffer_ftrace_now(buf, buf->cpu);
> 
> 	ring_buffer_reset_online_cpus(buffer);
> }
> 
> The reset_online_cpus() is already doing the synchronization, we don't need
> to do it twice.
> 
> I believe commit b23d7a5f4a07 ("ring-buffer: speed up buffer resets by
> avoiding synchronize_rcu for each CPU") made the synchronization in
> tracing_reset_online_cpus() obsolete.
> 
> -- Steve
> 

Yes, with above patch no need to take lock in tracing_reset_online_cpus.
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH] trace: Fix race in trace_open and buffer resize call

[Steven Rostedt]

On Tue, 15 Sep 2020 22:53:32 +0530
Gaurav Kohli <gkohli@codeaurora.org> wrote:

> On 9/15/2020 6:53 PM, Steven Rostedt wrote:
> > On Tue, 15 Sep 2020 10:38:03 +0530
> > Gaurav Kohli <gkohli@codeaurora.org> wrote:
> >   
> >>  
> >>   >>> +void ring_buffer_mutex_release(struct trace_buffer *buffer)
> >>   >>> +{
> >>   >>> +    mutex_unlock(&buffer->mutex);
> >>   >>> +}
> >>   >>> +EXPORT_SYMBOL_GPL(ring_buffer_mutex_release);  
> >>   >
> >>   > I really do not like to export these.
> >>   >  
> >>
> >> Actually available reader lock is not helping
> >> here(&cpu_buffer->reader_lock), So i took ring buffer mutex lock to
> >> resolve this(this came on 4.19/5.4), in latest tip it is trace buffer
> >> lock. Due to this i have exported api.  
> > 
> > I'm saying, why don't you take the buffer->mutex in the
> > ring_buffer_reset_online_cpus() function? And remove all the protection in
> > tracing_reset_online_cpus()?  
> 
> Yes, got your point. then we can avoid export. Actually we are seeing 
> issue in older kernel like 4.19/4.14/5.4 and there below patch was not 
> present in stable branches:
> 
> ommit b23d7a5f4a07 ("ring-buffer: speed up buffer resets by
>  > avoiding synchronize_rcu for each CPU")  

If you mark this patch for stable, you can add:

Depends-on: b23d7a5f4a07 ("ring-buffer: speed up buffer resets by avoiding synchronize_rcu for each CPU")  

> 
> Actually i have also thought to take mutex lock in ring_buffer_reset_cpu
> while doing individual cpu reset, but this could cause another problem:

Hmm, I think we should also take the buffer lock in the reset_cpu() call
too, and modify tracing_reset_cpu() the same way.

> 
> Different cpu buffer may have different state, so i have taken lock in 
> tracing_reset_online_cpus.

Why would different states be an issue in synchronizing?

-- Steve

> >
> > void tracing_reset_online_cpus(struct array_buffer *buf)
> > {
> > 	struct trace_buffer *buffer = buf->buffer;
> > 
> > 	if (!buffer)
> > 		return;
> > 
> > 	buf->time_start = buffer_ftrace_now(buf, buf->cpu);
> > 
> > 	ring_buffer_reset_online_cpus(buffer);
> > }
> > 
> > The reset_online_cpus() is already doing the synchronization, we don't need
> > to do it twice.
> > 
> > I believe commit b23d7a5f4a07 ("ring-buffer: speed up buffer resets by
> > avoiding synchronize_rcu for each CPU") made the synchronization in
> > tracing_reset_online_cpus() obsolete.
> > 
> > -- Steve
> >   
> 
> Yes, with above patch no need to take lock in tracing_reset_online_cpus.

Re: [PATCH] trace: Fix race in trace_open and buffer resize call

[Gaurav Kohli]

On 9/15/2020 11:43 PM, Steven Rostedt wrote:

>>>> Actually available reader lock is not helping
>>>> here(&cpu_buffer->reader_lock), So i took ring buffer mutex lock to
>>>> resolve this(this came on 4.19/5.4), in latest tip it is trace buffer
>>>> lock. Due to this i have exported api.
>>>
>>> I'm saying, why don't you take the buffer->mutex in the
>>> ring_buffer_reset_online_cpus() function? And remove all the protection in
>>> tracing_reset_online_cpus()?
>>
>> Yes, got your point. then we can avoid export. Actually we are seeing
>> issue in older kernel like 4.19/4.14/5.4 and there below patch was not
>> present in stable branches:
>>
>> ommit b23d7a5f4a07 ("ring-buffer: speed up buffer resets by
>>   > avoiding synchronize_rcu for each CPU")
> 
> If you mark this patch for stable, you can add:
> 
> Depends-on: b23d7a5f4a07 ("ring-buffer: speed up buffer resets by avoiding synchronize_rcu for each CPU")
> 

Thanks Steven, Yes this needs to be back ported. I have tried this in 
5.4 but this need more patches like
13292494379f92f532de71b31a54018336adc589
tracing: Make struct ring_buffer less ambiguous

Instead of protecting all reset, can we do it individually like below:


+++ b/kernel/trace/ring_buffer.c
@@ -4838,7 +4838,9 @@ rb_reset_cpu(struct ring_buffer_per_cpu *cpu_buffer)
  static void reset_disabled_cpu_buffer(struct ring_buffer_per_cpu 
*cpu_buffer)
  {
         unsigned long flags;
+       struct trace_buffer *buffer = cpu_buffer->buffer;

+       mutex_lock(&buffer->mutex);
         raw_spin_lock_irqsave(&cpu_buffer->reader_lock, flags);

         if (RB_WARN_ON(cpu_buffer, local_read(&cpu_buffer->committing)))
@@ -4852,6 +4854,7 @@ static void reset_disabled_cpu_buffer(struct 
ring_buffer_per_cpu *cpu_buffer)

   out:
         raw_spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
+       mutex_unlock(&buffer->mutex);
  }

Please let me know, if above looks good, we will do testing with this.
And this we can directly use in older kernel as well in 
ring_buffer_reset_cpu.

>>
>> Actually i have also thought to take mutex lock in ring_buffer_reset_cpu
>> while doing individual cpu reset, but this could cause another problem:
> 
> Hmm, I think we should also take the buffer lock in the reset_cpu() call
> too, and modify tracing_reset_cpu() the same way.
> 

if we take above patch, then this is not required.
Please let me know for the approach.
>>
>> Different cpu buffer may have different state, so i have taken lock in
>> tracing_reset_online_cpus.
> 
> Why would different states be an issue in synchronizing?
> 
> -- Steve
> 

Yes, this should not be problem.
>>>
>>> void tracing_reset_online_cpus(struct array_buffer *buf)
>>> {
>>> 	struct trace_buffer *buffer = buf->buffer;
>>>
>>> 	if (!buffer)
>>> 		return;
>>>
>>> 	buf->time_start = buffer_ftrace_now(buf, buf->cpu);
>>>
>>> 	ring_buffer_reset_online_cpus(buffer);
>>> }
>>>
>>> The reset_online_cpus() is already doing the synchronization, we don't need
>>> to do it twice.
>>>
>>> I believe commit b23d7a5f4a07 ("ring-buffer: speed up buffer resets by
>>> avoiding synchronize_rcu for each CPU") made the synchronization in
>>> tracing_reset_online_cpus() obsolete.
>>>
>>> -- Steve
>>>    
>>
>> Yes, with above patch no need to take lock in tracing_reset_online_cpus.
> 

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH] trace: Fix race in trace_open and buffer resize call

[Gaurav Kohli]

On 9/16/2020 12:02 PM, Gaurav Kohli wrote:

>>>
>>> Yes, got your point. then we can avoid export. Actually we are seeing
>>> issue in older kernel like 4.19/4.14/5.4 and there below patch was not
>>> present in stable branches:
>>>
>>> ommit b23d7a5f4a07 ("ring-buffer: speed up buffer resets by
>>>   > avoiding synchronize_rcu for each CPU")
>>
>> If you mark this patch for stable, you can add:
>>
>> Depends-on: b23d7a5f4a07 ("ring-buffer: speed up buffer resets by 
>> avoiding synchronize_rcu for each CPU")
>>
> 
> Thanks Steven, Yes this needs to be back ported. I have tried this in 
> 5.4 but this need more patches like
> 13292494379f92f532de71b31a54018336adc589
> tracing: Make struct ring_buffer less ambiguous
> 
> Instead of protecting all reset, can we do it individually like below:
> 
> 
> +++ b/kernel/trace/ring_buffer.c
> @@ -4838,7 +4838,9 @@ rb_reset_cpu(struct ring_buffer_per_cpu *cpu_buffer)
>   static void reset_disabled_cpu_buffer(struct ring_buffer_per_cpu 
> *cpu_buffer)
>   {
>          unsigned long flags;
> +       struct trace_buffer *buffer = cpu_buffer->buffer;
> 
> +       mutex_lock(&buffer->mutex);
>          raw_spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
> 
>          if (RB_WARN_ON(cpu_buffer, local_read(&cpu_buffer->committing)))
> @@ -4852,6 +4854,7 @@ static void reset_disabled_cpu_buffer(struct 
> ring_buffer_per_cpu *cpu_buffer)
> 
>    out:
>          raw_spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
> +       mutex_unlock(&buffer->mutex);
>   }
> 

Hi Steven,
Not seeing issue with above patch in 5.4, Please let me know if above 
approach looks good to you, will raise patch for same.

Otherwise we will raise patch for older approach by marking depends on 
of below patch:
depends-on: b23d7a5f4a07 ("ring-buffer: speed up buffer resets by

Thanks,
Gaurav
> Please let me know, if above looks good, we will do testing with this.
> And this we can directly use in older kernel as well in 
> ring_buffer_reset_cpu.
> 
>>>
>>> Actually i have also thought to take mutex lock in ring_buffer_reset_cpu
>>> while doing individual cpu reset, but this could cause another problem:
>>
>> Hmm, I think we should also take the buffer lock in the reset_cpu() call
>> too, and modify tracing_reset_cpu() the same way.
>>
> 
> if we take above patch, then this is not required.
> Please let me know for the approach.
>>>
>>> Different cpu buffer may have different state, so i have taken lock in
>>> tracing_reset_online_cpus.
>>
>> Why would different states be an issue in synchronizing?
>>
>> -- Steve
>>
> 
> Yes, this should not be problem.


-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH] trace: Fix race in trace_open and buffer resize call

[Steven Rostedt]

Sorry for not replying sooner, my email is still rather full from my 10 day
vacation, and I'm still in "skimming" mode at looking at it.

On Wed, 16 Sep 2020 12:02:46 +0530
Gaurav Kohli <gkohli@codeaurora.org> wrote:


> >> Yes, got your point. then we can avoid export. Actually we are seeing
> >> issue in older kernel like 4.19/4.14/5.4 and there below patch was not
> >> present in stable branches:
> >>
> >> ommit b23d7a5f4a07 ("ring-buffer: speed up buffer resets by  
> >>   > avoiding synchronize_rcu for each CPU")  
> > 
> > If you mark this patch for stable, you can add:
> > 
> > Depends-on: b23d7a5f4a07 ("ring-buffer: speed up buffer resets by avoiding synchronize_rcu for each CPU")
> >   
> 
> Thanks Steven, Yes this needs to be back ported. I have tried this in 
> 5.4 but this need more patches like
> 13292494379f92f532de71b31a54018336adc589
> tracing: Make struct ring_buffer less ambiguous

No, that is not needed. That's just a trivial renaming of structures. Use
the old structure. Dependency is if the algorithm depends on the change.
Not cosmetic.

> 
> Instead of protecting all reset, can we do it individually like below:
> 
> 
> +++ b/kernel/trace/ring_buffer.c
> @@ -4838,7 +4838,9 @@ rb_reset_cpu(struct ring_buffer_per_cpu *cpu_buffer)
>   static void reset_disabled_cpu_buffer(struct ring_buffer_per_cpu 
> *cpu_buffer)
>   {
>          unsigned long flags;
> +       struct trace_buffer *buffer = cpu_buffer->buffer;
> 
> +       mutex_lock(&buffer->mutex);
>          raw_spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
> 
>          if (RB_WARN_ON(cpu_buffer, local_read(&cpu_buffer->committing)))
> @@ -4852,6 +4854,7 @@ static void reset_disabled_cpu_buffer(struct 
> ring_buffer_per_cpu *cpu_buffer)
> 
>    out:
>          raw_spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
> +       mutex_unlock(&buffer->mutex);
>   }
> 
> Please let me know, if above looks good, we will do testing with this.
> And this we can directly use in older kernel as well in 
> ring_buffer_reset_cpu.

No that will not work. You need the lock around the disabling of the
buffers and the synchronizing with RCU.

-- Steve

Re: [PATCH] trace: Fix race in trace_open and buffer resize call

[Gaurav Kohli]

Sorry for spam, saw some failure so sending mail again.

On 9/15/2020 6:53 PM, Steven Rostedt wrote:
 > On Tue, 15 Sep 2020 10:38:03 +0530
 > Gaurav Kohli <gkohli@codeaurora.org> wrote:
 >
 >>
 >>   >>> +void ring_buffer_mutex_release(struct trace_buffer *buffer)
 >>   >>> +{
 >>   >>> +    mutex_unlock(&buffer->mutex);
 >>   >>> +}
 >>   >>> +EXPORT_SYMBOL_GPL(ring_buffer_mutex_release);
 >>   >
 >>   > I really do not like to export these.
 >>   >
 >>
 >> Actually available reader lock is not helping
 >> here(&cpu_buffer->reader_lock), So i took ring buffer mutex lock to
 >> resolve this(this came on 4.19/5.4), in latest tip it is trace buffer
 >> lock. Due to this i have exported api.
 >
 > I'm saying, why don't you take the buffer->mutex in the
 > ring_buffer_reset_online_cpus() function? And remove all the 
protection in
 > tracing_reset_online_cpus()?

Yes, got your point. then we can avoid export. Actually we are seeing 
issue in older kernel like 4.19/4.14/5.4 and there below patch is not 
present in stable branches:

ommit b23d7a5f4a07 ("ring-buffer: speed up buffer resets by
 > avoiding synchronize_rcu for each CPU")

Actually i have also thought to take mutex lock in ring_buffer_reset_cpu
while doing individual cpu reset, but this could cause another problem:

Different cpu buffer may have different state, so i have taken lock in 
tracing_reset_online_cpus.
 >
 > void tracing_reset_online_cpus(struct array_buffer *buf)
 > {
 >     struct trace_buffer *buffer = buf->buffer;
 >
 >     if (!buffer)
 >         return;
 >
 >     buf->time_start = buffer_ftrace_now(buf, buf->cpu);
 >
 >     ring_buffer_reset_online_cpus(buffer);
 > }
 >
 > The reset_online_cpus() is already doing the synchronization, we 
don't need
 > to do it twice.
 >
 > I believe commit b23d7a5f4a07 ("ring-buffer: speed up buffer resets by
 > avoiding synchronize_rcu for each CPU") made the synchronization in
 > tracing_reset_online_cpus() obsolete.
 >
 > -- Steve
 >

Yes, with above patch no need to take lock in tracing_reset_online_cpus.
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.