packet: clarify timestamp overflow

I tried to figure out what it would take to do a version 4 mmap packet socket interface to completely avoid the y2106 overflow problem. This is what I came up with, reusing most of the v3 code, except for the parts where we access the timestamps.

For kselftest, I'm adding support for testing v4 in addition to v1-v3, but the test currently does not look at the timestamps, so it won't check that the timestamp format actually works as intended, only that I didn't break the parts that worked in the v3 selftest.

Overall, this is more of a mess than I expected, so it's probably not worth doing a v4 format just for the timestamp, but the patch can serve as a reference for anyone that needs a new format for other reasons and fixes this along with the other changes.

Signed-off-by Arnd Bergmann <arnd at arndb.de>

---

Quoted 17 lines · Mon, Nov 27, 2017@05:19:25PM CET, [email protected] wrote:

I tried to figure out what it would take to do a version 4 mmap packet socket interface to completely avoid the y2106 overflow problem. This is what I came up with, reusing most of the v3 code, except for the parts where we access the timestamps.

For kselftest, I'm adding support for testing v4 in addition to v1-v3, but the test currently does not look at the timestamps, so it won't check that the timestamp format actually works as intended, only that I didn't break the parts that worked in the v3 selftest.

Overall, this is more of a mess than I expected, so it's probably not worth doing a v4 format just for the timestamp, but the patch can serve as a reference for anyone that needs a new format for other reasons and fixes this along with the other changes.

Signed-off-by Arnd Bergmann <arnd at arndb.de>

---

[...]

