Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] net: packetmmap: fix only tx timestamp on request
@ 2021-05-03 23:46 Richard Sanger
  2021-05-04  0:36 ` Willem de Bruijn
  0 siblings, 1 reply; 7+ messages in thread
From: Richard Sanger @ 2021-05-03 23:46 UTC (permalink / raw)
  To: netdev; +Cc: 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: packetmmap: fix only tx timestamp on request
  2021-05-03 23:46 [PATCH] net: packetmmap: fix only tx timestamp on request Richard Sanger
@ 2021-05-04  0:36 ` Willem de Bruijn
  2021-05-04  1:22   ` Richard Sanger
  0 siblings, 1 reply; 7+ messages in thread
From: Willem de Bruijn @ 2021-05-04  0:36 UTC (permalink / raw)
  To: Richard Sanger; +Cc: Network Development, Daniel Borkmann

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: packetmmap: fix only tx timestamp on request
  2021-05-04  0:36 ` Willem de Bruijn
@ 2021-05-04  1:22   ` Richard Sanger
  2021-05-04 14:45     ` Willem de Bruijn
  0 siblings, 1 reply; 7+ messages in thread
From: Richard Sanger @ 2021-05-04  1:22 UTC (permalink / raw)
  To: Willem de Bruijn; +Cc: Network Development, Daniel Borkmann

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: packetmmap: fix only tx timestamp on request
  2021-05-04  1:22   ` Richard Sanger
@ 2021-05-04 14:45     ` Willem de Bruijn
  2021-05-05  4:29       ` Richard Sanger
  0 siblings, 1 reply; 7+ messages in thread
From: Willem de Bruijn @ 2021-05-04 14:45 UTC (permalink / raw)
  To: Richard Sanger; +Cc: Willem de Bruijn, Network Development, Daniel Borkmann

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: packetmmap: fix only tx timestamp on request
  2021-05-04 14:45     ` Willem de Bruijn
@ 2021-05-05  4:29       ` Richard Sanger
  2021-05-05  5:09         ` Richard Sanger
  2021-05-06  1:23         ` Willem de Bruijn
  0 siblings, 2 replies; 7+ messages in thread
From: Richard Sanger @ 2021-05-05  4:29 UTC (permalink / raw)
  To: Willem de Bruijn; +Cc: Network Development, Daniel Borkmann

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: packetmmap: fix only tx timestamp on request
  2021-05-05  4:29       ` Richard Sanger
@ 2021-05-05  5:09         ` Richard Sanger
  2021-05-06  1:23         ` Willem de Bruijn
  1 sibling, 0 replies; 7+ messages in thread
From: Richard Sanger @ 2021-05-05  5:09 UTC (permalink / raw)
  To: Willem de Bruijn; +Cc: Network Development, Daniel Borkmann

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: packetmmap: fix only tx timestamp on request
  2021-05-05  4:29       ` Richard Sanger
  2021-05-05  5:09         ` Richard Sanger
@ 2021-05-06  1:23         ` Willem de Bruijn
  1 sibling, 0 replies; 7+ messages in thread
From: Willem de Bruijn @ 2021-05-06  1:23 UTC (permalink / raw)
  To: Richard Sanger; +Cc: Network Development, Daniel Borkmann

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-03 23:46 [PATCH] net: packetmmap: fix only tx timestamp on request Richard Sanger
2021-05-04  0:36 ` Willem de Bruijn
2021-05-04  1:22   ` Richard Sanger
2021-05-04 14:45     ` Willem de Bruijn
2021-05-05  4:29       ` Richard Sanger
2021-05-05  5:09         ` Richard Sanger
2021-05-06  1:23         ` Willem de Bruijn

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git