net/core/sock.c lacks some SO_TIMESTAMPING_NEW support

net/core/sock.c lacks some SO_TIMESTAMPING_NEW support

[Thomas Lange]

It seems that net/core/sock.c is missing support for SO_TIMESTAMPING_NEW in
some paths.

I cross compile for a 32bit ARM system using Yocto 4.3.1, which seems to have
64bit time by default. This maps SO_TIMESTAMPING to SO_TIMESTAMPING_NEW which
is expected AFAIK.

However, this breaks my application (Chrony) that sends SO_TIMESTAMPING as
a cmsg:

sendmsg(4, {msg_name={sa_family=AF_INET, sin_port=htons(123), sin_addr=inet_addr("172.16.11.22")}, msg_namelen=16, msg_iov=[{iov_base="#\0\6 \0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., iov_len=48}], msg_iovlen=1, msg_control=[{cmsg_len=16, cmsg_level=SOL_SOCKET, cmsg_type=SO_TIMESTAMPING_NEW, cmsg_data=???}], msg_controllen=16, msg_flags=0}, 0) = -1 EINVAL (Invalid argument)

This is because __sock_cmsg_send() does not support SO_TIMESTAMPING_NEW as-is.

This patch seems to fix things and the packet is transmitted:

diff --git a/net/core/sock.c b/net/core/sock.c
index 16584e2dd648..a56ec1d492c9 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2821,6 +2821,7 @@ int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
                 sockc->mark = *(u32 *)CMSG_DATA(cmsg);
                 break;
         case SO_TIMESTAMPING_OLD:
+       case SO_TIMESTAMPING_NEW:
                 if (cmsg->cmsg_len != CMSG_LEN(sizeof(u32)))
                         return -EINVAL;

However, looking through the module, it seems that sk_getsockopt() has no
support for SO_TIMESTAMPING_NEW either, but sk_setsockopt() has.
Testing seems to confirm this:

setsockopt(4, SOL_SOCKET, SO_TIMESTAMPING_NEW, [1048], 4) = 0
getsockopt(4, SOL_SOCKET, SO_TIMESTAMPING_NEW, 0x7ed5db20, [4]) = -1 ENOPROTOOPT (Protocol not available)

Patching sk_getsockopt() is not as obvious to me though.

I used a custom 6.6 kernel for my tests.
The relevant code seems unchanged in net-next.git though.

/Thomas

Re: net/core/sock.c lacks some SO_TIMESTAMPING_NEW support

[Willem de Bruijn]

Thomas Lange wrote:
> It seems that net/core/sock.c is missing support for SO_TIMESTAMPING_NEW in
> some paths.
> 
> I cross compile for a 32bit ARM system using Yocto 4.3.1, which seems to have
> 64bit time by default. This maps SO_TIMESTAMPING to SO_TIMESTAMPING_NEW which
> is expected AFAIK.
> 
> However, this breaks my application (Chrony) that sends SO_TIMESTAMPING as
> a cmsg:
> 
> sendmsg(4, {msg_name={sa_family=AF_INET, sin_port=htons(123), sin_addr=inet_addr("172.16.11.22")}, msg_namelen=16, msg_iov=[{iov_base="#\0\6 \0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., iov_len=48}], msg_iovlen=1, msg_control=[{cmsg_len=16, cmsg_level=SOL_SOCKET, cmsg_type=SO_TIMESTAMPING_NEW, cmsg_data=???}], msg_controllen=16, msg_flags=0}, 0) = -1 EINVAL (Invalid argument)
> 
> This is because __sock_cmsg_send() does not support SO_TIMESTAMPING_NEW as-is.
> 
> This patch seems to fix things and the packet is transmitted:
> 
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 16584e2dd648..a56ec1d492c9 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2821,6 +2821,7 @@ int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
>                  sockc->mark = *(u32 *)CMSG_DATA(cmsg);
>                  break;
>          case SO_TIMESTAMPING_OLD:
> +       case SO_TIMESTAMPING_NEW:
>                  if (cmsg->cmsg_len != CMSG_LEN(sizeof(u32)))
>                          return -EINVAL;
> 
> However, looking through the module, it seems that sk_getsockopt() has no
> support for SO_TIMESTAMPING_NEW either, but sk_setsockopt() has.

Good point. Adding the author to see if this was a simple oversight or
there was a rationale at the time for leaving it out.

> Testing seems to confirm this:
> 
> setsockopt(4, SOL_SOCKET, SO_TIMESTAMPING_NEW, [1048], 4) = 0
> getsockopt(4, SOL_SOCKET, SO_TIMESTAMPING_NEW, 0x7ed5db20, [4]) = -1 ENOPROTOOPT (Protocol not available)
> 
> Patching sk_getsockopt() is not as obvious to me though.
> 
> I used a custom 6.6 kernel for my tests.
> The relevant code seems unchanged in net-next.git though.
> 
> /Thomas

Re: net/core/sock.c lacks some SO_TIMESTAMPING_NEW support

[Arnd Bergmann]

On Wed, Dec 20, 2023, at 04:00, Willem de Bruijn wrote:
> Thomas Lange wrote:
>> diff --git a/net/core/sock.c b/net/core/sock.c
>> index 16584e2dd648..a56ec1d492c9 100644
>> --- a/net/core/sock.c
>> +++ b/net/core/sock.c
>> @@ -2821,6 +2821,7 @@ int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
>>                  sockc->mark = *(u32 *)CMSG_DATA(cmsg);
>>                  break;
>>          case SO_TIMESTAMPING_OLD:
>> +       case SO_TIMESTAMPING_NEW:
>>                  if (cmsg->cmsg_len != CMSG_LEN(sizeof(u32)))
>>                          return -EINVAL;
>> 
>> However, looking through the module, it seems that sk_getsockopt() has no
>> support for SO_TIMESTAMPING_NEW either, but sk_setsockopt() has.
>
> Good point. Adding the author to see if this was a simple oversight or
> there was a rationale at the time for leaving it out.

I'm fairly sure this was just a mistake on our side. For the cmsg case,
I think we just missed it because there is no corresponding SO_TIMESTAMP{,NS}
version of this, so it fell through the cracks.

In the patch above, I'm not entirely sure about what needs to happen
with the old/new format, i.e. the

   sock_valbool_flag(sk, SOCK_TSTAMP_NEW, optname == SO_TIMESTAMPING_NEW)

from setsockopt(). Is __sock_cmsg_send() allowed to turn on timestamping
without it being first enabled using setsockopt()? If so, I think
we need to set the flag here the same way that setsockopt does. If
not, then I think we instead should check that the old/new format
in the option sent via cmsg is the same that was set earlier with
setsockopt.

For the missing getsockopt, there was even a patch earlier this year
by Jörn-Thorben Hinz [1], but I failed to realize that we need patch
1/2 from his series regardless of patch 2/2.

     Arnd

[1] https://lore.kernel.org/lkml/20230703175048.151683-2-jthinz@mailbox.tu-berlin.de/

Re: net/core/sock.c lacks some SO_TIMESTAMPING_NEW support

[Jörn-Thorben Hinz]

Hi Arnd,

thanks for indirectly pinging me here about the unfinished patches. I
kinda forgot about them over other things happening.

Happy to look back into them, it looks like it would be helpful to
apply them. Is it fine to just answer the remarks from earlier this
year, after a few months, in the same mail thread? Or preferable to
resubmit the series[1] first?

Thorben

[1]
https://lore.kernel.org/lkml/20230703175048.151683-1-jthinz@mailbox.tu-berlin.de/

On Wed, 2023-12-20 at 09:43 +0000, Arnd Bergmann wrote:
> On Wed, Dec 20, 2023, at 04:00, Willem de Bruijn wrote:
> > Thomas Lange wrote:
> > > diff --git a/net/core/sock.c b/net/core/sock.c
> > > index 16584e2dd648..a56ec1d492c9 100644
> > > --- a/net/core/sock.c
> > > +++ b/net/core/sock.c
> > > @@ -2821,6 +2821,7 @@ int __sock_cmsg_send(struct sock *sk,
> > > struct cmsghdr *cmsg,
> > >                  sockc->mark = *(u32 *)CMSG_DATA(cmsg);
> > >                  break;
> > >          case SO_TIMESTAMPING_OLD:
> > > +       case SO_TIMESTAMPING_NEW:
> > >                  if (cmsg->cmsg_len != CMSG_LEN(sizeof(u32)))
> > >                          return -EINVAL;
> > > 
> > > However, looking through the module, it seems that
> > > sk_getsockopt() has no
> > > support for SO_TIMESTAMPING_NEW either, but sk_setsockopt() has.
> > 
> > Good point. Adding the author to see if this was a simple oversight
> > or
> > there was a rationale at the time for leaving it out.
> 
> I'm fairly sure this was just a mistake on our side. For the cmsg
> case,
> I think we just missed it because there is no corresponding
> SO_TIMESTAMP{,NS}
> version of this, so it fell through the cracks.
> 
> In the patch above, I'm not entirely sure about what needs to happen
> with the old/new format, i.e. the
> 
>    sock_valbool_flag(sk, SOCK_TSTAMP_NEW, optname ==
> SO_TIMESTAMPING_NEW)
> 
> from setsockopt(). Is __sock_cmsg_send() allowed to turn on
> timestamping
> without it being first enabled using setsockopt()? If so, I think
> we need to set the flag here the same way that setsockopt does. If
> not, then I think we instead should check that the old/new format
> in the option sent via cmsg is the same that was set earlier with
> setsockopt.
> 
> For the missing getsockopt, there was even a patch earlier this year
> by Jörn-Thorben Hinz [1], but I failed to realize that we need patch
> 1/2 from his series regardless of patch 2/2.
> 
>      Arnd
> 
> [1]
> https://lore.kernel.org/lkml/20230703175048.151683-2-jthinz@mailbox.tu-berlin.de/

Re: net/core/sock.c lacks some SO_TIMESTAMPING_NEW support

[Willem de Bruijn]

Jörn-Thorben Hinz wrote:
> Hi Arnd,
> 
> thanks for indirectly pinging me here about the unfinished patches. I
> kinda forgot about them over other things happening.
> 
> Happy to look back into them, it looks like it would be helpful to
> apply them. Is it fine to just answer the remarks from earlier this
> year, after a few months, in the same mail thread? Or preferable to
> resubmit the series[1] first?

Please resubmit instead of reviving the old thread. Thanks for reviving
that.

IIRC the only open item was to limit the new BPF user to the new API?
That only applies to patch 2/2.

The missing sk_getsockopt SO_TIMESTAMPING_NEW might be breaking users,
so is best sent stand-alone to net, rather than net-next.

> Thorben
> 
> [1]
> https://lore.kernel.org/lkml/20230703175048.151683-1-jthinz@mailbox.tu-berlin.de/
> 
> On Wed, 2023-12-20 at 09:43 +0000, Arnd Bergmann wrote:
> > On Wed, Dec 20, 2023, at 04:00, Willem de Bruijn wrote:
> > > Thomas Lange wrote:
> > > > diff --git a/net/core/sock.c b/net/core/sock.c
> > > > index 16584e2dd648..a56ec1d492c9 100644
> > > > --- a/net/core/sock.c
> > > > +++ b/net/core/sock.c
> > > > @@ -2821,6 +2821,7 @@ int __sock_cmsg_send(struct sock *sk,
> > > > struct cmsghdr *cmsg,
> > > >                  sockc->mark = *(u32 *)CMSG_DATA(cmsg);
> > > >                  break;
> > > >          case SO_TIMESTAMPING_OLD:
> > > > +       case SO_TIMESTAMPING_NEW:
> > > >                  if (cmsg->cmsg_len != CMSG_LEN(sizeof(u32)))
> > > >                          return -EINVAL;
> > > > 
> > > > However, looking through the module, it seems that
> > > > sk_getsockopt() has no
> > > > support for SO_TIMESTAMPING_NEW either, but sk_setsockopt() has.
> > > 
> > > Good point. Adding the author to see if this was a simple oversight
> > > or
> > > there was a rationale at the time for leaving it out.
> > 
> > I'm fairly sure this was just a mistake on our side. For the cmsg
> > case,
> > I think we just missed it because there is no corresponding
> > SO_TIMESTAMP{,NS}
> > version of this, so it fell through the cracks.
> > 
> > In the patch above, I'm not entirely sure about what needs to happen
> > with the old/new format, i.e. the
> > 
> >    sock_valbool_flag(sk, SOCK_TSTAMP_NEW, optname ==
> > SO_TIMESTAMPING_NEW)
> > 
> > from setsockopt(). Is __sock_cmsg_send() allowed to turn on
> > timestamping
> > without it being first enabled using setsockopt()? If so, I think
> > we need to set the flag here the same way that setsockopt does. If
> > not, then I think we instead should check that the old/new format
> > in the option sent via cmsg is the same that was set earlier with
> > setsockopt.

__sock_cmsg_send can only modify a subset of the bits in the
timestamping feature bitmap, so a call to setsockopt is still needed

But there is no ordering requirement, so the __sock_cmsg_send call can
come before the setsockopt call. It would be odd, but the API allows it.
> > 
> > For the missing getsockopt, there was even a patch earlier this year
> > by Jörn-Thorben Hinz [1], but I failed to realize that we need patch
> > 1/2 from his series regardless of patch 2/2.
> > 
> >      Arnd
> > 
> > [1]
> > https://lore.kernel.org/lkml/20230703175048.151683-2-jthinz@mailbox.tu-berlin.de/
> 

Re: net/core/sock.c lacks some SO_TIMESTAMPING_NEW support

[Willem de Bruijn]

Willem de Bruijn wrote:
> Jörn-Thorben Hinz wrote:
> > Hi Arnd,
> > 
> > thanks for indirectly pinging me here about the unfinished patches. I
> > kinda forgot about them over other things happening.
> > 
> > Happy to look back into them, it looks like it would be helpful to
> > apply them. Is it fine to just answer the remarks from earlier this
> > year, after a few months, in the same mail thread? Or preferable to
> > resubmit the series[1] first?
> 
> Please resubmit instead of reviving the old thread. Thanks for reviving
> that.
> 
> IIRC the only open item was to limit the new BPF user to the new API?
> That only applies to patch 2/2.
> 
> The missing sk_getsockopt SO_TIMESTAMPING_NEW might be breaking users,
> so is best sent stand-alone to net, rather than net-next.
> 
> > Thorben
> > 
> > [1]
> > https://lore.kernel.org/lkml/20230703175048.151683-1-jthinz@mailbox.tu-berlin.de/
> > 
> > On Wed, 2023-12-20 at 09:43 +0000, Arnd Bergmann wrote:
> > > On Wed, Dec 20, 2023, at 04:00, Willem de Bruijn wrote:
> > > > Thomas Lange wrote:
> > > > > diff --git a/net/core/sock.c b/net/core/sock.c
> > > > > index 16584e2dd648..a56ec1d492c9 100644
> > > > > --- a/net/core/sock.c
> > > > > +++ b/net/core/sock.c
> > > > > @@ -2821,6 +2821,7 @@ int __sock_cmsg_send(struct sock *sk,
> > > > > struct cmsghdr *cmsg,
> > > > >                  sockc->mark = *(u32 *)CMSG_DATA(cmsg);
> > > > >                  break;
> > > > >          case SO_TIMESTAMPING_OLD:
> > > > > +       case SO_TIMESTAMPING_NEW:
> > > > >                  if (cmsg->cmsg_len != CMSG_LEN(sizeof(u32)))
> > > > >                          return -EINVAL;
> > > > > 
> > > > > However, looking through the module, it seems that
> > > > > sk_getsockopt() has no
> > > > > support for SO_TIMESTAMPING_NEW either, but sk_setsockopt() has.
> > > > 
> > > > Good point. Adding the author to see if this was a simple oversight
> > > > or
> > > > there was a rationale at the time for leaving it out.
> > > 
> > > I'm fairly sure this was just a mistake on our side. For the cmsg
> > > case,
> > > I think we just missed it because there is no corresponding
> > > SO_TIMESTAMP{,NS}
> > > version of this, so it fell through the cracks.
> > > 
> > > In the patch above, I'm not entirely sure about what needs to happen
> > > with the old/new format, i.e. the
> > > 
> > >    sock_valbool_flag(sk, SOCK_TSTAMP_NEW, optname ==
> > > SO_TIMESTAMPING_NEW)
> > > 
> > > from setsockopt(). Is __sock_cmsg_send() allowed to turn on
> > > timestamping
> > > without it being first enabled using setsockopt()? If so, I think
> > > we need to set the flag here the same way that setsockopt does. If
> > > not, then I think we instead should check that the old/new format
> > > in the option sent via cmsg is the same that was set earlier with
> > > setsockopt.
> 
> __sock_cmsg_send can only modify a subset of the bits in the
> timestamping feature bitmap, so a call to setsockopt is still needed
> 
> But there is no ordering requirement, so the __sock_cmsg_send call can
> come before the setsockopt call. It would be odd, but the API allows it.

But no timestamp is returned unless setsockopt is called. So we can
continue to rely on that for selecting SOCK_TSTAMP_NEW.

Only question is whether the kernel needs to enfornce the two
operations to be consistent in their choice between NEW and OLD. I
don't think so. If they are not, this would be a weird, likely
deliberate, edge case. It only affects the data returned to the
process, not kernel integrity.

Re: net/core/sock.c lacks some SO_TIMESTAMPING_NEW support

[Arnd Bergmann]

On Wed, Dec 20, 2023, at 15:06, Willem de Bruijn wrote:
> Willem de Bruijn wrote:
>> Jörn-Thorben Hinz wrote:
>> 
>> __sock_cmsg_send can only modify a subset of the bits in the
>> timestamping feature bitmap, so a call to setsockopt is still needed
>> 
>> But there is no ordering requirement, so the __sock_cmsg_send call can
>> come before the setsockopt call. It would be odd, but the API allows it.
>
> But no timestamp is returned unless setsockopt is called. So we can
> continue to rely on that for selecting SOCK_TSTAMP_NEW.

Ok, makes sense. In that case the one-line patch should be sufficient.

> Only question is whether the kernel needs to enfornce the two
> operations to be consistent in their choice between NEW and OLD. I
> don't think so. If they are not, this would be a weird, likely
> deliberate, edge case. It only affects the data returned to the
> process, not kernel integrity.

There is the one corner case where a file descriptor is shared
between tasks that disagree on the layout of the timestamp,
or is accessed from different parts of an application that
were built with inconsistent time32/time64 settings. Both of
these are already impossible to fix in a generic way.

      Arnd

Re: net/core/sock.c lacks some SO_TIMESTAMPING_NEW support

[Jörn-Thorben Hinz]

On Wed, 2023-12-20 at 09:53 -0500, Willem de Bruijn wrote:
> Jörn-Thorben Hinz wrote:
> > Hi Arnd,
> > 
> > thanks for indirectly pinging me here about the unfinished patches.
> > I
> > kinda forgot about them over other things happening.
> > 
> > Happy to look back into them, it looks like it would be helpful to
> > apply them. Is it fine to just answer the remarks from earlier this
> > year, after a few months, in the same mail thread? Or preferable to
> > resubmit the series[1] first?
> 
> Please resubmit instead of reviving the old thread. Thanks for
> reviving
> that.
Thanks for the hint, will do so! (Maybe after Christmas.)

> 
> IIRC the only open item was to limit the new BPF user to the new API?
> That only applies to patch 2/2.
Another point was to not change the behavior of
getsockopt(SO_TIMESTAMPING_OLD), that’s just a minor change.

About limiting BPF to the SO_TIMESTAMPING_NEW, I am unsure if this is
feasible, necessary, or even makes a difference (for a BPF program). In
many places, BPF just passes-through calls like to get-/setsockopt(),
only testing whether this call is explicitly allowed from BPF space.

Also, due to its nature, BPF code often has to re-provide defines, see
for example tools/testing/selftests/bpf/progs/bpf_tracing_net.h This is
also the case for SO_TIMESTAMPING_*. A limitation of BPF to
SO_TIMESTAMPING_NEW could only be done in the allowed get-/setsockopt()
calls, not through any BPF-provided defines.

I will take another look at this aspect and add my comments/findings to
a resubmission.

> 
> The missing sk_getsockopt SO_TIMESTAMPING_NEW might be breaking
> users,
> so is best sent stand-alone to net, rather than net-next.
Hmm, I initially sent both patches together and to bpf-next since the
second, BPF-related patch depends (for the included selftest) on the
first one already being applied.

I’m unsure how to split them because of the dependency. Would one add a
comment that commit X needs to be pulled in from net for commit Y to be
applied in bpf-next? (That sounds bound to break something.)

Also, getsockopt(SO_TIMESTAMPING_NEW) has been missing since 2019,
since SO_TIMESTAMPING_NEW was added. Do you think it is still "urgent"
enough to provide it through net instead of net-next/bpf-next?

> 
> > Thorben
> > 
> > [1]
> > https://lore.kernel.org/lkml/20230703175048.151683-1-jthinz@mailbox.tu-berlin.de/
> > 
> > On Wed, 2023-12-20 at 09:43 +0000, Arnd Bergmann wrote:
> > > On Wed, Dec 20, 2023, at 04:00, Willem de Bruijn wrote:
> > > > Thomas Lange wrote:
> > > > > diff --git a/net/core/sock.c b/net/core/sock.c
> > > > > index 16584e2dd648..a56ec1d492c9 100644
> > > > > --- a/net/core/sock.c
> > > > > +++ b/net/core/sock.c
> > > > > @@ -2821,6 +2821,7 @@ int __sock_cmsg_send(struct sock *sk,
> > > > > struct cmsghdr *cmsg,
> > > > >                  sockc->mark = *(u32 *)CMSG_DATA(cmsg);
> > > > >                  break;
> > > > >          case SO_TIMESTAMPING_OLD:
> > > > > +       case SO_TIMESTAMPING_NEW:
> > > > >                  if (cmsg->cmsg_len != CMSG_LEN(sizeof(u32)))
> > > > >                          return -EINVAL;
> > > > > 
> > > > > However, looking through the module, it seems that
> > > > > sk_getsockopt() has no
> > > > > support for SO_TIMESTAMPING_NEW either, but sk_setsockopt()
> > > > > has.
> > > > 
> > > > Good point. Adding the author to see if this was a simple
> > > > oversight
> > > > or
> > > > there was a rationale at the time for leaving it out.
> > > 
> > > I'm fairly sure this was just a mistake on our side. For the cmsg
> > > case,
> > > I think we just missed it because there is no corresponding
> > > SO_TIMESTAMP{,NS}
> > > version of this, so it fell through the cracks.
> > > 
> > > In the patch above, I'm not entirely sure about what needs to
> > > happen
> > > with the old/new format, i.e. the
> > > 
> > >    sock_valbool_flag(sk, SOCK_TSTAMP_NEW, optname ==
> > > SO_TIMESTAMPING_NEW)
> > > 
> > > from setsockopt(). Is __sock_cmsg_send() allowed to turn on
> > > timestamping
> > > without it being first enabled using setsockopt()? If so, I think
> > > we need to set the flag here the same way that setsockopt does.
> > > If
> > > not, then I think we instead should check that the old/new format
> > > in the option sent via cmsg is the same that was set earlier with
> > > setsockopt.
> 
> __sock_cmsg_send can only modify a subset of the bits in the
> timestamping feature bitmap, so a call to setsockopt is still needed
> 
> But there is no ordering requirement, so the __sock_cmsg_send call
> can
> come before the setsockopt call. It would be odd, but the API allows
> it.
> > > 
> > > For the missing getsockopt, there was even a patch earlier this
> > > year
> > > by Jörn-Thorben Hinz [1], but I failed to realize that we need
> > > patch
> > > 1/2 from his series regardless of patch 2/2.
> > > 
> > >      Arnd
> > > 
> > > [1]
> > > https://lore.kernel.org/lkml/20230703175048.151683-2-jthinz@mailbox.tu-berlin.de/
> > 
> 
> 

Re: net/core/sock.c lacks some SO_TIMESTAMPING_NEW support

[Willem de Bruijn]

Jörn-Thorben Hinz wrote:
> On Wed, 2023-12-20 at 09:53 -0500, Willem de Bruijn wrote:
> > Jörn-Thorben Hinz wrote:
> > > Hi Arnd,
> > > 
> > > thanks for indirectly pinging me here about the unfinished patches.
> > > I
> > > kinda forgot about them over other things happening.
> > > 
> > > Happy to look back into them, it looks like it would be helpful to
> > > apply them. Is it fine to just answer the remarks from earlier this
> > > year, after a few months, in the same mail thread? Or preferable to
> > > resubmit the series[1] first?
> > 
> > Please resubmit instead of reviving the old thread. Thanks for
> > reviving
> > that.
> Thanks for the hint, will do so! (Maybe after Christmas.)
> 
> > 
> > IIRC the only open item was to limit the new BPF user to the new API?
> > That only applies to patch 2/2.
> Another point was to not change the behavior of
> getsockopt(SO_TIMESTAMPING_OLD), that’s just a minor change.
> 
> About limiting BPF to the SO_TIMESTAMPING_NEW, I am unsure if this is
> feasible, necessary, or even makes a difference (for a BPF program). In
> many places, BPF just passes-through calls like to get-/setsockopt(),
> only testing whether this call is explicitly allowed from BPF space.
> 
> Also, due to its nature, BPF code often has to re-provide defines, see
> for example tools/testing/selftests/bpf/progs/bpf_tracing_net.h This is
> also the case for SO_TIMESTAMPING_*. A limitation of BPF to
> SO_TIMESTAMPING_NEW could only be done in the allowed get-/setsockopt()
> calls, not through any BPF-provided defines.
> 
> I will take another look at this aspect and add my comments/findings to
> a resubmission.
> 
> > 
> > The missing sk_getsockopt SO_TIMESTAMPING_NEW might be breaking
> > users,
> > so is best sent stand-alone to net, rather than net-next.
> Hmm, I initially sent both patches together and to bpf-next since the
> second, BPF-related patch depends (for the included selftest) on the
> first one already being applied.
> 
> I’m unsure how to split them because of the dependency. Would one add a
> comment that commit X needs to be pulled in from net for commit Y to be
> applied in bpf-next? (That sounds bound to break something.)
>
> Also, getsockopt(SO_TIMESTAMPING_NEW) has been missing since 2019,
> since SO_TIMESTAMPING_NEW was added. Do you think it is still "urgent"
> enough to provide it through net instead of net-next/bpf-next?

net gets pulled into net-next at least once a week. If you submit this
patch now, it will likely be in bpf-next by the time we get to the
second more involved patch.

This report was a reminder that the current omission can actually
break users, so having it as a fix that goes to stable is warranted.
The Fixes tag will be

Fixes: 9718475e6908 ("socket: Add SO_TIMESTAMPING_NEW")


> > 
> > > Thorben
> > > 
> > > [1]
> > > https://lore.kernel.org/lkml/20230703175048.151683-1-jthinz@mailbox.tu-berlin.de/
> > > 
> > > On Wed, 2023-12-20 at 09:43 +0000, Arnd Bergmann wrote:
> > > > On Wed, Dec 20, 2023, at 04:00, Willem de Bruijn wrote:
> > > > > Thomas Lange wrote:
> > > > > > diff --git a/net/core/sock.c b/net/core/sock.c
> > > > > > index 16584e2dd648..a56ec1d492c9 100644
> > > > > > --- a/net/core/sock.c
> > > > > > +++ b/net/core/sock.c
> > > > > > @@ -2821,6 +2821,7 @@ int __sock_cmsg_send(struct sock *sk,
> > > > > > struct cmsghdr *cmsg,
> > > > > >                  sockc->mark = *(u32 *)CMSG_DATA(cmsg);
> > > > > >                  break;
> > > > > >          case SO_TIMESTAMPING_OLD:
> > > > > > +       case SO_TIMESTAMPING_NEW:
> > > > > >                  if (cmsg->cmsg_len != CMSG_LEN(sizeof(u32)))
> > > > > >                          return -EINVAL;
> > > > > > 
> > > > > > However, looking through the module, it seems that
> > > > > > sk_getsockopt() has no
> > > > > > support for SO_TIMESTAMPING_NEW either, but sk_setsockopt()
> > > > > > has.
> > > > > 
> > > > > Good point. Adding the author to see if this was a simple
> > > > > oversight
> > > > > or
> > > > > there was a rationale at the time for leaving it out.
> > > > 
> > > > I'm fairly sure this was just a mistake on our side. For the cmsg
> > > > case,
> > > > I think we just missed it because there is no corresponding
> > > > SO_TIMESTAMP{,NS}
> > > > version of this, so it fell through the cracks.
> > > > 
> > > > In the patch above, I'm not entirely sure about what needs to
> > > > happen
> > > > with the old/new format, i.e. the
> > > > 
> > > >    sock_valbool_flag(sk, SOCK_TSTAMP_NEW, optname ==
> > > > SO_TIMESTAMPING_NEW)
> > > > 
> > > > from setsockopt(). Is __sock_cmsg_send() allowed to turn on
> > > > timestamping
> > > > without it being first enabled using setsockopt()? If so, I think
> > > > we need to set the flag here the same way that setsockopt does.
> > > > If
> > > > not, then I think we instead should check that the old/new format
> > > > in the option sent via cmsg is the same that was set earlier with
> > > > setsockopt.
> > 
> > __sock_cmsg_send can only modify a subset of the bits in the
> > timestamping feature bitmap, so a call to setsockopt is still needed
> > 
> > But there is no ordering requirement, so the __sock_cmsg_send call
> > can
> > come before the setsockopt call. It would be odd, but the API allows
> > it.
> > > > 
> > > > For the missing getsockopt, there was even a patch earlier this
> > > > year
> > > > by Jörn-Thorben Hinz [1], but I failed to realize that we need
> > > > patch
> > > > 1/2 from his series regardless of patch 2/2.
> > > > 
> > > >      Arnd
> > > > 
> > > > [1]
> > > > https://lore.kernel.org/lkml/20230703175048.151683-2-jthinz@mailbox.tu-berlin.de/
> > > 
> > 
> > 
> 

Re: net/core/sock.c lacks some SO_TIMESTAMPING_NEW support

[Jörn-Thorben Hinz]

On Thu, 2023-12-21 at 12:07 -0500, Willem de Bruijn wrote:
> Jörn-Thorben Hinz wrote:
> > On Wed, 2023-12-20 at 09:53 -0500, Willem de Bruijn wrote:
> > > Jörn-Thorben Hinz wrote:
> > > > Hi Arnd,
> > > > 
> > > > thanks for indirectly pinging me here about the unfinished
> > > > patches.
> > > > I
> > > > kinda forgot about them over other things happening.
> > > > 
> > > > Happy to look back into them, it looks like it would be helpful
> > > > to
> > > > apply them. Is it fine to just answer the remarks from earlier
> > > > this
> > > > year, after a few months, in the same mail thread? Or
> > > > preferable to
> > > > resubmit the series[1] first?
> > > 
> > > Please resubmit instead of reviving the old thread. Thanks for
> > > reviving
> > > that.
> > Thanks for the hint, will do so! (Maybe after Christmas.)
> > 
> > > 
> > > IIRC the only open item was to limit the new BPF user to the new
> > > API?
> > > That only applies to patch 2/2.
> > Another point was to not change the behavior of
> > getsockopt(SO_TIMESTAMPING_OLD), that’s just a minor change.
> > 
> > About limiting BPF to the SO_TIMESTAMPING_NEW, I am unsure if this
> > is
> > feasible, necessary, or even makes a difference (for a BPF
> > program). In
> > many places, BPF just passes-through calls like to get-
> > /setsockopt(),
> > only testing whether this call is explicitly allowed from BPF
> > space.
> > 
> > Also, due to its nature, BPF code often has to re-provide defines,
> > see
> > for example tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> > This is
> > also the case for SO_TIMESTAMPING_*. A limitation of BPF to
> > SO_TIMESTAMPING_NEW could only be done in the allowed get-
> > /setsockopt()
> > calls, not through any BPF-provided defines.
> > 
> > I will take another look at this aspect and add my
> > comments/findings to
> > a resubmission.
> > 
> > > 
> > > The missing sk_getsockopt SO_TIMESTAMPING_NEW might be breaking
> > > users,
> > > so is best sent stand-alone to net, rather than net-next.
> > Hmm, I initially sent both patches together and to bpf-next since
> > the
> > second, BPF-related patch depends (for the included selftest) on
> > the
> > first one already being applied.
> > 
> > I’m unsure how to split them because of the dependency. Would one
> > add a
> > comment that commit X needs to be pulled in from net for commit Y
> > to be
> > applied in bpf-next? (That sounds bound to break something.)
> > 
> > Also, getsockopt(SO_TIMESTAMPING_NEW) has been missing since 2019,
> > since SO_TIMESTAMPING_NEW was added. Do you think it is still
> > "urgent"
> > enough to provide it through net instead of net-next/bpf-next?
> 
> net gets pulled into net-next at least once a week. If you submit
> this
> patch now, it will likely be in bpf-next by the time we get to the
> second more involved patch.
Thank you for the explanation. I wasn’t aware that the synchronization
happens that frequently. Then it’s of course not a problem to split the
series.

I’ve submitted the old patch 1/2 on its own to net.

> 
> This report was a reminder that the current omission can actually
> break users, so having it as a fix that goes to stable is warranted.
> The Fixes tag will be
> 
> Fixes: 9718475e6908 ("socket: Add SO_TIMESTAMPING_NEW")
Agree, that sounds reasonable. The Fixes: was already present in the
old submission.

> 
> 
> > > 
> > > > Thorben
> > > > 
> > > > [1]
> > > > https://lore.kernel.org/lkml/20230703175048.151683-1-jthinz@mailbox.tu-berlin.de/
> > > > 
> > > > On Wed, 2023-12-20 at 09:43 +0000, Arnd Bergmann wrote:
> > > > > On Wed, Dec 20, 2023, at 04:00, Willem de Bruijn wrote:
> > > > > > Thomas Lange wrote:
> > > > > > > diff --git a/net/core/sock.c b/net/core/sock.c
> > > > > > > index 16584e2dd648..a56ec1d492c9 100644
> > > > > > > --- a/net/core/sock.c
> > > > > > > +++ b/net/core/sock.c
> > > > > > > @@ -2821,6 +2821,7 @@ int __sock_cmsg_send(struct sock
> > > > > > > *sk,
> > > > > > > struct cmsghdr *cmsg,
> > > > > > >                  sockc->mark = *(u32 *)CMSG_DATA(cmsg);
> > > > > > >                  break;
> > > > > > >          case SO_TIMESTAMPING_OLD:
> > > > > > > +       case SO_TIMESTAMPING_NEW:
> > > > > > >                  if (cmsg->cmsg_len !=
> > > > > > > CMSG_LEN(sizeof(u32)))
> > > > > > >                          return -EINVAL;
> > > > > > > 
> > > > > > > However, looking through the module, it seems that
> > > > > > > sk_getsockopt() has no
> > > > > > > support for SO_TIMESTAMPING_NEW either, but
> > > > > > > sk_setsockopt()
> > > > > > > has.
> > > > > > 
> > > > > > Good point. Adding the author to see if this was a simple
> > > > > > oversight
> > > > > > or
> > > > > > there was a rationale at the time for leaving it out.
> > > > > 
> > > > > I'm fairly sure this was just a mistake on our side. For the
> > > > > cmsg
> > > > > case,
> > > > > I think we just missed it because there is no corresponding
> > > > > SO_TIMESTAMP{,NS}
> > > > > version of this, so it fell through the cracks.
> > > > > 
> > > > > In the patch above, I'm not entirely sure about what needs to
> > > > > happen
> > > > > with the old/new format, i.e. the
> > > > > 
> > > > >    sock_valbool_flag(sk, SOCK_TSTAMP_NEW, optname ==
> > > > > SO_TIMESTAMPING_NEW)
> > > > > 
> > > > > from setsockopt(). Is __sock_cmsg_send() allowed to turn on
> > > > > timestamping
> > > > > without it being first enabled using setsockopt()? If so, I
> > > > > think
> > > > > we need to set the flag here the same way that setsockopt
> > > > > does.
> > > > > If
> > > > > not, then I think we instead should check that the old/new
> > > > > format
> > > > > in the option sent via cmsg is the same that was set earlier
> > > > > with
> > > > > setsockopt.
> > > 
> > > __sock_cmsg_send can only modify a subset of the bits in the
> > > timestamping feature bitmap, so a call to setsockopt is still
> > > needed
> > > 
> > > But there is no ordering requirement, so the __sock_cmsg_send
> > > call
> > > can
> > > come before the setsockopt call. It would be odd, but the API
> > > allows
> > > it.
> > > > > 
> > > > > For the missing getsockopt, there was even a patch earlier
> > > > > this
> > > > > year
> > > > > by Jörn-Thorben Hinz [1], but I failed to realize that we
> > > > > need
> > > > > patch
> > > > > 1/2 from his series regardless of patch 2/2.
> > > > > 
> > > > >      Arnd
> > > > > 
> > > > > [1]
> > > > > https://lore.kernel.org/lkml/20230703175048.151683-2-jthinz@mailbox.tu-berlin.de/
> > > > 
> > > 
> > > 
> > 
> 
> 

Re: net/core/sock.c lacks some SO_TIMESTAMPING_NEW support

[Willem de Bruijn]

Thomas Lange wrote:
> It seems that net/core/sock.c is missing support for SO_TIMESTAMPING_NEW in
> some paths.
> 
> I cross compile for a 32bit ARM system using Yocto 4.3.1, which seems to have
> 64bit time by default. This maps SO_TIMESTAMPING to SO_TIMESTAMPING_NEW which
> is expected AFAIK.
> 
> However, this breaks my application (Chrony) that sends SO_TIMESTAMPING as
> a cmsg:
> 
> sendmsg(4, {msg_name={sa_family=AF_INET, sin_port=htons(123), sin_addr=inet_addr("172.16.11.22")}, msg_namelen=16, msg_iov=[{iov_base="#\0\6 \0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., iov_len=48}], msg_iovlen=1, msg_control=[{cmsg_len=16, cmsg_level=SOL_SOCKET, cmsg_type=SO_TIMESTAMPING_NEW, cmsg_data=???}], msg_controllen=16, msg_flags=0}, 0) = -1 EINVAL (Invalid argument)
> 
> This is because __sock_cmsg_send() does not support SO_TIMESTAMPING_NEW as-is.
> 
> This patch seems to fix things and the packet is transmitted:
> 
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 16584e2dd648..a56ec1d492c9 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2821,6 +2821,7 @@ int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
>                  sockc->mark = *(u32 *)CMSG_DATA(cmsg);
>                  break;
>          case SO_TIMESTAMPING_OLD:
> +       case SO_TIMESTAMPING_NEW:
>                  if (cmsg->cmsg_len != CMSG_LEN(sizeof(u32)))
>                          return -EINVAL;
> 
> However, looking through the module, it seems that sk_getsockopt() has no
> support for SO_TIMESTAMPING_NEW either, but sk_setsockopt() has.
> Testing seems to confirm this:
> 
> setsockopt(4, SOL_SOCKET, SO_TIMESTAMPING_NEW, [1048], 4) = 0
> getsockopt(4, SOL_SOCKET, SO_TIMESTAMPING_NEW, 0x7ed5db20, [4]) = -1 ENOPROTOOPT (Protocol not available)
> 
> Patching sk_getsockopt() is not as obvious to me though.
> 
> I used a custom 6.6 kernel for my tests.
> The relevant code seems unchanged in net-next.git though.
> 
> /Thomas

The fix to getsockopt is now merged:

https://lore.kernel.org/netdev/20231221231901.67003-1-jthinz@mailbox.tu-berlin.de/T/

Do you want to send the above fix to __sock_cmsg_send?

Re: net/core/sock.c lacks some SO_TIMESTAMPING_NEW support

[Thomas Lange]

On 2024-01-02 16:26, Willem de Bruijn wrote:
> Thomas Lange wrote:
>> It seems that net/core/sock.c is missing support for SO_TIMESTAMPING_NEW in
>> some paths.
>>
>> I cross compile for a 32bit ARM system using Yocto 4.3.1, which seems to have
>> 64bit time by default. This maps SO_TIMESTAMPING to SO_TIMESTAMPING_NEW which
>> is expected AFAIK.
>>
>> However, this breaks my application (Chrony) that sends SO_TIMESTAMPING as
>> a cmsg:
>>
>> sendmsg(4, {msg_name={sa_family=AF_INET, sin_port=htons(123), sin_addr=inet_addr("172.16.11.22")}, msg_namelen=16, msg_iov=[{iov_base="#\0\6 \0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., iov_len=48}], msg_iovlen=1, msg_control=[{cmsg_len=16, cmsg_level=SOL_SOCKET, cmsg_type=SO_TIMESTAMPING_NEW, cmsg_data=???}], msg_controllen=16, msg_flags=0}, 0) = -1 EINVAL (Invalid argument)
>>
>> This is because __sock_cmsg_send() does not support SO_TIMESTAMPING_NEW as-is.
>>
>> This patch seems to fix things and the packet is transmitted:
>>
>> diff --git a/net/core/sock.c b/net/core/sock.c
>> index 16584e2dd648..a56ec1d492c9 100644
>> --- a/net/core/sock.c
>> +++ b/net/core/sock.c
>> @@ -2821,6 +2821,7 @@ int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
>>                   sockc->mark = *(u32 *)CMSG_DATA(cmsg);
>>                   break;
>>           case SO_TIMESTAMPING_OLD:
>> +       case SO_TIMESTAMPING_NEW:
>>                   if (cmsg->cmsg_len != CMSG_LEN(sizeof(u32)))
>>                           return -EINVAL;
>>
>> However, looking through the module, it seems that sk_getsockopt() has no
>> support for SO_TIMESTAMPING_NEW either, but sk_setsockopt() has.
>> Testing seems to confirm this:
>>
>> setsockopt(4, SOL_SOCKET, SO_TIMESTAMPING_NEW, [1048], 4) = 0
>> getsockopt(4, SOL_SOCKET, SO_TIMESTAMPING_NEW, 0x7ed5db20, [4]) = -1 ENOPROTOOPT (Protocol not available)
>>
>> Patching sk_getsockopt() is not as obvious to me though.
>>
>> I used a custom 6.6 kernel for my tests.
>> The relevant code seems unchanged in net-next.git though.
>>
>> /Thomas
> 
> The fix to getsockopt is now merged:
> 
> https://lore.kernel.org/netdev/20231221231901.67003-1-jthinz@mailbox.tu-berlin.de/T/
> 
> Do you want to send the above fix to __sock_cmsg_send? 

Sure, I can do that. Do you want it sent to [net]?

/Thomas

Re: net/core/sock.c lacks some SO_TIMESTAMPING_NEW support

[Willem de Bruijn]

Thomas Lange wrote:
> 
> On 2024-01-02 16:26, Willem de Bruijn wrote:
> > Thomas Lange wrote:
> >> It seems that net/core/sock.c is missing support for SO_TIMESTAMPING_NEW in
> >> some paths.
> >>
> >> I cross compile for a 32bit ARM system using Yocto 4.3.1, which seems to have
> >> 64bit time by default. This maps SO_TIMESTAMPING to SO_TIMESTAMPING_NEW which
> >> is expected AFAIK.
> >>
> >> However, this breaks my application (Chrony) that sends SO_TIMESTAMPING as
> >> a cmsg:
> >>
> >> sendmsg(4, {msg_name={sa_family=AF_INET, sin_port=htons(123), sin_addr=inet_addr("172.16.11.22")}, msg_namelen=16, msg_iov=[{iov_base="#\0\6 \0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., iov_len=48}], msg_iovlen=1, msg_control=[{cmsg_len=16, cmsg_level=SOL_SOCKET, cmsg_type=SO_TIMESTAMPING_NEW, cmsg_data=???}], msg_controllen=16, msg_flags=0}, 0) = -1 EINVAL (Invalid argument)
> >>
> >> This is because __sock_cmsg_send() does not support SO_TIMESTAMPING_NEW as-is.
> >>
> >> This patch seems to fix things and the packet is transmitted:
> >>
> >> diff --git a/net/core/sock.c b/net/core/sock.c
> >> index 16584e2dd648..a56ec1d492c9 100644
> >> --- a/net/core/sock.c
> >> +++ b/net/core/sock.c
> >> @@ -2821,6 +2821,7 @@ int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
> >>                   sockc->mark = *(u32 *)CMSG_DATA(cmsg);
> >>                   break;
> >>           case SO_TIMESTAMPING_OLD:
> >> +       case SO_TIMESTAMPING_NEW:
> >>                   if (cmsg->cmsg_len != CMSG_LEN(sizeof(u32)))
> >>                           return -EINVAL;
> >>
> >> However, looking through the module, it seems that sk_getsockopt() has no
> >> support for SO_TIMESTAMPING_NEW either, but sk_setsockopt() has.
> >> Testing seems to confirm this:
> >>
> >> setsockopt(4, SOL_SOCKET, SO_TIMESTAMPING_NEW, [1048], 4) = 0
> >> getsockopt(4, SOL_SOCKET, SO_TIMESTAMPING_NEW, 0x7ed5db20, [4]) = -1 ENOPROTOOPT (Protocol not available)
> >>
> >> Patching sk_getsockopt() is not as obvious to me though.
> >>
> >> I used a custom 6.6 kernel for my tests.
> >> The relevant code seems unchanged in net-next.git though.
> >>
> >> /Thomas
> > 
> > The fix to getsockopt is now merged:
> > 
> > https://lore.kernel.org/netdev/20231221231901.67003-1-jthinz@mailbox.tu-berlin.de/T/
> > 
> > Do you want to send the above fix to __sock_cmsg_send? 
> 
> Sure, I can do that. Do you want it sent to [net]?

Yes please. Same destination as the other patch, and same Fixes tag.