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

["Arnd Bergmann" <arnd@arndb.de>]

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/