| Message ID | 20181211202520.16799-7-deepa.kernel@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | net: y2038-safe socket timestamps | expand |
On Tue, Dec 11, 2018 at 3:30 PM Deepa Dinamani <deepa.kernel@gmail.com> wrote: > > Add SO_TIMESTAMP_NEW and SO_TIMESTAMPNS_NEW variants of > socket timestamp options. > These are the y2038 safe versions of the SO_TIMESTAMP_OLD > and SO_TIMESTAMPNS_OLD for all architectures. > > Note that the format of scm_timestamping.ts[0] is not changed > in this patch. > > Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com> > Cc: jejb@parisc-linux.org > Cc: ralf@linux-mips.org > Cc: rth@twiddle.net > Cc: linux-alpha@vger.kernel.org > Cc: linux-mips@linux-mips.org > Cc: linux-parisc@vger.kernel.org > Cc: linux-rdma@vger.kernel.org > Cc: netdev@vger.kernel.org > Cc: sparclinux@vger.kernel.org > --- > arch/alpha/include/uapi/asm/socket.h | 15 ++++++- > arch/mips/include/uapi/asm/socket.h | 14 ++++++- > arch/parisc/include/uapi/asm/socket.h | 14 ++++++- > arch/sparc/include/uapi/asm/socket.h | 14 ++++++- > include/linux/skbuff.h | 18 +++++++++ > include/net/sock.h | 1 + > include/uapi/asm-generic/socket.h | 15 ++++++- > net/core/sock.c | 51 ++++++++++++++++++------ > net/ipv4/tcp.c | 57 +++++++++++++++++++-------- > net/rds/af_rds.c | 8 +++- > net/rds/recv.c | 16 +++++++- > net/socket.c | 47 ++++++++++++++++------ > 12 files changed, 216 insertions(+), 54 deletions(-) > > diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h > index 00e45c80e574..352e3dc0b3d9 100644 > --- a/arch/alpha/include/uapi/asm/socket.h > +++ b/arch/alpha/include/uapi/asm/socket.h > @@ -3,6 +3,7 @@ > #define _UAPI_ASM_SOCKET_H > > #include <asm/sockios.h> > +#include <asm/bitsperlong.h> > > /* For setsockopt(2) */ > /* > @@ -110,12 +111,22 @@ > > #define SO_TIMESTAMP_OLD 29 > #define SO_TIMESTAMPNS_OLD 35 > + > #define SO_TIMESTAMPING_OLD 37 > > +#define SO_TIMESTAMP_NEW 62 > +#define SO_TIMESTAMPNS_NEW 63 > + > #if !defined(__KERNEL__) > > -#define SO_TIMESTAMP SO_TIMESTAMP_OLD > -#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD > +#if __BITS_PER_LONG == 64 > +#define SO_TIMESTAMP SO_TIMESTAMP_OLD > +#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD > +#else > +#define SO_TIMESTAMP (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMP_OLD : SO_TIMESTAMP_NEW) > +#define SO_TIMESTAMPNS (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMPNS_OLD : SO_TIMESTAMPNS_NEW) > +#endif > + This is not platform specific. Perhaps it can be deduplicated. The interface expects callers to include <linux/socket.h>, not <asm/socket.h> directly. So perhaps it can go there? > -/* Similar to __sock_recv_timestamp, but does not require an skb */ > -static void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk, > - struct scm_timestamping *tss) > +static void tcp_recv_sw_timestamp(struct msghdr *msg, const struct sock *sk, struct timespec64 *ts) > { > - struct __kernel_old_timeval tv; > - bool has_timestamping = false; > + int new_tstamp = sock_flag(sk, SOCK_TSTAMP_NEW); > + > + if (ts->tv_sec || ts->tv_nsec) { > + if (sock_flag(sk, SOCK_RCVTSTAMPNS)) { > + if (new_tstamp) { > + struct __kernel_timespec kts = {ts->tv_sec, ts->tv_nsec}; > + > + put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMPNS_NEW, > + sizeof(kts), &kts); > + } else { > + struct timespec ts_old = timespec64_to_timespec(*ts); > > - if (tss->ts[0].tv_sec || tss->ts[0].tv_nsec) { > - if (sock_flag(sk, SOCK_RCVTSTAMP)) { > - if (sock_flag(sk, SOCK_RCVTSTAMPNS)) { > put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMPNS_OLD, > - sizeof(tss->ts[0]), &tss->ts[0]); > + sizeof(ts), &ts_old); > + } > + } else if (sock_flag(sk, SOCK_RCVTSTAMP)) { > + if (new_tstamp) { > + struct __kernel_sock_timeval stv; > + > + stv.tv_sec = ts->tv_sec; > + stv.tv_usec = ts->tv_nsec / 1000; > + > + put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMP_NEW, > + sizeof(stv), &stv); > } else { > - tv.tv_sec = tss->ts[0].tv_sec; > - tv.tv_usec = tss->ts[0].tv_nsec / 1000; > + struct __kernel_old_timeval tv; > > + tv.tv_sec = ts->tv_sec; > + tv.tv_usec = ts->tv_nsec / 1000; > put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMP_OLD, > sizeof(tv), &tv); > } > } > - > - if (sk->sk_tsflags & SOF_TIMESTAMPING_SOFTWARE) > - has_timestamping = true; > - else > - tss->ts[0] = (struct timespec) {0}; > } > +} > + > +/* Similar to __sock_recv_timestamp, but does not require an skb */ > +static void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk, > + struct scm_timestamping *tss) > +{ > + bool has_timestamping = false; > + struct timespec64 ts = timespec_to_timespec64(tss->ts[0]); > + > + tcp_recv_sw_timestamp(msg, sk, &ts); > + > + if (sk->sk_tsflags & SOF_TIMESTAMPING_SOFTWARE) > + has_timestamping = true; > + else > + tss->ts[0] = (struct timespec) {0}; > > if (tss->ts[2].tv_sec || tss->ts[2].tv_nsec) { > if (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) This did not address yet the previous comments on consistency and unnecessary code churn. The existing logic to differentiate SO_TIMESTAMP from SO_TIMESTAMPNS in both tcp_recv_timestamp and __sock_recv_timestamp is if (sock_flag(sk, SOCK_RCVTSTAMP)) { if (sock_flag(sk, SOCK_RCVTSTAMPNS)) /* timespec case */ else /* timeval case */ } A new level of nesting needs to be added to differentiate .._OLD from .._NEW. Even if massively changing the original functions, please do so consistently, either if (sock_flag(sk, SOCK_RCVTSTAMP)) { if (sock_flag(sk, SOCK_TSTAMP_NEW) { /* new code */ } else { if (sock_flag(sk, SOCK_RCVTSTAMPNS)) /* timespec case */ else /* timeval case */ } } or if (sock_flag(sk, SOCK_RCVTSTAMP)) { if (sock_flag(sk, SOCK_TSTAMP_NEW) { if (sock_flag(sk, SOCK_RCVTSTAMPNS)) /* new timespec case */ else /* timespec case */ } else { if (sock_flag(sk, SOCK_RCVTSTAMPNS)) /* new timespec case */ else /* timespec case */ } } But not one variant in one function and one in the other. Deep nesting is hard to follow and, once again, massive code changes (even indentations) make git blame harder to use. So where possible, try to avoid both and just insert a branch to a new function for the .._NEW cases instead: if (sock_flag(sk, SOCK_RCVTSTAMP)) { + if (sock_flag(sk, SOCK_TSTAMP_NEW) + __sock_recv_timestamp_new(..); - if (sock_flag(sk, SOCK_RCVTSTAMPNS)) + else if (sock_flag(sk, SOCK_RCVTSTAMPNS)) /* timespec case */ else /* timeval case */ } and leave the rest of the function unmodified. > +static void sock_recv_sw_timestamp(struct msghdr *msg, struct sock *sk, > + struct sk_buff *skb) > +{ > + if (sock_flag(sk, SOCK_TSTAMP_NEW)) { > + if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) { > + struct __kernel_sock_timeval tv; > + > + skb_get_new_timestamp(skb, &tv); > + put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMP_NEW, > + sizeof(tv), &tv); > + } else { > + struct __kernel_timespec ts; > + > + skb_get_new_timestampns(skb, &ts); > + put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMPNS_NEW, > + sizeof(ts), &ts); > + } > + } In relation to previous: note the different branching approach to tcp_recv_timestamp. > + if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) { > + struct __kernel_old_timeval tv; > + > + skb_get_timestamp(skb, &tv); > + put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMP_OLD, > + sizeof(tv), &tv); > + } else { > + struct timespec ts; > + > + skb_get_timestampns(skb, &ts); > + put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMPNS_OLD, > + sizeof(ts), &ts); > + } > +} > /* > * called from sock_recv_timestamp() if sock_flag(sk, SOCK_RCVTSTAMP) > */ > @@ -718,19 +750,8 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, > false_tstamp = 1; > } > > - if (need_software_tstamp) { > - if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) { > - struct __kernel_old_timeval tv; > - skb_get_timestamp(skb, &tv); > - put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMP_OLD, > - sizeof(tv), &tv); > - } else { > - struct timespec ts; > - skb_get_timestampns(skb, &ts); > - put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMPNS_OLD, > - sizeof(ts), &ts); > - } > - } > + if (need_software_tstamp) > + sock_recv_sw_timestamp(msg, sk, skb); > > memset(&tss, 0, sizeof(tss)); > if ((sk->sk_tsflags & SOF_TIMESTAMPING_SOFTWARE) && > -- > 2.17.1 >
> This did not address yet the previous comments on consistency and > unnecessary code churn. > > The existing logic to differentiate SO_TIMESTAMP from SO_TIMESTAMPNS > in both tcp_recv_timestamp and __sock_recv_timestamp is > > if (sock_flag(sk, SOCK_RCVTSTAMP)) { > if (sock_flag(sk, SOCK_RCVTSTAMPNS)) > /* timespec case */ > else > /* timeval case */ > } > > A new level of nesting needs to be added to differentiate .._OLD from .._NEW. > > Even if massively changing the original functions, please do so > consistently, either > > if (sock_flag(sk, SOCK_RCVTSTAMP)) { > if (sock_flag(sk, SOCK_TSTAMP_NEW) { > /* new code */ > } else { > if (sock_flag(sk, SOCK_RCVTSTAMPNS)) > /* timespec case */ > else > /* timeval case */ > } > } This first example is wrong. I meant if (sock_flag(sk, SOCK_RCVTSTAMP)) { if (sock_flag(sk, SOCK_RCVTSTAMPNS)) { if (sock_flag(sk, SOCK_TSTAMP_NEW) /* new code */ else /* timespec case */ } else { if (sock_flag(sk, SOCK_TSTAMP_NEW) /* new code */ else /* timeval case */ } }
> > diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h > > index 00e45c80e574..352e3dc0b3d9 100644 > > --- a/arch/alpha/include/uapi/asm/socket.h > > +++ b/arch/alpha/include/uapi/asm/socket.h > > @@ -3,6 +3,7 @@ > > #define _UAPI_ASM_SOCKET_H > > > > #include <asm/sockios.h> > > +#include <asm/bitsperlong.h> > > > > /* For setsockopt(2) */ > > /* > > @@ -110,12 +111,22 @@ > > > > #define SO_TIMESTAMP_OLD 29 > > #define SO_TIMESTAMPNS_OLD 35 > > + > > #define SO_TIMESTAMPING_OLD 37 > > > > +#define SO_TIMESTAMP_NEW 62 > > +#define SO_TIMESTAMPNS_NEW 63 > > + > > #if !defined(__KERNEL__) > > > > -#define SO_TIMESTAMP SO_TIMESTAMP_OLD > > -#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD > > +#if __BITS_PER_LONG == 64 > > +#define SO_TIMESTAMP SO_TIMESTAMP_OLD > > +#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD > > +#else > > +#define SO_TIMESTAMP (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMP_OLD : SO_TIMESTAMP_NEW) > > +#define SO_TIMESTAMPNS (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMPNS_OLD : SO_TIMESTAMPNS_NEW) > > +#endif > > + > > This is not platform specific. Perhaps it can be deduplicated. The > interface expects callers to include <linux/socket.h>, not > <asm/socket.h> directly. So perhaps it can go there? I'm not following what you are saying here. Are you talking about in kernel users or userspace interface? Userspace should always include sys/socket.h according to the man page. I'm not sure if userspace can even include linux/socket.h directly. On my distribution this includes bits/socket.h which in turn includes asm/socket.h. Which file gets installed as asm/socket.h is defined per architecture in the kbuild file such as arch/ia64/include/uapi/asm/Kbuild (without series applied): generic-y += poll.h generic-y += sembuf.h generic-y += shmbuf.h generic-y += socket.h Also the new timestamp numbers being added are not the same for all architectures. So I'm not sure how this can be moved to linux/socket.h. > This did not address yet the previous comments on consistency and > unnecessary code churn. > > The existing logic to differentiate SO_TIMESTAMP from SO_TIMESTAMPNS > in both tcp_recv_timestamp and __sock_recv_timestamp is > > if (sock_flag(sk, SOCK_RCVTSTAMP)) { > if (sock_flag(sk, SOCK_RCVTSTAMPNS)) > /* timespec case */ > else > /* timeval case */ > } > > A new level of nesting needs to be added to differentiate .._OLD from .._NEW. > > Even if massively changing the original functions, please do so > consistently, either > > if (sock_flag(sk, SOCK_RCVTSTAMP)) { > if (sock_flag(sk, SOCK_TSTAMP_NEW) { > /* new code */ > } else { > if (sock_flag(sk, SOCK_RCVTSTAMPNS)) > /* timespec case */ > else > /* timeval case */ > } > } > > or > > if (sock_flag(sk, SOCK_RCVTSTAMP)) { > if (sock_flag(sk, SOCK_TSTAMP_NEW) { > if (sock_flag(sk, SOCK_RCVTSTAMPNS)) > /* new timespec case */ > else > /* timespec case */ > } else { > if (sock_flag(sk, SOCK_RCVTSTAMPNS)) > /* new timespec case */ > else > /* timespec case */ > } > } > > But not one variant in one function and one in the other. > > Deep nesting is hard to follow and, once again, massive code changes > (even indentations) make git blame harder to use. So where possible, > try to avoid both and just insert a branch to a new function for the > .._NEW cases instead: > > if (sock_flag(sk, SOCK_RCVTSTAMP)) { > + if (sock_flag(sk, SOCK_TSTAMP_NEW) > + __sock_recv_timestamp_new(..); > - if (sock_flag(sk, SOCK_RCVTSTAMPNS)) > + else if (sock_flag(sk, SOCK_RCVTSTAMPNS)) > /* timespec case */ > else > /* timeval case */ > } > > and leave the rest of the function unmodified. Ok, I will keep the functions consistent. -Deepa
On Fri, Dec 14, 2018 at 8:07 PM Deepa Dinamani <deepa.kernel@gmail.com> wrote: > > > > diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h > > > index 00e45c80e574..352e3dc0b3d9 100644 > > > --- a/arch/alpha/include/uapi/asm/socket.h > > > +++ b/arch/alpha/include/uapi/asm/socket.h > > > @@ -3,6 +3,7 @@ > > > #define _UAPI_ASM_SOCKET_H > > > > > > #include <asm/sockios.h> > > > +#include <asm/bitsperlong.h> > > > > > > /* For setsockopt(2) */ > > > /* > > > @@ -110,12 +111,22 @@ > > > > > > #define SO_TIMESTAMP_OLD 29 > > > #define SO_TIMESTAMPNS_OLD 35 > > > + > > > #define SO_TIMESTAMPING_OLD 37 > > > > > > +#define SO_TIMESTAMP_NEW 62 > > > +#define SO_TIMESTAMPNS_NEW 63 > > > + > > > #if !defined(__KERNEL__) > > > > > > -#define SO_TIMESTAMP SO_TIMESTAMP_OLD > > > -#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD > > > +#if __BITS_PER_LONG == 64 > > > +#define SO_TIMESTAMP SO_TIMESTAMP_OLD > > > +#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD > > > +#else > > > +#define SO_TIMESTAMP (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMP_OLD : SO_TIMESTAMP_NEW) > > > +#define SO_TIMESTAMPNS (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMPNS_OLD : SO_TIMESTAMPNS_NEW) > > > +#endif > > > + > > > > This is not platform specific. Perhaps it can be deduplicated. The > > interface expects callers to include <linux/socket.h>, not > > <asm/socket.h> directly. So perhaps it can go there? > > I'm not following what you are saying here. > > Are you talking about in kernel users or userspace interface? > > Userspace should always include sys/socket.h according to the man page. > I'm not sure if userspace can even include linux/socket.h directly. > On my distribution this includes bits/socket.h which in turn includes > asm/socket.h. I meant include/uapi/linux/socket.h. But you're right that that is not referenced from sys/socket.h. I do see a reference to it in my bits/socket.h /* Socket level message types. This must match the definitions in <linux/socket.h>. */ so perhaps the logic could be both there and in libc bits/socket.h. > Which file gets installed as asm/socket.h is defined per architecture > in the kbuild file such as > arch/ia64/include/uapi/asm/Kbuild (without series applied): > > generic-y += poll.h > generic-y += sembuf.h > generic-y += shmbuf.h > generic-y += socket.h > > Also the new timestamp numbers being added are not the same for all > architectures. > > So I'm not sure how this can be moved to linux/socket.h. Does that matter, as long as they are defined? This basic block is the same between all archs: -#define SO_TIMESTAMP SO_TIMESTAMP_OLD -#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD +#if __BITS_PER_LONG == 64 +#define SO_TIMESTAMP SO_TIMESTAMP_OLD +#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD +#else +#define SO_TIMESTAMP (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMP_OLD : SO_TIMESTAMP_NEW) +#define SO_TIMESTAMPNS (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMPNS_OLD : SO_TIMESTAMPNS_NEW) +#endif It might be too complex to coordinate changes between kernel and libc headers, in which case you're right that this just has to live in (each arch's) asm/socket.h directly.
On Sat, Dec 15, 2018 at 7:12 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Fri, Dec 14, 2018 at 8:07 PM Deepa Dinamani <deepa.kernel@gmail.com> wrote: > > > > > > diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h > > > > index 00e45c80e574..352e3dc0b3d9 100644 > > > > --- a/arch/alpha/include/uapi/asm/socket.h > > > > +++ b/arch/alpha/include/uapi/asm/socket.h > > > > @@ -3,6 +3,7 @@ > > > > #define _UAPI_ASM_SOCKET_H > > > > > > > > #include <asm/sockios.h> > > > > +#include <asm/bitsperlong.h> > > > > > > > > /* For setsockopt(2) */ > > > > /* > > > > @@ -110,12 +111,22 @@ > > > > > > > > #define SO_TIMESTAMP_OLD 29 > > > > #define SO_TIMESTAMPNS_OLD 35 > > > > + > > > > #define SO_TIMESTAMPING_OLD 37 > > > > > > > > +#define SO_TIMESTAMP_NEW 62 > > > > +#define SO_TIMESTAMPNS_NEW 63 > > > > + > > > > #if !defined(__KERNEL__) > > > > > > > > -#define SO_TIMESTAMP SO_TIMESTAMP_OLD > > > > -#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD > > > > +#if __BITS_PER_LONG == 64 > > > > +#define SO_TIMESTAMP SO_TIMESTAMP_OLD > > > > +#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD > > > > +#else > > > > +#define SO_TIMESTAMP (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMP_OLD : SO_TIMESTAMP_NEW) > > > > +#define SO_TIMESTAMPNS (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMPNS_OLD : SO_TIMESTAMPNS_NEW) > > > > +#endif > > > > + > > > > > > This is not platform specific. Perhaps it can be deduplicated. The > > > interface expects callers to include <linux/socket.h>, not > > > <asm/socket.h> directly. So perhaps it can go there? > > > > I'm not following what you are saying here. > > > > Are you talking about in kernel users or userspace interface? > > > > Userspace should always include sys/socket.h according to the man page. > > I'm not sure if userspace can even include linux/socket.h directly. > > On my distribution this includes bits/socket.h which in turn includes > > asm/socket.h. > > I meant include/uapi/linux/socket.h. > > But you're right that that is not referenced from sys/socket.h. > > I do see a reference to it in my bits/socket.h > > /* Socket level message types. This must match the definitions in > <linux/socket.h>. */ > > so perhaps the logic could be both there and in libc bits/socket.h. bits/socket.h cannot be included directly, and it's just how one of the libc implementations decided to do it. It doesn't even have to exist. > > Which file gets installed as asm/socket.h is defined per architecture > > in the kbuild file such as > > arch/ia64/include/uapi/asm/Kbuild (without series applied): > > > > generic-y += poll.h > > generic-y += sembuf.h > > generic-y += shmbuf.h > > generic-y += socket.h > > > > Also the new timestamp numbers being added are not the same for all > > architectures. > > > > So I'm not sure how this can be moved to linux/socket.h. > > Does that matter, as long as they are defined? This basic block is the > same between all archs: 3 reasons for not doing this: 1. We do not want to break userspace. If we move this to linux/socket.h all the userspace programs now have to include linux/socket.h or get this definition through a new libc. 2. All the socket options are together in the file asm/socket.h. It doesn't seem good for maintainability to move just a few bits elsewhere. 3. There are only 4 arches (after the series is applied) that have their own asm/socket.h. And, this is because there seems to be significant differences to asm-generic/socket.h that don't seem logically obvious to group and eliminate some of the defines. Also for the other comment. The reason the conditionals were not consistent is because they were not consistent to begin with. I'm trying to follow your request to keep code churn to minimal. It's just that I moved to a different function as that seemed logical to me. Do you prefer me to remove that refactoring? -Deepa
> 3 reasons for not doing this: > > 1. We do not want to break userspace. If we move this to > linux/socket.h all the userspace programs now have to include > linux/socket.h or get this definition through a new libc. > 2. All the socket options are together in the file asm/socket.h. It > doesn't seem good for maintainability to move just a few bits > elsewhere. > 3. There are only 4 arches (after the series is applied) that have > their own asm/socket.h. And, this is because there seems to be > significant differences to asm-generic/socket.h that don't seem > logically obvious to group and eliminate some of the defines. Agreed. All good reasons to leave as is. > Also for the other comment. The reason the conditionals were not > consistent is because they were not consistent to begin with. The only difference I see is an inversion of the test. Nesting order is the same: int need_software_tstamp = sock_flag(sk, SOCK_RCVTSTAMP); ... if (need_software_tstamp) { if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) { } else { } } vs if (sock_flag(sk, SOCK_RCVTSTAMP)) { if (sock_flag(sk, SOCK_RCVTSTAMPNS)) { } else { } } I suggest just adding something like if (need_software_tstamp) { + if (sock_uses_new_tstamp(sk) { + __sock_recv_timestamp_new(msg, sk, ktime_to_timespec64(skb->tstamp)); + } else if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) { - if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) { } else { } and if (sock_flag(sk, SOCK_RCVTSTAMP)) { + if (sock_uses_new_tstamp(sk) { + __sock_recv_timestamp_new(msg, sk, ts); + else if (sock_flag(sk, SOCK_RCVTSTAMPNS)) { - if (sock_flag(sk, SOCK_RCVTSTAMPNS)) { } else { } I think we can use the same helper for both the sock and tcp variant. The only intended difference between the two functions, as described in the tcp_recv_timestamp function comment, is the absence of an skb in the tcp case. That is immaterial at this level. Note also (2) tentative helper function sock_uses_new_tstamp(const struct sock *sk) instead of testing sock_flag(sk, SOCK_TSTAMP_NEW) directly. Since the .._NEW variants are equivalent to .._OLD on 64-bit, I wonder if we can just compile out the branch. Something like static inline bool sock_uses_new_tstamp(const struct sock *sk) { return (sizeof(time_t) != sizeof(__kernel_long_t)) && sock_flag(sk, SOCK_TSTAMP_NEW); } > I'm trying to follow your request to keep code churn to minimal. > It's just that I moved to a different function as that seemed logical > to me. Do you prefer me to remove that refactoring? Yes, please avoid rearranging existing code as much as possible. If there is any refactoring to be done, I think it would be to deduplicate the shared logic between __sock_recv_timestamp and tcp_recv_timestamp. I think the first can be rewritten to reuse the second, if the only difference really is that the first takes an skb with embedded timestamps, while the second directly takes a pointer to struct scm_timestamping. Either way, that's out of scope for this patchset.
> > Also for the other comment. The reason the conditionals were not > > consistent is because they were not consistent to begin with. > > The only difference I see is an inversion of the test. Nesting order > is the same: > > int need_software_tstamp = sock_flag(sk, SOCK_RCVTSTAMP); > ... > if (need_software_tstamp) { > if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) { > } else { > } > } > > vs > > if (sock_flag(sk, SOCK_RCVTSTAMP)) { > if (sock_flag(sk, SOCK_RCVTSTAMPNS)) { > } else { > } > } > > I suggest just adding something like > > if (need_software_tstamp) { > + if (sock_uses_new_tstamp(sk) { > + __sock_recv_timestamp_new(msg, sk, > ktime_to_timespec64(skb->tstamp)); > + } else if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) { > - if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) { > } else { > } > > and > > if (sock_flag(sk, SOCK_RCVTSTAMP)) { > + if (sock_uses_new_tstamp(sk) { > + __sock_recv_timestamp_new(msg, sk, ts); > + else if (sock_flag(sk, SOCK_RCVTSTAMPNS)) { > - if (sock_flag(sk, SOCK_RCVTSTAMPNS)) { > } else { > } > > I think we can use the same helper for both the sock and tcp variant. > The only intended difference between the two functions, as described > in the tcp_recv_timestamp function comment, is the absence of an skb > in the tcp case. That is immaterial at this level. I will just not refactor things into a function: __sock_rescv_timestamp_new(). I will just add new conditionals for the new timestamps. When you guys refactor the other timestamp stuff like you mentioned below maybe you can move the new timestamps to a new funtcion as you see fit. The helper functions in skbuff.h might first need to be refactored first. But I again leave this to you guys. > Note also (2) tentative helper function sock_uses_new_tstamp(const > struct sock *sk) instead of testing sock_flag(sk, SOCK_TSTAMP_NEW) > directly. Since the .._NEW variants are equivalent to .._OLD on 64-bit, > I wonder if we can just compile out the branch. Something like > > static inline bool sock_uses_new_tstamp(const struct sock *sk) { > return (sizeof(time_t) != sizeof(__kernel_long_t)) && > sock_flag(sk, SOCK_TSTAMP_NEW); > } > You could just ifdef CONFIG_64BIT if you are worried about branching. Note that SO_TIMESTAMP is by default SO_TIMESTAMP_OLD on 64 bit machines. But, I will again leave the optimization to you. I will implement in a straight forward way and you guys can deicde how you want to optimize the fast path or what should it even be. -Deepa
On Sat, Dec 15, 2018 at 7:52 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > > 3 reasons for not doing this: > > > > 1. We do not want to break userspace. If we move this to > > linux/socket.h all the userspace programs now have to include > > linux/socket.h or get this definition through a new libc. > > 2. All the socket options are together in the file asm/socket.h. It > > doesn't seem good for maintainability to move just a few bits > > elsewhere. > > 3. There are only 4 arches (after the series is applied) that have > > their own asm/socket.h. And, this is because there seems to be > > significant differences to asm-generic/socket.h that don't seem > > logically obvious to group and eliminate some of the defines. > > Agreed. All good reasons to leave as is. > > > Also for the other comment. The reason the conditionals were not > > consistent is because they were not consistent to begin with. > > The only difference I see is an inversion of the test. Nesting order > is the same: > > int need_software_tstamp = sock_flag(sk, SOCK_RCVTSTAMP); > ... > if (need_software_tstamp) { > if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) { > } else { > } > } > > vs > > if (sock_flag(sk, SOCK_RCVTSTAMP)) { > if (sock_flag(sk, SOCK_RCVTSTAMPNS)) { > } else { > } > } > > I suggest just adding something like > > if (need_software_tstamp) { > + if (sock_uses_new_tstamp(sk) { > + __sock_recv_timestamp_new(msg, sk, > ktime_to_timespec64(skb->tstamp)); > + } else if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) { > - if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) { > } else { > } > > and > > if (sock_flag(sk, SOCK_RCVTSTAMP)) { > + if (sock_uses_new_tstamp(sk) { > + __sock_recv_timestamp_new(msg, sk, ts); > + else if (sock_flag(sk, SOCK_RCVTSTAMPNS)) { > - if (sock_flag(sk, SOCK_RCVTSTAMPNS)) { > } else { > } Generally speaking, I think we want the new time handling to be written as the default case rather than have it hidden away in a separate function. If we didn't have the sparc64 quirk with its unusual timeval definition, we'd only need a special flag for the old 32-bit format, but that doesn't work as long we have to support two different 64-bit formats for 64-bit timeval on sparc64 (32 or 64 bit microseconds). > Note also (2) tentative helper function sock_uses_new_tstamp(const > struct sock *sk) instead of testing sock_flag(sk, SOCK_TSTAMP_NEW) > directly. Since the .._NEW variants are equivalent to .._OLD on 64-bit, > I wonder if we can just compile out the branch. Something like > > static inline bool sock_uses_new_tstamp(const struct sock *sk) { > return (sizeof(time_t) != sizeof(__kernel_long_t)) && > sock_flag(sk, SOCK_TSTAMP_NEW); > } I think that would break compat handling: when we have a 32-bit user space process, the difference between old and new timestamps is meaningful even on 64-bit kernels, but the distinction is only made all the way down in put_cmsg_compat(). Arnd
On Tue, Dec 18, 2018 at 8:33 AM Arnd Bergmann <arnd@arndb.de> wrote: > > On Sat, Dec 15, 2018 at 7:52 PM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > > 3 reasons for not doing this: > > > > > > 1. We do not want to break userspace. If we move this to > > > linux/socket.h all the userspace programs now have to include > > > linux/socket.h or get this definition through a new libc. > > > 2. All the socket options are together in the file asm/socket.h. It > > > doesn't seem good for maintainability to move just a few bits > > > elsewhere. > > > 3. There are only 4 arches (after the series is applied) that have > > > their own asm/socket.h. And, this is because there seems to be > > > significant differences to asm-generic/socket.h that don't seem > > > logically obvious to group and eliminate some of the defines. > > > > Agreed. All good reasons to leave as is. > > > > > Also for the other comment. The reason the conditionals were not > > > consistent is because they were not consistent to begin with. > > > > The only difference I see is an inversion of the test. Nesting order > > is the same: > > > > int need_software_tstamp = sock_flag(sk, SOCK_RCVTSTAMP); > > ... > > if (need_software_tstamp) { > > if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) { > > } else { > > } > > } > > > > vs > > > > if (sock_flag(sk, SOCK_RCVTSTAMP)) { > > if (sock_flag(sk, SOCK_RCVTSTAMPNS)) { > > } else { > > } > > } > > > > I suggest just adding something like > > > > if (need_software_tstamp) { > > + if (sock_uses_new_tstamp(sk) { > > + __sock_recv_timestamp_new(msg, sk, > > ktime_to_timespec64(skb->tstamp)); > > + } else if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) { > > - if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) { > > } else { > > } > > > > and > > > > if (sock_flag(sk, SOCK_RCVTSTAMP)) { > > + if (sock_uses_new_tstamp(sk) { > > + __sock_recv_timestamp_new(msg, sk, ts); > > + else if (sock_flag(sk, SOCK_RCVTSTAMPNS)) { > > - if (sock_flag(sk, SOCK_RCVTSTAMPNS)) { > > } else { > > } > > Generally speaking, I think we want the new time handling > to be written as the default case rather than have it hidden away > in a separate function. If we didn't have the sparc64 quirk with its > unusual timeval definition, we'd only need a special flag for the > old 32-bit format, but that doesn't work as long we have to support > two different 64-bit formats for 64-bit timeval on sparc64 > (32 or 64 bit microseconds). > > > Note also (2) tentative helper function sock_uses_new_tstamp(const > > struct sock *sk) instead of testing sock_flag(sk, SOCK_TSTAMP_NEW) > > directly. Since the .._NEW variants are equivalent to .._OLD on 64-bit, > > I wonder if we can just compile out the branch. Something like > > > > static inline bool sock_uses_new_tstamp(const struct sock *sk) { > > return (sizeof(time_t) != sizeof(__kernel_long_t)) && > > sock_flag(sk, SOCK_TSTAMP_NEW); > > } > > I think that would break compat handling: when we have a 32-bit > user space process, the difference between old and new timestamps > is meaningful even on 64-bit kernels, but the distinction is only made all > the way down in put_cmsg_compat(). As I mentioned previously, I have refrained from adding these optimizations for now. The old timestamps are as is and the new timestamps are not yet being used anywhere as we have not switched any of the architectures to use y2038 syscalls and data structures yet. So even if these optimizations are needed these can be added as separate patches. Let me know if this is acceptable for everyone and I can post the update. -Deepa
diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h index 00e45c80e574..352e3dc0b3d9 100644 --- a/arch/alpha/include/uapi/asm/socket.h +++ b/arch/alpha/include/uapi/asm/socket.h @@ -3,6 +3,7 @@ #define _UAPI_ASM_SOCKET_H #include <asm/sockios.h> +#include <asm/bitsperlong.h> /* For setsockopt(2) */ /* @@ -110,12 +111,22 @@ #define SO_TIMESTAMP_OLD 29 #define SO_TIMESTAMPNS_OLD 35 + #define SO_TIMESTAMPING_OLD 37 +#define SO_TIMESTAMP_NEW 62 +#define SO_TIMESTAMPNS_NEW 63 + #if !defined(__KERNEL__) -#define SO_TIMESTAMP SO_TIMESTAMP_OLD -#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD +#if __BITS_PER_LONG == 64 +#define SO_TIMESTAMP SO_TIMESTAMP_OLD +#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD +#else +#define SO_TIMESTAMP (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMP_OLD : SO_TIMESTAMP_NEW) +#define SO_TIMESTAMPNS (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMPNS_OLD : SO_TIMESTAMPNS_NEW) +#endif + #define SO_TIMESTAMPING SO_TIMESTAMPING_OLD #define SCM_TIMESTAMP SO_TIMESTAMP diff --git a/arch/mips/include/uapi/asm/socket.h b/arch/mips/include/uapi/asm/socket.h index b9553f770346..d1752e3f1248 100644 --- a/arch/mips/include/uapi/asm/socket.h +++ b/arch/mips/include/uapi/asm/socket.h @@ -11,6 +11,7 @@ #define _UAPI_ASM_SOCKET_H #include <asm/sockios.h> +#include <asm/bitsperlong.h> /* * For setsockopt(2) @@ -123,10 +124,19 @@ #define SO_TIMESTAMPNS_OLD 35 #define SO_TIMESTAMPING_OLD 37 +#define SO_TIMESTAMP_NEW 62 +#define SO_TIMESTAMPNS_NEW 63 + #if !defined(__KERNEL__) -#define SO_TIMESTAMP SO_TIMESTAMP_OLD -#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD +#if __BITS_PER_LONG == 64 +#define SO_TIMESTAMP SO_TIMESTAMP_OLD +#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD +#else +#define SO_TIMESTAMP (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMP_OLD : SO_TIMESTAMP_NEW) +#define SO_TIMESTAMPNS (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMPNS_OLD : SO_TIMESTAMPNS_NEW) +#endif + #define SO_TIMESTAMPING SO_TIMESTAMPING_OLD #define SCM_TIMESTAMP SO_TIMESTAMP diff --git a/arch/parisc/include/uapi/asm/socket.h b/arch/parisc/include/uapi/asm/socket.h index 37cdfe64bb27..0a45b668abd1 100644 --- a/arch/parisc/include/uapi/asm/socket.h +++ b/arch/parisc/include/uapi/asm/socket.h @@ -3,6 +3,7 @@ #define _UAPI_ASM_SOCKET_H #include <asm/sockios.h> +#include <asm/bitsperlong.h> /* For setsockopt(2) */ #define SOL_SOCKET 0xffff @@ -104,10 +105,19 @@ #define SO_TIMESTAMPNS_OLD 0x4013 #define SO_TIMESTAMPING_OLD 0x4020 +#define SO_TIMESTAMP_NEW 0x4037 +#define SO_TIMESTAMPNS_NEW 0x4038 + #if !defined(__KERNEL__) -#define SO_TIMESTAMP SO_TIMESTAMP_OLD -#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD +#if __BITS_PER_LONG == 64 +#define SO_TIMESTAMP SO_TIMESTAMP_OLD +#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD +#else +#define SO_TIMESTAMP (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMP_OLD : SO_TIMESTAMP_NEW) +#define SO_TIMESTAMPNS (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMPNS_OLD : SO_TIMESTAMPNS_NEW) +#endif + #define SO_TIMESTAMPING SO_TIMESTAMPING_OLD #define SCM_TIMESTAMP SO_TIMESTAMP diff --git a/arch/sparc/include/uapi/asm/socket.h b/arch/sparc/include/uapi/asm/socket.h index ca573641fc6c..dc8527cae5a7 100644 --- a/arch/sparc/include/uapi/asm/socket.h +++ b/arch/sparc/include/uapi/asm/socket.h @@ -3,6 +3,7 @@ #define _ASM_SOCKET_H #include <asm/sockios.h> +#include <asm/bitsperlong.h> /* For setsockopt(2) */ #define SOL_SOCKET 0xffff @@ -105,10 +106,19 @@ #define SO_TIMESTAMPNS_OLD 0x0021 #define SO_TIMESTAMPING_OLD 0x0023 +#define SO_TIMESTAMP_NEW 0x0040 +#define SO_TIMESTAMPNS_NEW 0x0041 + #if !defined(__KERNEL__) -#define SO_TIMESTAMP SO_TIMESTAMP_OLD -#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD +#if __BITS_PER_LONG == 64 +#define SO_TIMESTAMP SO_TIMESTAMP_OLD +#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD +#else +#define SO_TIMESTAMP (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMP_OLD : SO_TIMESTAMP_NEW) +#define SO_TIMESTAMPNS (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMPNS_OLD : SO_TIMESTAMPNS_NEW) +#endif + #define SO_TIMESTAMPING SO_TIMESTAMPING_OLD #define SCM_TIMESTAMP SO_TIMESTAMP diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index e2dc01330cb1..1d583c3e7f89 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -3465,12 +3465,30 @@ static inline void skb_get_timestamp(const struct sk_buff *skb, *stamp = ns_to_kernel_old_timeval(skb->tstamp); } +static inline void skb_get_new_timestamp(const struct sk_buff *skb, + struct __kernel_sock_timeval *stamp) +{ + struct timespec64 ts = ktime_to_timespec64(skb->tstamp); + + stamp->tv_sec = ts.tv_sec; + stamp->tv_usec = ts.tv_nsec / 1000; +} + static inline void skb_get_timestampns(const struct sk_buff *skb, struct timespec *stamp) { *stamp = ktime_to_timespec(skb->tstamp); } +static inline void skb_get_new_timestampns(const struct sk_buff *skb, + struct __kernel_timespec *stamp) +{ + struct timespec64 ts = ktime_to_timespec64(skb->tstamp); + + stamp->tv_sec = ts.tv_sec; + stamp->tv_nsec = ts.tv_nsec; +} + static inline void __net_timestamp(struct sk_buff *skb) { skb->tstamp = ktime_get_real(); diff --git a/include/net/sock.h b/include/net/sock.h index f665d74ae509..3c666a12cf6c 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -801,6 +801,7 @@ enum sock_flags { SOCK_RCU_FREE, /* wait rcu grace period in sk_destruct() */ SOCK_TXTIME, SOCK_XDP, /* XDP is attached */ + SOCK_TSTAMP_NEW, /* Indicates 64 bit timestamps always */ }; #define SK_FLAGS_TIMESTAMP ((1UL << SOCK_TIMESTAMP) | (1UL << SOCK_TIMESTAMPING_RX_SOFTWARE)) diff --git a/include/uapi/asm-generic/socket.h b/include/uapi/asm-generic/socket.h index dc704e41203d..0b0fae6b57a9 100644 --- a/include/uapi/asm-generic/socket.h +++ b/include/uapi/asm-generic/socket.h @@ -3,6 +3,7 @@ #define __ASM_GENERIC_SOCKET_H #include <asm/sockios.h> +#include <asm/bitsperlong.h> /* For setsockopt(2) */ #define SOL_SOCKET 1 @@ -107,10 +108,20 @@ #define SO_TIMESTAMPNS_OLD 35 #define SO_TIMESTAMPING_OLD 37 +#define SO_TIMESTAMP_NEW 62 +#define SO_TIMESTAMPNS_NEW 63 + #if !defined(__KERNEL__) -#define SO_TIMESTAMP SO_TIMESTAMP_OLD -#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD +#if __BITS_PER_LONG == 64 || (defined(__x86_64__) && defined(__ILP32__)) +/* on 64-bit and x32, avoid the ?: operator */ +#define SO_TIMESTAMP SO_TIMESTAMP_OLD +#define SO_TIMESTAMPNS SO_TIMESTAMPNS_OLD +#else +#define SO_TIMESTAMP (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMP_OLD : SO_TIMESTAMP_NEW) +#define SO_TIMESTAMPNS (sizeof(time_t) == sizeof(__kernel_long_t) ? SO_TIMESTAMPNS_OLD : SO_TIMESTAMPNS_NEW) +#endif + #define SO_TIMESTAMPING SO_TIMESTAMPING_OLD #define SCM_TIMESTAMP SO_TIMESTAMP diff --git a/net/core/sock.c b/net/core/sock.c index cf990db9b2a0..1f4db404782c 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -647,6 +647,35 @@ bool sk_mc_loop(struct sock *sk) } EXPORT_SYMBOL(sk_mc_loop); +static void setsockopt_timestamp(struct sock *sk, int type, int val) +{ + if (!val) { + sock_reset_flag(sk, SOCK_RCVTSTAMP); + sock_reset_flag(sk, SOCK_RCVTSTAMPNS); + sock_reset_flag(sk, SOCK_TSTAMP_NEW); + return; + } + + if (type == SO_TIMESTAMP_NEW || type == SO_TIMESTAMPNS_NEW) + sock_set_flag(sk, SOCK_TSTAMP_NEW); + else + sock_reset_flag(sk, SOCK_TSTAMP_NEW); + + switch (type) { + case SO_TIMESTAMP_OLD: + case SO_TIMESTAMP_NEW: + sock_reset_flag(sk, SOCK_RCVTSTAMPNS); + break; + case SO_TIMESTAMPNS_OLD: + case SO_TIMESTAMPNS_NEW: + sock_set_flag(sk, SOCK_RCVTSTAMPNS); + break; + } + + sock_set_flag(sk, SOCK_RCVTSTAMP); + sock_enable_timestamp(sk, SOCK_TIMESTAMP); +} + /* * This is meant for all protocols to use and covers goings on * at the socket level. Everything here is generic. @@ -815,18 +844,10 @@ int sock_setsockopt(struct socket *sock, int level, int optname, break; case SO_TIMESTAMP_OLD: + case SO_TIMESTAMP_NEW: case SO_TIMESTAMPNS_OLD: - if (valbool) { - if (optname == SO_TIMESTAMP_OLD) - sock_reset_flag(sk, SOCK_RCVTSTAMPNS); - else - sock_set_flag(sk, SOCK_RCVTSTAMPNS); - sock_set_flag(sk, SOCK_RCVTSTAMP); - sock_enable_timestamp(sk, SOCK_TIMESTAMP); - } else { - sock_reset_flag(sk, SOCK_RCVTSTAMP); - sock_reset_flag(sk, SOCK_RCVTSTAMPNS); - } + case SO_TIMESTAMPNS_NEW: + setsockopt_timestamp(sk, optname, valbool); break; case SO_TIMESTAMPING_OLD: @@ -1191,6 +1212,14 @@ int sock_getsockopt(struct socket *sock, int level, int optname, v.val = sock_flag(sk, SOCK_RCVTSTAMPNS); break; + case SO_TIMESTAMP_NEW: + v.val = sock_flag(sk, SOCK_RCVTSTAMP) && sock_flag(sk, SOCK_TSTAMP_NEW); + break; + + case SO_TIMESTAMPNS_NEW: + v.val = sock_flag(sk, SOCK_RCVTSTAMPNS) && sock_flag(sk, SOCK_TSTAMP_NEW); + break; + case SO_TIMESTAMPING_OLD: v.val = sk->sk_tsflags; break; diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 5a86ce0bdf32..73e1628a3946 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -1860,32 +1860,57 @@ static void tcp_update_recv_tstamps(struct sk_buff *skb, tss->ts[2] = (struct timespec) {0}; } -/* Similar to __sock_recv_timestamp, but does not require an skb */ -static void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk, - struct scm_timestamping *tss) +static void tcp_recv_sw_timestamp(struct msghdr *msg, const struct sock *sk, struct timespec64 *ts) { - struct __kernel_old_timeval tv; - bool has_timestamping = false; + int new_tstamp = sock_flag(sk, SOCK_TSTAMP_NEW); + + if (ts->tv_sec || ts->tv_nsec) { + if (sock_flag(sk, SOCK_RCVTSTAMPNS)) { + if (new_tstamp) { + struct __kernel_timespec kts = {ts->tv_sec, ts->tv_nsec}; + + put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMPNS_NEW, + sizeof(kts), &kts); + } else { + struct timespec ts_old = timespec64_to_timespec(*ts); - if (tss->ts[0].tv_sec || tss->ts[0].tv_nsec) { - if (sock_flag(sk, SOCK_RCVTSTAMP)) { - if (sock_flag(sk, SOCK_RCVTSTAMPNS)) { put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMPNS_OLD, - sizeof(tss->ts[0]), &tss->ts[0]); + sizeof(ts), &ts_old); + } + } else if (sock_flag(sk, SOCK_RCVTSTAMP)) { + if (new_tstamp) { + struct __kernel_sock_timeval stv; + + stv.tv_sec = ts->tv_sec; + stv.tv_usec = ts->tv_nsec / 1000; + + put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMP_NEW, + sizeof(stv), &stv); } else { - tv.tv_sec = tss->ts[0].tv_sec; - tv.tv_usec = tss->ts[0].tv_nsec / 1000; + struct __kernel_old_timeval tv; + tv.tv_sec = ts->tv_sec; + tv.tv_usec = ts->tv_nsec / 1000; put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMP_OLD, sizeof(tv), &tv); } } - - if (sk->sk_tsflags & SOF_TIMESTAMPING_SOFTWARE) - has_timestamping = true; - else - tss->ts[0] = (struct timespec) {0}; } +} + +/* Similar to __sock_recv_timestamp, but does not require an skb */ +static void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk, + struct scm_timestamping *tss) +{ + bool has_timestamping = false; + struct timespec64 ts = timespec_to_timespec64(tss->ts[0]); + + tcp_recv_sw_timestamp(msg, sk, &ts); + + if (sk->sk_tsflags & SOF_TIMESTAMPING_SOFTWARE) + has_timestamping = true; + else + tss->ts[0] = (struct timespec) {0}; if (tss->ts[2].tv_sec || tss->ts[2].tv_nsec) { if (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c index eeb4639adbe5..65571a6273c3 100644 --- a/net/rds/af_rds.c +++ b/net/rds/af_rds.c @@ -348,7 +348,7 @@ static int rds_set_transport(struct rds_sock *rs, char __user *optval, } static int rds_enable_recvtstamp(struct sock *sk, char __user *optval, - int optlen) + int optlen, int optname) { int val, valbool; @@ -360,6 +360,9 @@ static int rds_enable_recvtstamp(struct sock *sk, char __user *optval, valbool = val ? 1 : 0; + if (optname == SO_TIMESTAMP_NEW) + sock_set_flag(sk, SOCK_TSTAMP_NEW); + if (valbool) sock_set_flag(sk, SOCK_RCVTSTAMP); else @@ -431,8 +434,9 @@ static int rds_setsockopt(struct socket *sock, int level, int optname, release_sock(sock->sk); break; case SO_TIMESTAMP_OLD: + case SO_TIMESTAMP_NEW: lock_sock(sock->sk); - ret = rds_enable_recvtstamp(sock->sk, optval, optlen); + ret = rds_enable_recvtstamp(sock->sk, optval, optlen, optname); release_sock(sock->sk); break; case SO_RDS_MSG_RXPATH_LATENCY: diff --git a/net/rds/recv.c b/net/rds/recv.c index 435bf2320cd3..6bb6b16ca270 100644 --- a/net/rds/recv.c +++ b/net/rds/recv.c @@ -550,8 +550,20 @@ static int rds_cmsg_recv(struct rds_incoming *inc, struct msghdr *msg, if ((inc->i_rx_tstamp != 0) && sock_flag(rds_rs_to_sk(rs), SOCK_RCVTSTAMP)) { struct __kernel_old_timeval tv = ns_to_kernel_old_timeval(inc->i_rx_tstamp); - ret = put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMP_OLD, - sizeof(tv), &tv); + + if (!sock_flag(rds_rs_to_sk(rs), SOCK_TSTAMP_NEW)) { + ret = put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMP_OLD, + sizeof(tv), &tv); + } else { + struct __kernel_sock_timeval sk_tv; + + sk_tv.tv_sec = tv.tv_sec; + sk_tv.tv_usec = tv.tv_usec; + + ret = put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMP_NEW, + sizeof(sk_tv), &sk_tv); + } + if (ret) goto out; } diff --git a/net/socket.c b/net/socket.c index c92f0e97ae58..96fe831b79ad 100644 --- a/net/socket.c +++ b/net/socket.c @@ -699,6 +699,38 @@ static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb) sizeof(ts_pktinfo), &ts_pktinfo); } +static void sock_recv_sw_timestamp(struct msghdr *msg, struct sock *sk, + struct sk_buff *skb) +{ + if (sock_flag(sk, SOCK_TSTAMP_NEW)) { + if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) { + struct __kernel_sock_timeval tv; + + skb_get_new_timestamp(skb, &tv); + put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMP_NEW, + sizeof(tv), &tv); + } else { + struct __kernel_timespec ts; + + skb_get_new_timestampns(skb, &ts); + put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMPNS_NEW, + sizeof(ts), &ts); + } + } + if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) { + struct __kernel_old_timeval tv; + + skb_get_timestamp(skb, &tv); + put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMP_OLD, + sizeof(tv), &tv); + } else { + struct timespec ts; + + skb_get_timestampns(skb, &ts); + put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMPNS_OLD, + sizeof(ts), &ts); + } +} /* * called from sock_recv_timestamp() if sock_flag(sk, SOCK_RCVTSTAMP) */ @@ -718,19 +750,8 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, false_tstamp = 1; } - if (need_software_tstamp) { - if (!sock_flag(sk, SOCK_RCVTSTAMPNS)) { - struct __kernel_old_timeval tv; - skb_get_timestamp(skb, &tv); - put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMP_OLD, - sizeof(tv), &tv); - } else { - struct timespec ts; - skb_get_timestampns(skb, &ts); - put_cmsg(msg, SOL_SOCKET, SO_TIMESTAMPNS_OLD, - sizeof(ts), &ts); - } - } + if (need_software_tstamp) + sock_recv_sw_timestamp(msg, sk, skb); memset(&tss, 0, sizeof(tss)); if ((sk->sk_tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
Add SO_TIMESTAMP_NEW and SO_TIMESTAMPNS_NEW variants of socket timestamp options. These are the y2038 safe versions of the SO_TIMESTAMP_OLD and SO_TIMESTAMPNS_OLD for all architectures. Note that the format of scm_timestamping.ts[0] is not changed in this patch. Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com> Cc: jejb@parisc-linux.org Cc: ralf@linux-mips.org Cc: rth@twiddle.net Cc: linux-alpha@vger.kernel.org Cc: linux-mips@linux-mips.org Cc: linux-parisc@vger.kernel.org Cc: linux-rdma@vger.kernel.org Cc: netdev@vger.kernel.org Cc: sparclinux@vger.kernel.org --- arch/alpha/include/uapi/asm/socket.h | 15 ++++++- arch/mips/include/uapi/asm/socket.h | 14 ++++++- arch/parisc/include/uapi/asm/socket.h | 14 ++++++- arch/sparc/include/uapi/asm/socket.h | 14 ++++++- include/linux/skbuff.h | 18 +++++++++ include/net/sock.h | 1 + include/uapi/asm-generic/socket.h | 15 ++++++- net/core/sock.c | 51 ++++++++++++++++++------ net/ipv4/tcp.c | 57 +++++++++++++++++++-------- net/rds/af_rds.c | 8 +++- net/rds/recv.c | 16 +++++++- net/socket.c | 47 ++++++++++++++++------ 12 files changed, 216 insertions(+), 54 deletions(-)