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

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

[Richard Sanger]

The packetmmap tx ring should only return timestamps if requested,
as documented. This allows compatibility with non-timestamp aware
user-space code which checks tp_status == TP_STATUS_AVAILABLE;
not expecting additional timestamp flags to be set.

Signed-off-by: Richard Sanger <rsanger@wand.net.nz>
---
 net/packet/af_packet.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index ba96db1..b69805e 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -422,7 +422,8 @@ static __u32 tpacket_get_timestamp(struct sk_buff *skb, struct timespec64 *ts,
 	    ktime_to_timespec64_cond(shhwtstamps->hwtstamp, ts))
 		return TP_STATUS_TS_RAW_HARDWARE;
 
-	if (ktime_to_timespec64_cond(skb->tstamp, ts))
+	if ((flags & SOF_TIMESTAMPING_SOFTWARE) &&
+	    ktime_to_timespec64_cond(skb->tstamp, ts))
 		return TP_STATUS_TS_SOFTWARE;
 
 	return 0;
@@ -2340,7 +2341,10 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
 
 	skb_copy_bits(skb, 0, h.raw + macoff, snaplen);
 
-	if (!(ts_status = tpacket_get_timestamp(skb, &ts, po->tp_tstamp)))
+	/* always timestamp; prefer an existing software timestamp */
+	ts_status = tpacket_get_timestamp(skb, &ts,
+					  po->tp_tstamp | SOF_TIMESTAMPING_SOFTWARE);
+	if (!ts_status)
 		ktime_get_real_ts64(&ts);
 
 	status |= ts_status;
-- 
2.7.4

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

[Willem de Bruijn]

On Mon, May 3, 2021 at 8:04 PM Richard Sanger <rsanger@wand.net.nz> wrote:
>
> The packetmmap tx ring should only return timestamps if requested,
> as documented. This allows compatibility with non-timestamp aware
> user-space code which checks tp_status == TP_STATUS_AVAILABLE;
> not expecting additional timestamp flags to be set.

This is an established interface.

Passing the status goes back to 2013, since commit b9c32fb27170
("packet: if hw/sw ts enabled in rx/tx ring, report which ts we got").

Passing a timestamp itself in tp_sec/tp_usec goes back to before git,
probably to the introduction of the ring.

I don't think we can change this now. That will likely break
applications that have come to expect current behavior.

Is it documented somewhere that the ring works differently? Or are you
referring to the general SO_TIMESTAMPING behavior, which is a separate
timestamp interface.

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

[Richard Sanger]

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.

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

Thanks,
Richard


On Tue, May 4, 2021 at 12:36 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Mon, May 3, 2021 at 8:04 PM Richard Sanger <rsanger@wand.net.nz> wrote:
> >
> > The packetmmap tx ring should only return timestamps if requested,
> > as documented. This allows compatibility with non-timestamp aware
> > user-space code which checks tp_status == TP_STATUS_AVAILABLE;
> > not expecting additional timestamp flags to be set.
>
> This is an established interface.
>
> Passing the status goes back to 2013, since commit b9c32fb27170
> ("packet: if hw/sw ts enabled in rx/tx ring, report which ts we got").
>
> Passing a timestamp itself in tp_sec/tp_usec goes back to before git,
> probably to the introduction of the ring.
>
> I don't think we can change this now. That will likely break
> applications that have come to expect current behavior.
>
> Is it documented somewhere that the ring works differently? Or are you
> referring to the general SO_TIMESTAMPING behavior, which is a separate
> timestamp interface.

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

[Willem de Bruijn]

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

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

[Richard Sanger]

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 */

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

[Richard Sanger]

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

Sorry, I've confused a function name here, meant to say:
However, when ***tpacket_get_timestamp()*** is called within the packetmmap
code path skb->tstamp holds a valid time.

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

[Willem de Bruijn]

On Wed, May 5, 2021 at 7:42 PM Richard Sanger <rsanger@wand.net.nz> wrote:
>
> 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.

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.

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

[Richard Sanger]

I've had a chance to look into this further and have found where the
timestamp is added. Details are at the end of this message.

On Thu, May 6, 2021 at 1:23 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Wed, May 5, 2021 at 7:42 PM Richard Sanger <rsanger@wand.net.nz> wrote:
[...]
> >
> > 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.
>

Here's the output of putting a WARN_ON_ONCE(1) statement in
tpacket_destruct_skb, I don't believe it is related to the problem.

