mbox series

[0/2] bpf, net: Allow setting SO_TIMESTAMPING* from BPF

Message ID 20230703175048.151683-1-jthinz@mailbox.tu-berlin.de
Headers show
Series bpf, net: Allow setting SO_TIMESTAMPING* from BPF | expand

Message

Jörn-Thorben Hinz July 3, 2023, 5:50 p.m. UTC
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(-)

Comments

John Fastabend July 3, 2023, 9:25 p.m. UTC | #1
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
> 
>
Jörn-Thorben Hinz July 4, 2023, 4:11 p.m. UTC | #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
> >
Willem de Bruijn July 4, 2023, 7:36 p.m. UTC | #3
> 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.
Arnd Bergmann July 4, 2023, 8:26 p.m. UTC | #4
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