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: <CA+h21hob3FmbQYyXMeLTtbHF1SeFO=LZVGyQt4jniS9-VXEO-w@mail.gmail.com>
Date:   Tue, 17 Dec 2019 22:07:03 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     "Keller, Jacob E" <jacob.e.keller@...el.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

Hi Jake,

On Tue, 17 Dec 2019 at 21:57, Keller, Jacob E <jacob.e.keller@...el.com> wrote:
>
> > -----Original Message-----
> > From: netdev-owner@...r.kernel.org <netdev-owner@...r.kernel.org> On
> > Behalf Of Vladimir Oltean
> > Sent: Monday, December 16, 2019 2:34 PM
> > To: davem@...emloft.net; jakub.kicinski@...ronome.com
> > Cc: richardcochran@...il.com; f.fainelli@...il.com; vivien.didelot@...il.com;
> > andrew@...n.ch; netdev@...r.kernel.org; Vladimir Oltean
> > <olteanv@...il.com>
> > Subject: [PATCH net] net: dsa: sja1105: Fix double delivery of TX timestamps to
> > socket error queue
> >
> > On the LS1021A-TSN board, it can be seen that under rare conditions,
> > ptp4l gets unexpected extra data in the event socket error queue.
> >
> > This is because the DSA master driver (gianfar) has TX timestamping
> > logic along the lines of:
> >
> > 1. In gfar_start_xmit:
> >       do_tstamp = (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> >                   priv->hwts_tx_en;
> >       (...)
> >       if (unlikely(do_tstamp))
> >               skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> > 2. Later in gfar_clean_tx_ring:
> >       if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
> >               (...)
> >               skb_tstamp_tx(skb, &shhwtstamps);
>
> I'm not sure I fully understand the problem.
>
> 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.

> 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.

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.

>
> Thanks,
> Jake
>
>

Thanks,
-Vladimir