[   37.249629] RIP: 0010:tpacket_destruct_skb+0x24/0x60
[...]
[   37.249659] Call Trace:
[   37.249661]  <IRQ>
[   37.249666]  skb_release_head_state+0x44/0x90
[   37.249680]  skb_release_all+0x13/0x30
[   37.249684]  kfree_skb+0x2f/0xa0
[   37.249689]  llc_rcv+0x2e/0x360 [llc]
[   37.249698]  __netif_receive_skb_one_core+0x8f/0xa0
[   37.249707]  __netif_receive_skb+0x18/0x60
[   37.249710]  process_backlog+0xa9/0x160
[   37.249714]  __napi_poll+0x31/0x140
[   37.249717]  net_rx_action+0xde/0x210
[   37.249722]  __do_softirq+0xe0/0x29b
[   37.249737]  do_softirq+0x66/0x80
[   37.249747]  </IRQ>
[   37.249748]  __local_bh_enable_ip+0x50/0x60
[   37.249751]  __dev_queue_xmit+0x23a/0x6e0
[   37.249756]  dev_queue_xmit+0x10/0x20
[   37.249759]  packet_sendmsg+0x6b8/0x1c90
[   37.249763]  ? __drain_all_pages+0x150/0x1c0
[   37.249772]  sock_sendmsg+0x65/0x70
[   37.249778]  __sys_sendto+0x113/0x190
[   37.249783]  ? handle_mm_fault+0xda/0x2b0
[   37.249790]  ? exit_to_user_mode_prepare+0x3c/0x1e0
[   37.249800]  ? do_user_addr_fault+0x1d3/0x640
[   37.249805]  __x64_sys_sendto+0x29/0x30
[   37.249809]  do_syscall_64+0x40/0xb0
[   37.249816]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[   37.249820] RIP: 0033:0x7f43950d27ea

[...]

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

Okay, so the call path that adds the timestamp looks like this:

send() syscall triggers tpacket_snd() which calls the veth_xmit() hander.
In drivers/net/veth.c veth_xmit() calls veth_forward_skb() which then
calls netif_rx()/netif_rx_internal() in net/core/dev.c.
And finally, net_timestamp_check(netdev_tstamp_prequeue, skb) adds
the timestamp, netdev_tstamp_prequeue defaults to 1.

net_timestamp_check in its current form was added by 588f033075
("net: use jump_label for netstamp_needed ")
In the kernel since 3.3-rc1, so it looks like this issue has been present the
entire time. Pre-conditions are netstamp_needed_key and
netdev_tstamp_prequeue, so if either is false, timestamping won't happen
at this stage in the code.

Here's the call trace of where the timestamp is added

[  251.619538] Call Trace:
[  251.619550]  netif_rx+0x1b/0x60
[  251.619556]  veth_xmit+0x19d/0x230 [veth]
[  251.619563]  netdev_start_xmit+0x4a/0x8b
[  251.619566]  dev_hard_start_xmit.cold+0xc8/0x1d5
[  251.619569]  __dev_queue_xmit.cold+0xa3/0x12c
[  251.619572]  dev_queue_xmit+0x10/0x20
[  251.619575]  packet_sendmsg+0x6b8/0x1c90
[  251.619580]  ? __drain_all_pages+0x150/0x1c0
[  251.619588]  sock_sendmsg+0x65/0x70
[  251.619594]  __sys_sendto+0x113/0x190
[  251.619598]  ? handle_mm_fault+0xda/0x2b0
[  251.619604]  ? exit_to_user_mode_prepare+0x3c/0x1e0
[  251.619611]  ? do_user_addr_fault+0x1d3/0x640
[  251.619615]  __x64_sys_sendto+0x29/0x30
[  251.619618]  do_syscall_64+0x40/0xb0
[  251.619623]  entry_SYSCALL_64_after_hwframe+0x44/0xae

This appears to be reasonable, but I don't know what the expected behaviour
is. Should this timestamp still be cleared before returning the sent skb?

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

[Willem de Bruijn]

On Tue, May 11, 2021 at 6:11 PM Richard Sanger <rsanger@wand.net.nz> wrote:
>
> I've had a chance to look into this further and have found where the
> timestamp is added. Details are at the end of this message.
>
> On Thu, May 6, 2021 at 1:23 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Wed, May 5, 2021 at 7:42 PM Richard Sanger <rsanger@wand.net.nz> wrote:
> [...]
> > >
> > > 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.
> >
>
> Here's the output of putting a WARN_ON_ONCE(1) statement in
> tpacket_destruct_skb, I don't believe it is related to the problem.

It might be, if this is indeed an llc (ETH_P_802_2) packet that was
inserted with tpacket.

veth calls netif_rx, which enqueues the packet to the percpu queue for
later processing in softirq context using process_backlog.

