BPF applications, e.g., a TCP congestion control, might benefit from precise packet timestamps. These timestamps are already available in __sk_buff and bpf_sock_ops, but could not be requested: A BPF program was not allowed to set SO_TIMESTAMPING* on a socket. This change enables BPF programs to actively request the generation of timestamps from a stream socket.
To reuse the setget_sockopt BPF prog test for bpf_{get,set}sockopt(SO_TIMESTAMPING_NEW), also implement the missing getsockopt(SO_TIMESTAMPING_NEW) in the network stack.
I reckon the way I added getsockopt(SO_TIMESTAMPING_NEW) causes an API change: For existing users that set SO_TIMESTAMPING_NEW but queried SO_TIMESTAMPING_OLD afterwards, it would now look as if no timestamping flags have been set. Is this an acceptable change? If not, I’m happy to change getsockopt() to only be strict about the newly-implemented getsockopt(SO_TIMESTAMPING_NEW), or not distinguish between SO_TIMESTAMPING_NEW and SO_TIMESTAMPING_OLD at all.
Jörn-Thorben Hinz (2): net: Implement missing getsockopt(SO_TIMESTAMPING_NEW) bpf: Allow setting SO_TIMESTAMPING* with bpf_setsockopt()
include/uapi/linux/bpf.h | 3 ++- net/core/filter.c | 2 ++ net/core/sock.c | 9 +++++++-- tools/include/uapi/linux/bpf.h | 3 ++- tools/testing/selftests/bpf/progs/bpf_tracing_net.h | 2 ++ tools/testing/selftests/bpf/progs/setget_sockopt.c | 4 ++++ 6 files changed, 19 insertions(+), 4 deletions(-)
Commit 9718475e6908 ("socket: Add SO_TIMESTAMPING_NEW") added the new socket option SO_TIMESTAMPING_NEW. Setting the option is handled in sk_setsockopt(), querying it was not handled in sk_getsockopt(), though.
For consistency, implement the missing getsockopt(SO_TIMESTAMPING_NEW). Similar as with SO_TIMESTAMP_{OLD,NEW}, the active timestamping flags are only returned when querying the same SO_TIMESTAMPING_{OLD,NEW} option they were set for. Empty flags are returned with SO_TIMESTAMPING_{NEW,OLD} otherwise.
Fixes: 9718475e6908 ("socket: Add SO_TIMESTAMPING_NEW") Signed-off-by: Jörn-Thorben Hinz jthinz@mailbox.tu-berlin.de --- net/core/sock.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/net/core/sock.c b/net/core/sock.c index 9370fd50aa2c..cfb48244ed12 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1710,9 +1710,14 @@ int sk_getsockopt(struct sock *sk, int level, int optname, break;
case SO_TIMESTAMPING_OLD: + case SO_TIMESTAMPING_NEW: lv = sizeof(v.timestamping); - v.timestamping.flags = sk->sk_tsflags; - v.timestamping.bind_phc = sk->sk_bind_phc; + if (optname == (sock_flag(sk, SOCK_TSTAMP_NEW) ? + SO_TIMESTAMPING_NEW : + SO_TIMESTAMPING_OLD)) { + v.timestamping.flags = sk->sk_tsflags; + v.timestamping.bind_phc = sk->sk_bind_phc; + } break;
case SO_RCVTIMEO_OLD:
A BPF application, e.g., a TCP congestion control, might benefit from or even require precise (=hardware) packet timestamps. These timestamps are already available in __sk_buff.hwtstamp and bpf_sock_ops.skb_hwtstamp, but could not be requested: A BPF program was not allowed to set SO_TIMESTAMPING* on a socket.
Enable BPF programs to actively request the generation of timestamps from a stream socket. The also required ioctl(SIOCSHWTSTAMP) on the network device must still be done separately, in user space.
Signed-off-by: Jörn-Thorben Hinz jthinz@mailbox.tu-berlin.de --- include/uapi/linux/bpf.h | 3 ++- net/core/filter.c | 2 ++ tools/include/uapi/linux/bpf.h | 3 ++- tools/testing/selftests/bpf/progs/bpf_tracing_net.h | 2 ++ tools/testing/selftests/bpf/progs/setget_sockopt.c | 4 ++++ 5 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 60a9d59beeab..3e64b8137931 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -2640,7 +2640,8 @@ union bpf_attr { * **SO_RCVBUF**, **SO_SNDBUF**, **SO_MAX_PACING_RATE**, * **SO_PRIORITY**, **SO_RCVLOWAT**, **SO_MARK**, * **SO_BINDTODEVICE**, **SO_KEEPALIVE**, **SO_REUSEADDR**, - * **SO_REUSEPORT**, **SO_BINDTOIFINDEX**, **SO_TXREHASH**. + * **SO_REUSEPORT**, **SO_BINDTOIFINDEX**, **SO_TXREHASH**, + * **SO_TIMESTAMPING_NEW**, **SO_TIMESTAMPING_OLD**. * * **IPPROTO_TCP**, which supports the following *optname*\ s: * **TCP_CONGESTION**, **TCP_BPF_IW**, * **TCP_BPF_SNDCWND_CLAMP**, **TCP_SAVE_SYN**, diff --git a/net/core/filter.c b/net/core/filter.c index 06ba0e56e369..af0f3a6762de 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -5108,6 +5108,8 @@ static int sol_socket_sockopt(struct sock *sk, int optname, case SO_MAX_PACING_RATE: case SO_BINDTOIFINDEX: case SO_TXREHASH: + case SO_TIMESTAMPING_NEW: + case SO_TIMESTAMPING_OLD: if (*optlen != sizeof(int)) return -EINVAL; break; diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 60a9d59beeab..3e64b8137931 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -2640,7 +2640,8 @@ union bpf_attr { * **SO_RCVBUF**, **SO_SNDBUF**, **SO_MAX_PACING_RATE**, * **SO_PRIORITY**, **SO_RCVLOWAT**, **SO_MARK**, * **SO_BINDTODEVICE**, **SO_KEEPALIVE**, **SO_REUSEADDR**, - * **SO_REUSEPORT**, **SO_BINDTOIFINDEX**, **SO_TXREHASH**. + * **SO_REUSEPORT**, **SO_BINDTOIFINDEX**, **SO_TXREHASH**, + * **SO_TIMESTAMPING_NEW**, **SO_TIMESTAMPING_OLD**. * * **IPPROTO_TCP**, which supports the following *optname*\ s: * **TCP_CONGESTION**, **TCP_BPF_IW**, * **TCP_BPF_SNDCWND_CLAMP**, **TCP_SAVE_SYN**, diff --git a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h index cfed4df490f3..8d526db8ceeb 100644 --- a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h +++ b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h @@ -15,8 +15,10 @@ #define SO_RCVLOWAT 18 #define SO_BINDTODEVICE 25 #define SO_MARK 36 +#define SO_TIMESTAMPING_OLD 37 #define SO_MAX_PACING_RATE 47 #define SO_BINDTOIFINDEX 62 +#define SO_TIMESTAMPING_NEW 65 #define SO_TXREHASH 74 #define __SO_ACCEPTCON (1 << 16)
diff --git a/tools/testing/selftests/bpf/progs/setget_sockopt.c b/tools/testing/selftests/bpf/progs/setget_sockopt.c index 7a438600ae98..54205d10793c 100644 --- a/tools/testing/selftests/bpf/progs/setget_sockopt.c +++ b/tools/testing/selftests/bpf/progs/setget_sockopt.c @@ -48,6 +48,10 @@ static const struct sockopt_test sol_socket_tests[] = { { .opt = SO_MARK, .new = 0xeb9f, .expected = 0xeb9f, }, { .opt = SO_MAX_PACING_RATE, .new = 0xeb9f, .expected = 0xeb9f, }, { .opt = SO_TXREHASH, .flip = 1, }, + { .opt = SO_TIMESTAMPING_NEW, .new = SOF_TIMESTAMPING_RX_HARDWARE, + .expected = SOF_TIMESTAMPING_RX_HARDWARE, }, + { .opt = SO_TIMESTAMPING_OLD, .new = SOF_TIMESTAMPING_RX_HARDWARE, + .expected = SOF_TIMESTAMPING_RX_HARDWARE, }, { .opt = 0, }, };
Jörn-Thorben Hinz wrote:
BPF applications, e.g., a TCP congestion control, might benefit from precise packet timestamps. These timestamps are already available in __sk_buff and bpf_sock_ops, but could not be requested: A BPF program was not allowed to set SO_TIMESTAMPING* on a socket. This change enables BPF programs to actively request the generation of timestamps from a stream socket.
To reuse the setget_sockopt BPF prog test for bpf_{get,set}sockopt(SO_TIMESTAMPING_NEW), also implement the missing getsockopt(SO_TIMESTAMPING_NEW) in the network stack.
I reckon the way I added getsockopt(SO_TIMESTAMPING_NEW) causes an API change: For existing users that set SO_TIMESTAMPING_NEW but queried SO_TIMESTAMPING_OLD afterwards, it would now look as if no timestamping flags have been set. Is this an acceptable change? If not, I’m happy to change getsockopt() to only be strict about the newly-implemented getsockopt(SO_TIMESTAMPING_NEW), or not distinguish between SO_TIMESTAMPING_NEW and SO_TIMESTAMPING_OLD at all.
Yeah, I think it would be best if we keep the old behavior and let SO_TIMESTAMPING_OLD return timestamps for both new/old. It looks like it should be relatively easy to implement?
Otherwise the series lgtm.
Jörn-Thorben Hinz (2): net: Implement missing getsockopt(SO_TIMESTAMPING_NEW) bpf: Allow setting SO_TIMESTAMPING* with bpf_setsockopt()
include/uapi/linux/bpf.h | 3 ++- net/core/filter.c | 2 ++ net/core/sock.c | 9 +++++++-- tools/include/uapi/linux/bpf.h | 3 ++- tools/testing/selftests/bpf/progs/bpf_tracing_net.h | 2 ++ tools/testing/selftests/bpf/progs/setget_sockopt.c | 4 ++++ 6 files changed, 19 insertions(+), 4 deletions(-)
-- 2.39.2
Thank you for the feedback.
Just noticed I missed the “bpf-next” designation in the subject. Will add that in v2.
On Mon, 2023-07-03 at 14:25 -0700, John Fastabend wrote:
Jörn-Thorben Hinz wrote:
BPF applications, e.g., a TCP congestion control, might benefit from precise packet timestamps. These timestamps are already available in __sk_buff and bpf_sock_ops, but could not be requested: A BPF program was not allowed to set SO_TIMESTAMPING* on a socket. This change enables BPF programs to actively request the generation of timestamps from a stream socket.
To reuse the setget_sockopt BPF prog test for bpf_{get,set}sockopt(SO_TIMESTAMPING_NEW), also implement the missing getsockopt(SO_TIMESTAMPING_NEW) in the network stack.
I reckon the way I added getsockopt(SO_TIMESTAMPING_NEW) causes an API change: For existing users that set SO_TIMESTAMPING_NEW but queried SO_TIMESTAMPING_OLD afterwards, it would now look as if no timestamping flags have been set. Is this an acceptable change? If not, I’m happy to change getsockopt() to only be strict about the newly-implemented getsockopt(SO_TIMESTAMPING_NEW), or not distinguish between SO_TIMESTAMPING_NEW and SO_TIMESTAMPING_OLD at all.
Yeah, I think it would be best if we keep the old behavior and let SO_TIMESTAMPING_OLD return timestamps for both new/old. It looks like it should be relatively easy to implement?
Alright, I guessed that would be preferred.
Yes, if there is no objection to making the added getsockopt(SO_TIMESTAMPING_NEW) this tiny bit more “strict”, it’s just a matter of modifying the if inserted in sk_getsockopt(). (And, well, in the other case I would even remove this if.)
Otherwise the series lgtm.
Great, thanks.
Jörn-Thorben Hinz (2): net: Implement missing getsockopt(SO_TIMESTAMPING_NEW) bpf: Allow setting SO_TIMESTAMPING* with bpf_setsockopt()
include/uapi/linux/bpf.h | 3 ++- net/core/filter.c | 2 ++ net/core/sock.c | 9 +++++++-- tools/include/uapi/linux/bpf.h | 3 ++- tools/testing/selftests/bpf/progs/bpf_tracing_net.h | 2 ++ tools/testing/selftests/bpf/progs/setget_sockopt.c | 4 ++++ 6 files changed, 19 insertions(+), 4 deletions(-)
-- 2.39.2
On Mon, 2023-07-03 at 14:25 -0700, John Fastabend wrote:
Jörn-Thorben Hinz wrote:
BPF applications, e.g., a TCP congestion control, might benefit from precise packet timestamps. These timestamps are already available in __sk_buff and bpf_sock_ops, but could not be requested: A BPF program was not allowed to set SO_TIMESTAMPING* on a socket. This change enables BPF programs to actively request the generation of timestamps from a stream socket.
To reuse the setget_sockopt BPF prog test for bpf_{get,set}sockopt(SO_TIMESTAMPING_NEW), also implement the missing getsockopt(SO_TIMESTAMPING_NEW) in the network stack.
I reckon the way I added getsockopt(SO_TIMESTAMPING_NEW) causes an API change: For existing users that set SO_TIMESTAMPING_NEW but queried SO_TIMESTAMPING_OLD afterwards, it would now look as if no timestamping flags have been set. Is this an acceptable change? If not, I’m happy to change getsockopt() to only be strict about the newly-implemented getsockopt(SO_TIMESTAMPING_NEW), or not distinguish between SO_TIMESTAMPING_NEW and SO_TIMESTAMPING_OLD at all.
Yeah, I think it would be best if we keep the old behavior and let SO_TIMESTAMPING_OLD return timestamps for both new/old. It looks like it should be relatively easy to implement?
Alright, I guessed that would be preferred.
Yes, if there is no objection to making the added getsockopt(SO_TIMESTAMPING_NEW) this tiny bit more “strict”, it’s just a matter of modifying the if inserted in sk_getsockopt(). (And, well, in the other case I would even remove this if.)
The difference is in the struct that is returned, on 32-bit platforms. Both calls should always be allowed? See also put_cmsg_scm_timestamping64 vs put_cmsg_scm_timestamping.
For the second patch: the _OLD/_NEW was introduced to work around limitations on 32-bit platforms. This is intended to be transparent to users, by defining SO_TIMESTAMPING accordingly.
Can the new BPF code always enforce the 64-bit version, that is, only implement the _NEW variants? And perhaps just call it SO_TIMESTAMPING directly.
On Tue, Jul 4, 2023, at 21:36, Willem de Bruijn wrote:
On Mon, 2023-07-03 at 14:25 -0700, John Fastabend wrote:
Jörn-Thorben Hinz wrote:
Yes, if there is no objection to making the added getsockopt(SO_TIMESTAMPING_NEW) this tiny bit more “strict”, it’s just a matter of modifying the if inserted in sk_getsockopt(). (And, well, in the other case I would even remove this if.)
The difference is in the struct that is returned, on 32-bit platforms. Both calls should always be allowed? See also put_cmsg_scm_timestamping64 vs put_cmsg_scm_timestamping.
For the second patch: the _OLD/_NEW was introduced to work around limitations on 32-bit platforms. This is intended to be transparent to users, by defining SO_TIMESTAMPING accordingly.
Can the new BPF code always enforce the 64-bit version, that is, only implement the _NEW variants? And perhaps just call it SO_TIMESTAMPING directly.
I guess that depends on how the returned timestamps are interpreted.
In normal userspace code, the 'struct scm_timestamping' is defined in terms of the libc-provided 'struct timespec'. If this is a normal glibc based distro binary, then it probably expects the old format.
OTOH, if the code reading the timestamp data is in BPF code itself, it's probably safe to mandate that to use the time64 format and define the timespec type as __kernel_timespec with 64-bit members.
Arnd
linux-kselftest-mirror@lists.linaro.org
-
Arnd Bergmann -
John Fastabend -
Jörn-Thorben Hinz -
Willem de Bruijn