include/uapi/linux/if_packet.h struct tpacket_block_desc { · line 273

Quoted 7 lines

include/uapi/linux/if_packet.h +2 -1

@@ -250,7 +269,8 @@ struct tpacket_block_desc {
 enum tpacket_versions {
      TPACKET_V1,
      TPACKET_V2,
-      TPACKET_V3
+      TPACKET_V3,
+      TPACKET_V4,

I wonder with how many versions are we going to eventually end up with :O

Quoted 22 lines · Jiri Pirko

Quoted 17 lines · Mon, Nov 27, 2017@05:19:25PM CET, [email protected] wrote:

I tried to figure out what it would take to do a version 4 mmap packet socket interface to completely avoid the y2106 overflow problem. This is what I came up with, reusing most of the v3 code, except for the parts where we access the timestamps.

For kselftest, I'm adding support for testing v4 in addition to v1-v3, but the test currently does not look at the timestamps, so it won't check that the timestamp format actually works as intended, only that I didn't break the parts that worked in the v3 selftest.

Overall, this is more of a mess than I expected, so it's probably not worth doing a v4 format just for the timestamp, but the patch can serve as a reference for anyone that needs a new format for other reasons and fixes this along with the other changes.

Signed-off-by Arnd Bergmann <arnd at arndb.de>

---

[...]

Quoted 9 lines

include/uapi/linux/if_packet.h struct tpacket_block_desc { · line 273

Quoted 7 lines

include/uapi/linux/if_packet.h +2 -1

@@ -250,7 +269,8 @@ struct tpacket_block_desc {
 enum tpacket_versions {
      TPACKET_V1,
      TPACKET_V2,
-      TPACKET_V3
+      TPACKET_V3,
+      TPACKET_V4,

I wonder with how many versions are we going to eventually end up with :O

There already is an effort to come up with a new AF_PACKET V4 [1]. We should make sure that any new interface does not have the Y2038/Y2106 issue. But, if a new version is being developed and that subsumes all existing use cases, then there probably is no need for another version that is a very small diff to V3.

If adding support for existing applications is useful, another approach would be to add a new socket option that changes the semantics for the two u32 fields in each of V1, V2 and V3 to hold nsec. Add a single check after filling in those structs whether the option is set and, if so, overwrite the two fields.

[1] https://lwn.net/Articles/737947/

Quoted 5 lines

There already is an effort to come up with a new AF_PACKET V4 [1]. We should make sure that any new interface does not have the Y2038/Y2106 issue. But, if a new version is being developed and that subsumes all existing use cases, then there probably is no need for another version that is a very small diff to V3.

Ah, perfect, that's good timing. Adding Björn to Cc here.

Quoted 6 lines

If adding support for existing applications is useful, another approach would be to add a new socket option that changes the semantics for the two u32 fields in each of V1, V2 and V3 to hold nsec. Add a single check after filling in those structs whether the option is set and, if so, overwrite the two fields.

Quoted 1 line

[1] https://lwn.net/Articles/737947/

I don't think that's necessary. As long as the V4 capabilities are a superset of V1-V3, we should be able to just require all users to move to V4 (or later) in the next 89 years, and make sure that they use unsigned seconds if they care about 2038.

Arnd

2017-11-27 21:51 GMT+01:00 Arnd Bergmann <arnd at arndb.de>:
[...]

Quoted 6 lines

Quoted 5 lines

There already is an effort to come up with a new AF_PACKET V4 [1]. We should make sure that any new interface does not have the Y2038/Y2106 issue. But, if a new version is being developed and that subsumes all existing use cases, then there probably is no need for another version that is a very small diff to V3.

Quoted 2 lines

Ah, perfect, that's good timing. Adding Björn to Cc here.

Unfortunately, for the Y2038/Y2106 cases, we'll be (as a result of netdevconf discussions) moving the AF_PACKET V4 implementation to a separate, new, address/packet family.

Quoted 6 lines

Quoted 6 lines

If adding support for existing applications is useful, another approach would be to add a new socket option that changes the semantics for the two u32 fields in each of V1, V2 and V3 to hold nsec. Add a single check after filling in those structs whether the option is set and, if so, overwrite the two fields.

Quoted 7 lines

Quoted 1 line

[1] https://lwn.net/Articles/737947/

I don't think that's necessary. As long as the V4 capabilities are a superset of V1-V3, we should be able to just require all users to move to V4 (or later) in the next 89 years, and make sure that they use unsigned seconds if they care about 2038.

Given that V4 wont be around for AF_PACKET -- at least not in the shape of our patches -- Willem's suggestion is probably a good way forward.

This is a second attempt to allow 64-bit timestamps in packet sockets,

The implementation is fairly straightforward, but I'm less sure about the interface. Using SOF_TIMESTAMPING_* flags in PACKET_TIMESTAMP is a bit odd already since most of the other flags make no sense here. Adding two more flags that only make sense for packet sockets but not the normal SO_TIMESTAMPING option on other sockets makes this even more confusing.

On Tue, Nov 28, 2017 at 8:14 AM, Arnd Bergmann <[email protected]> wrote:

Unfortunately, we're already stuck with SOL_PACKET/PACKET_TIMESTAMP accepting SOF_TIMESTAMPING_RAW_HARDWARE.

Perhaps we can define a new PF_PACKET specific enum where the equivalent option has the same value, so is backwards compatible:

enum {
PACKET_TIMESTAMP_ORIG = 0,
PACKET_TIMESTAMP_ZERO = 1 << 0,
PACKET_TIMESTAMP_NS64 = 1 << 1,
PACKET_TIMESTAMP_HW = 1 << 6
};

and BUILD_BUG_ON(PACKET_TIMESTAMP_RAW != SOF_TIMESTAMPING_RAW_HARDWARE) to document the dependency.

At high level, the code looks great to me, itself.

The implementation is fairly straightforward, but I'm less sure about the interface. Using SOF_TIMESTAMPING_* flags in PACKET_TIMESTAMP is a bit odd already since most of the other flags make no sense here. Adding two more flags that only make sense for packet sockets but not the normal SO_TIMESTAMPING option on other sockets makes this even more confusing.

From: Arnd Bergmann <[email protected]>
Date: Tue, 28 Nov 2017 14:14:05 +0100

Quoted 5 lines

The implementation is fairly straightforward, but I'm less sure about the interface. Using SOF_TIMESTAMPING_* flags in PACKET_TIMESTAMP is a bit odd already since most of the other flags make no sense here. Adding two more flags that only make sense for packet sockets but not the normal SO_TIMESTAMPING option on other sockets makes this even more confusing.

We unfortunately never enforced any checking whatsoever of the PACKET_TIMESTAMP mask the user gives us, we accept anything.

That makes any changes in this area effectively a grenade ready to go off potentially at any moment.

We can't add new checks without potentially making existing apps stop working. And at least theoretically if we add new bits it is possible for an existing app passing those bits in by accident to start behaving improperly.

I know it sounds like overkill for this, but maybe we can add a new socket option for the SKIP and 64-bit stuff. That would be %100 safe.

Does this mean you think the general idea of an extended interface for 64-bit timestamps is useful for traditional packet sockets? I think that was still an open question, though we seem to be getting closer to consensus on the implementation and the interface that it should use if we want it.

From: Arnd Bergmann <[email protected]>
Date: Tue, 28 Nov 2017 21:02:05 +0100

Quoted 5 lines

Does this mean you think the general idea of an extended interface for 64-bit timestamps is useful for traditional packet sockets? I think that was still an open question, though we seem to be getting closer to consensus on the implementation and the interface that it should use if we want it.

If it can be done reasonably easy, which you patches seem to indicate is the case, I have no objections to extending packet socket for 64-bit timestamps.

I hope that AF_CAPTURE will be designed in such a way that all apps can migrate to it, and I will be making sure it is implemented appropriately with that in mind.

But I don't think AF_CAPTURE should block your work here.

As I noticed in my previous patch to remove the 'timespec' usage in the packet socket, the timestamps in the packet socket are slightly inefficient as they convert a nanosecond value into seconds/nanoseconds or seconds/microseconds.

This adds two new socket options for the timestamp to resolve that:

PACKET_SKIPTIMESTAMP sets a flag to indicate whether to generate timestamps at all. When this is set, all timestamps are hardcoded to zero, which saves a few cycles for the conversion and the access of the hardware clocksource. The idea was taken from pktgen, which has an F_NO_TIMESTAMP option for the same purpose.

PACKET_TIMESTAMP_NS64 changes the interpretation of the time stamp fields:
instead of having 32 bits for seconds plus 32 bits for nanoseconds or microseconds, we now always send down 64 bits worth of nanoseconds when this flag is set.

Link https://patchwork.kernel.org/patch/10077199/

Suggested-by Willem de Bruijn <[email protected]>

Signed-off-by Arnd Bergmann <[email protected]>

{

net/packet/af_packet.c +17 -5

+       struct packet_sock *po = pkt_sk(skb->sk);
        struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
+       ktime_t stamp;
+       u32 type;
+
+       if (po->tp_skiptstamp)
+               return 0;
 
        if (shhwtstamps &&
-           (flags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
-           ktime_to_timespec64_cond(shhwtstamps->hwtstamp, ts))
-               return TP_STATUS_TS_RAW_HARDWARE;
+           (po->tp_tstamp & SOF_TIMESTAMPING_RAW_HARDWARE) &&
+           shhwtstamps->hwtstamp) {
+               stamp = shhwtstamps->hwtstamp;
+               type = TP_STATUS_TS_RAW_HARDWARE;
+       } else if (skb->tstamp) {
+               stamp = skb->tstamp;
+               type = TP_STATUS_TS_SOFTWARE;
+       } else {
+               return 0;
+       }
 
-       if (ktime_to_timespec64_cond(skb->tstamp, ts))
-               return TP_STATUS_TS_SOFTWARE;
+       if (po->tp_tstamp_ns64) {

static __u32 __packet_set_timestamp(struct packet_sock *po, void *frame,
struct sk_buff *skb)

 {
        union tpacket_uhdr h;

net/packet/af_packet.c +2 -3

-       struct timespec64 ts;
-       __u32 ts_status;
+       __u32 ts_status, hi, lo;
 
-       if (!(ts_status = tpacket_get_timestamp(skb, &ts, po->tp_tstamp)))
+       if (!(ts_status = tpacket_get_timestamp(skb, &hi, &lo, po->tp_tstamp)))

Quoted 13 lines · Arnd Bergmann

As I noticed in my previous patch to remove the 'timespec' usage in the packet socket, the timestamps in the packet socket are slightly inefficient as they convert a nanosecond value into seconds/nanoseconds or seconds/microseconds.

This adds two new socket options for the timestamp to resolve that:

PACKET_SKIPTIMESTAMP sets a flag to indicate whether to generate timestamps at all. When this is set, all timestamps are hardcoded to zero, which saves a few cycles for the conversion and the access of the hardware clocksource. The idea was taken from pktgen, which has an F_NO_TIMESTAMP option for the same purpose.

Quoted 8 lines

PACKET_TIMESTAMP_NS64 changes the interpretation of the time stamp fields:
instead of having 32 bits for seconds plus 32 bits for nanoseconds or microseconds, we now always send down 64 bits worth of nanoseconds when this flag is set.

Link https://patchwork.kernel.org/patch/10077199/

Suggested-by Willem de Bruijn <[email protected]>

Signed-off-by Arnd Bergmann <[email protected]>

This works. Another option would be to add a PACKET_TIMESTAMP_EX with the semantics we discussed previously + fail hard when any undefined bits are set. I don't feel strong either way, we don't intend to extend further.

If taking this approach, it might be good to split into separate patches, one for each flag?

net/packet/af_packet.c static int __packet_get_status(struct packet_sock *po, void *frame) · line 443

Quoted 3 lines

net/packet/af_packet.c +1 -1

-static __u32 tpacket_get_timestamp(struct sk_buff *skb, struct timespec64 *ts,
+static __u32 tpacket_get_timestamp(struct sk_buff *skb, __u32 *hi, __u32 *lo,
                                   unsigned int flags)

Argument flags is no longer used.

net/packet/af_packet.c static int __packet_get_status(struct packet_sock *po, void *frame) · line 474

Quoted 8 lines

net/packet/af_packet.c +7 -1

-       return 0;
+               *hi = upper_32_bits(ns);
+               *lo = lower_32_bits(ns);
+       } else {
+               struct timespec64 ts = ktime_to_timespec64(stamp);
+
+               *hi = ts.tv_sec;
+               if (po->tp_version == TPACKET_V1)

Very minor: may want to invert test to make newer the protocols the likely branch.

Quoted 10 lines

static __u32 __packet_set_timestamp(struct packet_sock *po, void *frame,
struct sk_buff *skb)

 {
        union tpacket_uhdr h;

net/packet/af_packet.c +2 -3

-       struct timespec64 ts;
-       __u32 ts_status;
+       __u32 ts_status, hi, lo;
 
-       if (!(ts_status = tpacket_get_timestamp(skb, &ts, po->tp_tstamp)))
+       if (!(ts_status = tpacket_get_timestamp(skb, &hi, &lo, po->tp_tstamp)))

net/packet/af_packet.c static int __packet_get_status(struct packet_sock *po, void *frame) · line 475

Quoted 10 lines

net/packet/af_packet.c +0 -7

                return 0;
 
        h.raw = frame;
-       /*
-        * versions 1 through 3 overflow the timestamps in y2106, since they
-        * all store the seconds in a 32-bit unsigned integer.
-        * If we create a version 4, that should have a 64-bit timestamp,
-        * either 64-bit seconds + 32-bit nanoseconds, or just 64-bit
-        * nanoseconds.
-        */

Probably no need to introduce this in patch 1/2 when removing it in 2/2.

Quoted 4 lines

net/packet/af_packet.c +0 -0

@@ -2191,8 +2226,8 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
        unsigned long status = TP_STATUS_USER;
        unsigned short macoff, netoff, hdrlen;
        struct sk_buff *copy_skb = NULL;

net/packet/af_packet.c static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, · line 2230

Quoted 3 lines

net/packet/af_packet.c +1 -1

-       struct timespec64 ts;
        __u32 ts_status;
+       __u32 hi, lo;

since this function is not time-specific, the context of hi and lo is not immediately obvious here. tstamp_hi, tstamp_lo? Or even __u32 tstamp[2] and have tpacket_get_timestamp and packet_get_time take one fewer argument.

Thanks for the review! Any suggestions for how to do the testing? If you have existing test cases, could you give my next version a test run to see if there are any regressions and if the timestamps work as expected?

I see that there are test cases in tools/testing/selftests/net/, but none of them seem to use the time stamps so far, and I'm not overly familiar with how it works in the details to extend it in a meaningful way.

Quoted 7 lines

Thanks for the review! Any suggestions for how to do the testing? If you have existing test cases, could you give my next version a test run to see if there are any regressions and if the timestamps work as expected?

I see that there are test cases in tools/testing/selftests/net/, but none of them seem to use the time stamps so far, and I'm not overly familiar with how it works in the details to extend it in a meaningful way.

I could not find any good tests for this interface, either. The only user of the interface I found was a little tool I wrote a few years ago that compares timestamps at multiple points in the transmit path for latency measurement [1]. But it may be easier to just write a new test under tools/testing/selftests/net for this purpose. I can help with that, too, if you want.

Quoted 8 lines · Willem de Bruijn

Quoted 7 lines

Thanks for the review! Any suggestions for how to do the testing? If you have existing test cases, could you give my next version a test run to see if there are any regressions and if the timestamps work as expected?

I see that there are test cases in tools/testing/selftests/net/, but none of them seem to use the time stamps so far, and I'm not overly familiar with how it works in the details to extend it in a meaningful way.

Quoted 6 lines

I could not find any good tests for this interface, either. The only user of the interface I found was a little tool I wrote a few years ago that compares timestamps at multiple points in the transmit path for latency measurement [1]. But it may be easier to just write a new test under tools/testing/selftests/net for this purpose. I can help with that, too, if you want.

Thanks, that would be great!

Quoted 17 lines · Arnd Bergmann

Quoted 14 lines · Willem de Bruijn

Quoted 7 lines

Thanks for the review! Any suggestions for how to do the testing? If you have existing test cases, could you give my next version a test run to see if there are any regressions and if the timestamps work as expected?

I see that there are test cases in tools/testing/selftests/net/, but none of them seem to use the time stamps so far, and I'm not overly familiar with how it works in the details to extend it in a meaningful way.

I could not find any good tests for this interface, either. The only user of the interface I found was a little tool I wrote a few years ago that compares timestamps at multiple points in the transmit path for latency measurement [1]. But it may be easier to just write a new test under tools/testing/selftests/net for this purpose. I can help with that, too, if you want.

Quoted 1 line

Thanks, that would be great!

I'll reply to this thread with git send-email with an extension to tools/testing/selftests/net/psock_tpacket.c.

Quoted 8 lines · Björn Töpel

2017-11-27 21:51 GMT+01:00 Arnd Bergmann <arnd at arndb.de>:
[...]

Quoted 6 lines

Quoted 5 lines

There already is an effort to come up with a new AF_PACKET V4 [1]. We should make sure that any new interface does not have the Y2038/Y2106 issue. But, if a new version is being developed and that subsumes all existing use cases, then there probably is no need for another version that is a very small diff to V3.

Quoted 6 lines

Quoted 2 lines

Ah, perfect, that's good timing. Adding Björn to Cc here.

Unfortunately, for the Y2038/Y2106 cases, we'll be (as a result of netdevconf discussions) moving the AF_PACKET V4 implementation to a separate, new, address/packet family.

Ok, I see.

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

If adding support for existing applications is useful, another approach would be to add a new socket option that changes the semantics for the two u32 fields in each of V1, V2 and V3 to hold nsec. Add a single check after filling in those structs whether the option is set and, if so, overwrite the two fields.

Quoted 7 lines

Quoted 1 line

[1] https://lwn.net/Articles/737947/

I don't think that's necessary. As long as the V4 capabilities are a superset of V1-V3, we should be able to just require all users to move to V4 (or later) in the next 89 years, and make sure that they use unsigned seconds if they care about 2038.

Quoted 3 lines

Given that V4 wont be around for AF_PACKET -- at least not in the shape of our patches -- Willem's suggestion is probably a good way forward.

That leaves one question: should we do that now, or wait until some other reason for a V4 comes up? I don't mind creating another patch for this, just want to get a feeling of whether the API clutter is worth it when we have a way out that works until y2106 (at which point we run into other problems as well).

From that point of view, and if we want to offer a Y2106 proof

Quoted 12 lines · Arnd Bergmann

Quoted 11 lines · Björn Töpel

2017-11-27 21:51 GMT+01:00 Arnd Bergmann <arnd at arndb.de>:
[...]

Quoted 8 lines

Quoted 5 lines

There already is an effort to come up with a new AF_PACKET V4 [1]. We should make sure that any new interface does not have the Y2038/Y2106 issue. But, if a new version is being developed and that subsumes all existing use cases, then there probably is no need for another version that is a very small diff to V3.

Ah, perfect, that's good timing. Adding Björn to Cc here.

Quoted 5 lines

Quoted 3 lines

Unfortunately, for the Y2038/Y2106 cases, we'll be (as a result of netdevconf discussions) moving the AF_PACKET V4 implementation to a separate, new, address/packet family.

Ok, I see.

Does it matter whether the replacement is a new version or a new packet family?

Quoted 18 lines

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

If adding support for existing applications is useful, another approach would be to add a new socket option that changes the semantics for the two u32 fields in each of V1, V2 and V3 to hold nsec. Add a single check after filling in those structs whether the option is set and, if so, overwrite the two fields.

Quoted 7 lines

Quoted 1 line

[1] https://lwn.net/Articles/737947/

I don't think that's necessary. As long as the V4 capabilities are a superset of V1-V3, we should be able to just require all users to move to V4 (or later) in the next 89 years, and make sure that they use unsigned seconds if they care about 2038.

Quoted 3 lines

Given that V4 wont be around for AF_PACKET -- at least not in the shape of our patches -- Willem's suggestion is probably a good way forward.

Quoted 5 lines

That leaves one question: should we do that now, or wait until some other reason for a V4 comes up? I don't mind creating another patch for this, just want to get a feeling of whether the API clutter is worth it when we have a way out that works until y2106 (at which point we run into other problems as well).

I don't expect that we'll have another packet version independent from the work that Björn is doing. The choice to implement using a new packet family is given by the complexity of the existing code, especially the various locking mechanisms.

From that point of view, and if we want to offer a Y2106 proof AF_PACKET independent from the above, no reason to wait.