>
> [   37.249629] RIP: 0010:tpacket_destruct_skb+0x24/0x60
> [...]
> [   37.249659] Call Trace:
> [   37.249661]  <IRQ>
> [   37.249666]  skb_release_head_state+0x44/0x90
> [   37.249680]  skb_release_all+0x13/0x30
> [   37.249684]  kfree_skb+0x2f/0xa0
> [   37.249689]  llc_rcv+0x2e/0x360 [llc]
> [   37.249698]  __netif_receive_skb_one_core+0x8f/0xa0
> [   37.249707]  __netif_receive_skb+0x18/0x60
> [   37.249710]  process_backlog+0xa9/0x160
> [   37.249714]  __napi_poll+0x31/0x140
> [   37.249717]  net_rx_action+0xde/0x210
> [   37.249722]  __do_softirq+0xe0/0x29b
> [   37.249737]  do_softirq+0x66/0x80
> [   37.249747]  </IRQ>
> [   37.249748]  __local_bh_enable_ip+0x50/0x60
> [   37.249751]  __dev_queue_xmit+0x23a/0x6e0
> [   37.249756]  dev_queue_xmit+0x10/0x20
> [   37.249759]  packet_sendmsg+0x6b8/0x1c90
> [   37.249763]  ? __drain_all_pages+0x150/0x1c0
> [   37.249772]  sock_sendmsg+0x65/0x70
> [   37.249778]  __sys_sendto+0x113/0x190
> [   37.249783]  ? handle_mm_fault+0xda/0x2b0
> [   37.249790]  ? exit_to_user_mode_prepare+0x3c/0x1e0
> [   37.249800]  ? do_user_addr_fault+0x1d3/0x640
> [   37.249805]  __x64_sys_sendto+0x29/0x30
> [   37.249809]  do_syscall_64+0x40/0xb0
> [   37.249816]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> [   37.249820] RIP: 0033:0x7f43950d27ea
>
> [...]
>
> > I think we need to understand exactly what goes on before we apply a
> > patch. It might just be papering over the problem otherwise.
>
> Okay, so the call path that adds the timestamp looks like this:
>
> send() syscall triggers tpacket_snd() which calls the veth_xmit() hander.
> In drivers/net/veth.c veth_xmit() calls veth_forward_skb() which then
> calls netif_rx()/netif_rx_internal() in net/core/dev.c.
> And finally, net_timestamp_check(netdev_tstamp_prequeue, skb) adds
> the timestamp, netdev_tstamp_prequeue defaults to 1.
>
> net_timestamp_check in its current form was added by 588f033075
> ("net: use jump_label for netstamp_needed ")
> In the kernel since 3.3-rc1, so it looks like this issue has been present the
> entire time. Pre-conditions are netstamp_needed_key and
> netdev_tstamp_prequeue, so if either is false, timestamping won't happen
> at this stage in the code.

Thanks. That's as I expected.

We cannot suppress the timestamp assignment by net_timestamp_check, as
it may be expected by the receive process.

I don't immediately see a way to clear/ignore the field in
tpacket_destruct_skb if the field was set from a different network
namespace than the one in which the packet socket inserted the packet.

So your suggested patch to suppress spurious tx timestamps if software
timestamping reporting is not enabled SGTM.


> Here's the call trace of where the timestamp is added
>
> [  251.619538] Call Trace:
> [  251.619550]  netif_rx+0x1b/0x60
> [  251.619556]  veth_xmit+0x19d/0x230 [veth]
> [  251.619563]  netdev_start_xmit+0x4a/0x8b
> [  251.619566]  dev_hard_start_xmit.cold+0xc8/0x1d5
> [  251.619569]  __dev_queue_xmit.cold+0xa3/0x12c
> [  251.619572]  dev_queue_xmit+0x10/0x20
> [  251.619575]  packet_sendmsg+0x6b8/0x1c90
> [  251.619580]  ? __drain_all_pages+0x150/0x1c0
> [  251.619588]  sock_sendmsg+0x65/0x70
> [  251.619594]  __sys_sendto+0x113/0x190
> [  251.619598]  ? handle_mm_fault+0xda/0x2b0
> [  251.619604]  ? exit_to_user_mode_prepare+0x3c/0x1e0
> [  251.619611]  ? do_user_addr_fault+0x1d3/0x640
> [  251.619615]  __x64_sys_sendto+0x29/0x30
> [  251.619618]  do_syscall_64+0x40/0xb0
> [  251.619623]  entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> This appears to be reasonable, but I don't know what the expected behaviour
> is. Should this timestamp still be cleared before returning the sent skb?