netdev - Re: [PATCH net-next] net: stmmac: optimize locking around PTP clock reads

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 1 Feb 2022 18:54:15 +0100
From:   Yannick Vignon <yannick.vignon@....nxp.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Jose Abreu <joabreu@...opsys.com>,
        "David S. Miller" <davem@...emloft.net>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        netdev@...r.kernel.org, Vladimir Oltean <olteanv@...il.com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>, mingkai.hu@....com,
        Joakim Zhang <qiangqing.zhang@....com>,
        sebastien.laveze@....com, Yannick Vignon <yannick.vignon@....com>
Subject: Re: [PATCH net-next] net: stmmac: optimize locking around PTP clock
 reads

On 2/1/2022 6:42 AM, Jakub Kicinski wrote:
> On Fri, 28 Jan 2022 18:02:57 +0100 Yannick Vignon wrote:
>> Reading the PTP clock is a simple operation requiring only 2 register
>> reads. Under a PREEMPT_RT kernel, protecting those reads by a spin_lock is
>> counter-productive:
>>   * if the task is preempted in-between the 2 reads, the return time value
>> could become inconsistent,
>>   * if the 2nd task preempting the 1st has a higher prio but needs to
>> read time as well, it will require 2 context switches, which will pretty
>> much always be more costly than just disabling preemption for the duration
>> of the 2 reads.
>>
>> Improve the above situation by:
>> * replacing the PTP spinlock by a rwlock, and using read_lock for PTP
>> clock reads so simultaneous reads do not block each other,
> 
> Are you sure the reads don't latch the other register? Otherwise this
> code is buggy, it should check for wrap around. (e.g. during 1.99 ->
> 2.00 transition driver can read .99, then 2, resulting in 2.99).
> 

Well, we did observe the issue on another device (micro-controller, not 
running Linux) using the same IP, and we were wondering how the Linux 
driver could work and why we didn't observe the issue... I experimented 
again today, and I did observe the problem, so I guess we didn't try 
hard enough before. (this time I bypassed the kernel by doing tight read 
loops from user-space after mmap'ing the registers).
Going to add another commit to this patch-queue to fix that.

>> * protecting the register reads by local_irq_save/local_irq_restore, to
>> ensure the code is not preempted between the 2 reads, even with PREEMPT_RT.
> 
>>   	/* Get the TSSS value */
>>   	ns = readl(ioaddr + PTP_STNSR);
>>   	/* Get the TSS and convert sec time value to nanosecond */
>>   	ns += readl(ioaddr + PTP_STSR) * 1000000000ULL;