netdev - Re: [PATCH net-next v2 4/5] ptp: Support late timestamp determination

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:   Tue, 12 Apr 2022 21:24:10 +0200
From:   Gerhard Engleder <gerhard@...leder-embedded.com>
To:     Richard Cochran <richardcochran@...il.com>,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>
Cc:     yangbo.lu@....com, David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, mlichvar@...hat.com,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 4/5] ptp: Support late timestamp determination

> > > > > @@ -887,18 +885,28 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
> > > > >       if (shhwtstamps &&
> > > > >           (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
> > > > >           !skb_is_swtx_tstamp(skb, false_tstamp)) {
> > > > > +             rcu_read_lock();
> > > > > +             orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
> > > >
> > > > __sock_recv_timestamp() is hot path.
> > > >
> > > > No need to call dev_get_by_napi_id() for the vast majority of cases
> > > > using plain old MAC time stamping.
> > >
> > > Isn't dev_get_by_napi_id() called most of the time anyway in put_ts_pktinfo()?
> >
> > No.  Only when SOF_TIMESTAMPING_OPT_PKTINFO is requested.
>
> You are right, my fault.
>
> > > That's the reason for the removal of a separate flag, which signals the need to
> > > timestamp determination based on address/cookie. I thought there is no need
> > > for that flag, as netdev is already available later in the existing code.
> > >
> > > > Make this conditional on (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC).
> > >
> > > This flag tells netdev_get_tstamp() which timestamp is required. If it
> > > is not set, then
> > > netdev_get_tstamp() has to deliver the normal timestamp as always. But
> > > this normal
> > > timestamp is only available via address/cookie. So netdev_get_tstamp() must be
> > > called.
> >
> > It should be this:
> >
> > - normal, non-vclock:   use hwtstamps->hwtstamp directly
> > - vclock:               use slower path with lookup
> >
> > I don't see why you can't implement that.
>
> I will try to implement it that way.

I'm thinking about why there should be a slow path with lookup. If the
address/cookie
points to a defined data structure with two timestamps, then no lookup
for the phc or
netdev is necessary. It should be possible for every driver to
allocate a skbuff with enough
space for this structure in front of the received Ethernet frame. The
structure could be:

struct skb_inline_hwtstamps {
        ktime_t hwtstamp;
        ktime_t hwcstamp;
};

Actually my device and igc are storing the timestamps in front of the
received Ethernet
frame. In my opinion it is obvious to the store metadata of received
Ethernet frames in
front of it, because it eliminates the need for another DMA transfer.
What is your opinion
Vinicius?

Gerhard