Possible bug: SO_TIMESTAMPING 2.6.30+

I've searched both the LKML archives, and the patchsets for kernels since 2.6.30 to see if
this problem has been observed.

I have an Ethernet driver that supports hardware timestamping, and it has been working
swimmingly well for several months using the SO_TIMESTAMPING
infrastructure that
Patrick Ohly put in the kernel starting around 2.6.30.

What I've discovered, that really causes me to pull my hair out, is that packets that are
either AF_INET6 or AF_PACKET, *are not* getting Tx (transmit)
timestamps. Diving into
the driver, I've discovered that the sk_buffs associated with these
packets *dont* have
the appropriate skb_tx state in the skb:

              shtx = skb_tx(skb);
              if (shtx->hardware)
              {
                       /* Do the timestamping thing */
               }

Now the puzzling thing is that the socket that originates these packets has SO_TIMESTAMPING
turned on, and was created as an AF_INET6 socket, but carries both V4
and V6 traffic. The
V4 traffic skbs have shtx->hardware true appropriately, but the
V6 traffic doesn't. Using
Wireshark, I can clearly see the V6 packets leaving the interface, so
it's not like they're
getting routed somewhere else, and debugging in the driver *clearly*
shows that V4 packets
have shtx->hardware, while the V6 ones (from the SAME SOCKET) do not.

I know that Patrick Ohly has essentially moved on from doing the SO_TIMESTAMPING stuff, so
who's maintaining it now?

When I look at the V6 udp code, it seems to call the same udp_sendmsg() function that V4
uses, which appears to support Tx timestamping the way it should.

Any clues anyone?

I worked on it a month ago or so and have a patchset from Patick Ohly and some changes by me which fix software timestamping and make the ioctl interface to the hardware more flexible (keeping backwards compatibility). Patches are attached.
Still I never tried IPv6 and don't think the patches do anything about it.
It would be nice to know weather software tx timestamps work with/without the patches.

Christopher

From: Christopher Zimmermann <[email protected]>
Date: Mon, 17 Aug 2009 21:26:17 +0200
Subject: [PATCH 1/3] timecompare: use ARRAY_SIZE() macro

---
 kernel/time/timecompare.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index 71e7f1a..fe24977 100644
@@ -56,11 +56,11 @@ int timecompare_offset(struct timecompare *sync,
 	int index;
 	int num_samples = sync->num_samples;
 
-	if (num_samples > sizeof(buffer)/sizeof(buffer[0])) {
+	if (num_samples > ARRAY_SIZE(buffer)) {
 		samples = kmalloc(sizeof(*samples) * num_samples, GFP_ATOMIC);
 		if (!samples) {
 			samples = buffer;
-			num_samples = sizeof(buffer)/sizeof(buffer[0]);
+			num_samples = ARRAY_SIZE(buffer);
 		}
 	} else {
 		samples = buffer;

1.6.3.3

From: Christopher Zimmermann <[email protected]>
Date: Mon, 17 Aug 2009 21:27:50 +0200
Subject: [PATCH 2/3] net: software TX time stamping

This patch implements the software fallback to TX time stamping. The necessary access to the buffer and socket are secured by taking references before calling ndo_start_xmit().

That avoids race conditions (buffer remains available even if transmission completes before ndo_start_xmit() returns) and works even if the driver calls skb_orphan().

The caller of skb_tstamp_tx() is now responsible for providing the socket, which requires minor changes in users of the previous call:
add skb->sk as parameter to get the old behavior.

---
 drivers/net/igb/igb_main.c |    2 +-
 include/linux/skbuff.h     |   11 +++++---
 net/core/dev.c             |   65 ++++++++++++++++++++++++++++++++++++++++++++
 net/core/skbuff.c          |    4 +-
 4 files changed, 75 insertions(+), 7 deletions(-)

index adb09d3..3ce4d78 100644
@@ -4370,7 +4370,7 @@ static void igb_tx_hwtstamp(struct igb_adapter *adapter, struct sk_buff *skb)
 			shhwtstamps.hwtstamp = ns_to_ktime(ns);
 			shhwtstamps.syststamp =
 				timecompare_transform(&adapter->compare, ns);
-			skb_tstamp_tx(skb, &shhwtstamps);
+			skb_tstamp_tx(skb, skb->sk, &shhwtstamps);
 		}
 	}
 }

index f2c69a2..e29e43d 100644
@@ -1873,17 +1873,20 @@ static inline ktime_t net_invalid_timestamp(void)
 
 /**
  * skb_tstamp_tx - queue clone of skb with send time stamps
- * @orig_skb:	the original outgoing packet
+ * @skb:	the original outgoing packet
+ * @sk:		sending socket (either from skb->sk or previous sock_get()),
+ *		may be NULL
  * @hwtstamps:	hardware time stamps, may be NULL if not available
  *
- * If the skb has a socket associated, then this function clones the
+ * If the socket is available, then this function clones the
  * skb (thus sharing the actual data and optional structures), stores
  * the optional hardware time stamping information (if non NULL) or
  * generates a software time stamp (otherwise), then queues the clone
  * to the error queue of the socket.  Errors are silently ignored.
  */
