netdev - RE: [PATCH net] net: dsa: sja1105: Fix double delivery of TX timestamps to socket error queue

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]

Message-ID: <02874ECE860811409154E81DA85FBB58B26DF1C9@fmsmsx101.amr.corp.intel.com>
Date:   Tue, 17 Dec 2019 22:21:29 +0000
From:   "Keller, Jacob E" <jacob.e.keller@...el.com>
To:     Vladimir Oltean <olteanv@...il.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "jakub.kicinski@...ronome.com" <jakub.kicinski@...ronome.com>,
        "richardcochran@...il.com" <richardcochran@...il.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH net] net: dsa: sja1105: Fix double delivery of TX
 timestamps to socket error queue

> -----Original Message-----
> From: netdev-owner@...r.kernel.org <netdev-owner@...r.kernel.org> On
> Behalf Of Vladimir Oltean
> Sent: Tuesday, December 17, 2019 12:07 PM
> To: Keller, Jacob E <jacob.e.keller@...el.com>
> Cc: davem@...emloft.net; jakub.kicinski@...ronome.com;
> richardcochran@...il.com; f.fainelli@...il.com; vivien.didelot@...il.com;
> andrew@...n.ch; netdev@...r.kernel.org
> Subject: Re: [PATCH net] net: dsa: sja1105: Fix double delivery of TX timestamps
> to socket error queue
> 
> Hi Jake,
> 
> > I thought the point of SKBTX_IN_PROGRESS was to inform the stack that a
> timestamp was pending. By not setting it, you no longer do this.
> >
> > Maybe that has changed since the original implementation? Or am I
> misunderstanding this patch..?
> >
> 
> I am not quite sure what the point of SKBTX_IN_PROGRESS is. If you
> search through the kernel you will find a single occurrence right now
> that is supposed to do something (I don't understand what exactly)
> when there is a concurrent sw and hw timestamp.
> 

My understanding was that setting it prevented the stack from generating a SW timestamp if the hardware timestamp was going to be provided. Basically, this is because we would otherwise report the timestamp twice to applications that expect only one timestamp.

There were some patches from Miroslav that enabled optionally allowed the reporting of both SW and HW timestamps at the same time.

> > You're removing the sja1105 assignment, not the one from the gianfar. Hmm
> >
> > Ok, so the issue is that sja1105_ptp.c was incorrectly setting the flag.
> >
> > Would it make more sense for gianfar to set SKBTX_IN_PROGRESS, but then
> use some other indicator internally, so that other callers who set it don't cause
> the gianfar driver to behave incorrectly? I believe we handle it in the Intel drivers
> that way by storing the skb. Then we don't check the SKBTX_IN_PROGRESS later.
> 
> Yes, the point is that it should check priv->hwts_tx_en &&
> (skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS). That was my initial
> fix for the bug, but Richard argued that setting SKBTX_IN_PROGRESS in
> itself isn't needed.
> 

I'd trust Richard on this one for sure.

> There are many more drivers that are in principle broken with DSA PTP,
> since they don't even have the equivalent check for priv->hwts_tx_en.
> 

Right. I'm wondering what the correct fix would be so that we can fix the drivers and hopefully avoid introducing a similar issue in the future.

Thanks,
Jake

> >
> > Thanks,
> > Jake
> >
> >
> 
> Thanks,
> -Vladimir