[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+FuTSfE9wW55BbYRWNE1=XYAjG7gKVLLLbfAvB-4F+dL=8gHA@mail.gmail.com>
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