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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Mon, 31 Jan 2022 21:42:00 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Yannick Vignon <yannick.vignon@....nxp.com>
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 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).

> * 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;