Re: [PATCH] net: packetmmap: fix only tx timestamp on request

[Richard Sanger <rsanger@wand.net.nz>]

On Wed, May 5, 2021 at 2:45 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Mon, May 3, 2021 at 9:22 PM Richard Sanger <rsanger@wand.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
[ ... ]
>
> Then this would need a
>
> Fixes: b9c32fb27170 ("packet: if hw/sw ts enabled in rx/tx ring,
> report which ts we got")

ack, I will resubmit the patch with that as the summary line of the commit
message.

> I don't fully follow the commit message in that patch for why enabling
> this unconditionally on Tx is safe:
>
[...]
>
> 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:
>

I came to the same understanding, tx timestamping should be disabled unless
the code calls setsockopt SOL_SOCKET/SO_TIMESTAMPING.

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

You are right, that check is working correctly, I'm mistaken on the trigger of
this behaviour. It doesn't appear related to aa4e689ed1
(veth: add software timestamping). In fact, this bug is present in Linux 4.19
the version before that patch was added, and likely earlier versions too.

I've just verified using printk() that after the call to skb_tx_timestamp(skb)
in veth_xmit() skb->tstamp == 0 as expected.

However, when skb_tx_timestamp() is called within the packetmmap code path
skb->tstamp holds a valid time.

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

Yes, I can confirm my code does not setsockopt SO_TIMESTAMPING
Here is the filtered output of strace

# strace ./test-live -c 1 ring:veth0 2>&1  | grep sock
socket(AF_PACKET, SOCK_RAW, htons(0 /* ETH_P_??? */)) = 3
setsockopt(3, SOL_PACKET, PACKET_VERSION, [1], 4) = 0
setsockopt(3, SOL_PACKET, PACKET_TX_RING, {tp_block_size=1048576,
tp_block_nr=1, tp_frame_size=4096, tp_frame_nr=256}, 16) = 0
socket(AF_UNIX, SOCK_DGRAM|SOCK_CLOEXEC, 0) = 4

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

Agreed, if this isn't expected behaviour, how skb->tstamp is getting filled
with a timestamp remains a mystery to me. I'll report back if I find the
source.

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

Absolutely, I'll replace it with something along the lines of
/* always timestamp; prefer an existing software timestamp taken closer to
   the time of capture */