linux-kernel - Re: [PATCH] trace: Fix race in trace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 22 Sep 2020 13:01:29 +0530
From:   Gaurav Kohli <gkohli@...eaurora.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     mingo@...hat.com, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH] trace: Fix race in trace_open and buffer resize call



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.