Date:   Tue, 4 May 2021 10:45:45 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Richard Sanger <rsanger@...d.net.nz>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Network Development <netdev@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH] net: packetmmap: fix only tx timestamp on request

On Mon, May 3, 2021 at 9:22 PM Richard Sanger <rsanger@...d.net.nz> wrote:
>
> Hi Willem,
>
> This is to match up with the documented behaviour; see the timestamping section
> at the bottom of
> https://www.kernel.org/doc/html/latest/networking/packet_mmap.html
>
> If no call to setsockopt(fd, SOL_PACKET, PACKET_TIMESTAMP, ...) is made then
> the tx path ring should not return timestamps, or timestamp flags set in
> tp_status.
>
> As noted in b9c32fb27170
> ("packet: if hw/sw ts enabled in rx/tx ring, report which ts we got")
> this is to retain backwards compatibility with old code.
>
> However, currently, a timestamp can be returned without setting
> PACKET_TIMESTAMP, in the case that skb->tstamp includes a timestamp.
> I only noticed this recently due to:
> aa4e689ed1 (veth: add software timestamping)
> which means skb->tstamp now includes a timestamp.
>
> The issue this bug causes for old/non-timestamp aware code is that tp_status
> may incorrectly have the TP_STATUS_TS_SOFTWARE flag set, so the documented
> check (tp_status == TP_STATUS_AVAILABLE) that a frame in the ring is free fails.
> Causing such code to hang infinitely.

Then this would need a

Fixes: b9c32fb27170 ("packet: if hw/sw ts enabled in rx/tx ring,
report which ts we got")

I don't fully follow the commit message in that patch for why enabling
this unconditionally on Tx is safe:

"
   This should not break
    anything for the following reasons: [..]

    ii) in TX ring path, time stamps with PACKET_TIMESTAMP
    socketoption are not available resp. had no effect except that the
    application setting this is buggy. Next to TP_STATUS_AVAILABLE, the
    user also should check for other flags such as TP_STATUS_WRONG_FORMAT
    to reclaim frames to the application. Thus, in case TX ts are turned
    off (default case), nothing happens to the application logic
"

But I think the point is that tx packets are not timestamped unless
skb_shinfo(skb)->tx_flags holds a timestamp request. Such as for
the software timestamps that veth can now generate:

"
static inline void skb_tx_timestamp(struct sk_buff *skb)
{
        skb_clone_tx_timestamp(skb);
        if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP)
                skb_tstamp_tx(skb, NULL);
}
"

So unless this packet socket has SOF_TIMESTAMPING_TX_SOFTWARE
configured, no timestamps should be recorded for its packets, as tx flag
SKBTX_SW_TSTAMP is not set.

> This patch corrects the behaviour for the tx path. But, doesn't change the
> behaviour on the rx path. The rx path still includes a timestamp (hence
> the patch always sets the SOF_TIMESTAMPING_SOFTWARE flag on rx).

Right, this patch suppresses reporting of any recorded timestamps. But
the system should already be suppressing recording of these
timestamps.

Assuming you discovered this with a real application: does it call
setsockopt SOL_SOCKET/SO_TIMESTAMPING at all?

It's safe to suppress on the reporting side as extra precaution against
spuriously timestamped packets. I just want to understand how these
timestamps are even recorded in the first place.

Small nit wrt the patch: the comment "/* always timestamp; prefer an
existing software timestamp */" states what the code does, but more
interesting would be why.

Powered by blists - more mailing lists