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] [day] [month] [year] [list]

Message-ID: <20220201111230.7141ee8f@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Tue, 1 Feb 2022 11:12:30 -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 Tue, 1 Feb 2022 18:54:15 +0100 Yannick Vignon wrote:
> 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.

That's a fix tho, it needs to be a separate change containing a Fixes
tag and targeted at the netdev/net tree. We'd first apply that, and
then the optimizations on top of it into the net-next tree.