-extern void skb_tstamp_tx(struct sk_buff *orig_skb,
-			struct skb_shared_hwtstamps *hwtstamps);
+extern void skb_tstamp_tx(struct sk_buff *skb,
+			  struct sock *sk,
+			  struct skb_shared_hwtstamps *hwtstamps);
 
 extern __sum16 __skb_checksum_complete_head(struct sk_buff *skb, int len);
 extern __sum16 __skb_checksum_complete(struct sk_buff *skb);

index 9e0597d..78ae3e9 100644
@@ -2971,9 +2971,9 @@ int skb_cow_data(struct sk_buff *skb, int tailbits, struct sk_buff **trailer)
 EXPORT_SYMBOL_GPL(skb_cow_data);
 
 void skb_tstamp_tx(struct sk_buff *orig_skb,
-		struct skb_shared_hwtstamps *hwtstamps)
+		   struct sock *sk,
+		   struct skb_shared_hwtstamps *hwtstamps)
 {
-	struct sock *sk = orig_skb->sk;
 	struct sock_exterr_skb *serr;
 	struct sk_buff *skb;
 	int err;

1.6.3.3

From: Christopher Zimmermann <madroach@sancho.(none)>
Date: Wed, 7 Oct 2009 17:28:26 +0200
Subject: [PATCH 3/3] Change kernel timestamping interface

Allow userspace to fine-tune hardware registers.
Stay backwards compatible to old userspace?

---
 .gitignore                   |    1 +
 drivers/net/igb/e1000_regs.h |   41 ++++++++++--------
 drivers/net/igb/igb_main.c   |   95 +++++++++++++++++++++++++----------------
 include/linux/net_tstamp.h   |   35 +++++++++++----
 4 files changed, 107 insertions(+), 65 deletions(-)

index b93fb7e..6fac88c 100644
@@ -32,6 +32,7 @@
 #
 # Top-level generic files
 #
+debian
 tags
 TAGS
 vmlinux

index a3b8546..7b069dd 100644
@@ -40,26 +40,43 @@ enum {
  */
 struct hwtstamp_config {
 	int flags;
-	int tx_type;
-	int rx_filter;
+	/* deprecated, only used for backwards compatibility when flags == 0 */
+	int tx_type;            /* put this into flags ? */
+	int rx_filter;          /* deprecated */
+	/* hardware dependend part starts here */
+	union {
+		struct {
+			unsigned short port;  /* for dst port filter, zero to disable */
+			unsigned short etype; /* for ethertype filter, zero to disable */
+			unsigned ptp1_control	:8; /* TSYNCRXCFG.CTRLT */
+			/* Careful! According to reference manual message ids
+			 * 2 and 3 always get timestamped. */
+			unsigned ptp2_id	:4; /* TSYNCRXCFG.MSGT */
+			unsigned ptp2_val	:4; /* TSYNCRXCFG.TRNSSPC */
+			unsigned ptp_type	:3; /* TSYNCRXCTL.Type */
+		} igb;
+	};
 };
 
-/* possible values for hwtstamp_config->tx_type */
+/* flags for hwtstamp_config */
 enum {
 	/*
-	 * No outgoing packet will need hardware time stamping;
-	 * should a packet arrive which asks for it, no hardware
-	 * time stamping will be done.
+	 * these first two values are the only allowed values in
+	 * deprecated hwtstamp_config->tx_type
 	 */
-	HWTSTAMP_TX_OFF,
-
 	/*
 	 * Enables hardware time stamping for outgoing packets;
 	 * the sender of the packet decides which are to be
 	 * time stamped by setting %SOF_TIMESTAMPING_TX_SOFTWARE
 	 * before sending the packet.
+	 * If disabled no packet will be timestamped; even if it
+	 * asks for it.
 	 */
-	HWTSTAMP_TX_ON,
+	HWTSTAMP_TX_OFF = 0, /* only used in hwtstamp_config->tx_type */
+	HWTSTAMP_TX_ON = 1,
+	HWTSTAMP_TX = 1,
+
+	HWTSTAMP_RX = 2
 };
 
 /* possible values for hwtstamp_config->rx_filter */

1.6.3.3

I see some sock_put()/sock_hold() stuff in your second patch.

We removed these things on transmit path, please dont add them back...

If skb->sk is set, it probably also has a destructor and a reference already taken.

(This reference being on sk_wmem_alloc by the way, not on sk_refcnt)

You probably can try to change skb destructor ?

Have a look at commit 51f31cabe3ce5345b51e4a4f82138b38c4d5dc91. It adds support for SO_TIMESTAMPING in the transport layer, but only for the ipv4 variants.
Good news is, it does look like it was simple to do the same to ipv6/udp.c. Have fun.

ip: support for TX timestamps on UDP and RAW sockets

Instructions for time stamping outgoing packets are take from the socket layer and later copied into the new skb.

But only in ipv4/udp.c:

-------------------------------- net/ipv4/udp.c -------------------------------- index c47c989..4bd178a 100644 @@ -596,6 +596,7 @@ int udp_sendmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg, return -EOPNOTSUPP;

ipc.opt = NULL;
+ ipc.shtx.flags = 0;

if (up->pending) {
/*
@@ -643,6 +644,9 @@ int udp_sendmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg, ipc.addr = inet->saddr;

ipc.oif = sk->sk_bound_dev_if;
+ err = sock_tx_timestamp(msg, sk, &ipc.shtx);
+ if (err)
+ return err;
if (msg->msg_controllen) {
err = ip_cmsg_send(sock_net(sk), msg, &ipc); if (err)