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

On Wed, May 5, 2021 at 7:42 PM Richard Sanger <rsanger@...d.net.nz> wrote:
>
> On Wed, May 5, 2021 at 2:45 AM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> >
> > 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
> [ ... ]
> >
> > 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.

The fixes tag is not the summary line.

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

Interesting. I had expected veth_xmit to trigger skb_orphan, which
calls the destructor.

But this is no longer true as of commit 9c4c325252c5 ("skbuff:
preserve sock reference when scrubbing the skb.").

As a result, I suppose the skb can enter the next namespace and be
timestamped there if receive timestamps are enabled (this is not
per-socket).

One way to verify, if you can easily recompile a kernel, is to add a
WARN_ON_ONCE(1) to tpacket_destruct_skb to see which path led up to
queuing the completion notification.

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

I think we need to understand exactly what goes on before we apply a
patch. It might just be papering over the problem otherwise.

Powered by blists - more mailing lists