Message-ID: <4ac608c49852fae8e6707508a00b39dcfd77eed2.camel@mailbox.tu-berlin.de>
Date: Fri, 22 Dec 2023 00:32:29 +0100
From: Jörn-Thorben Hinz <jthinz@...lbox.tu-berlin.de>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>, Arnd Bergmann
	<arnd@...db.de>, Thomas Lange <thomas@...elatus.se>, Netdev
	<netdev@...r.kernel.org>, Deepa Dinamani <deepa.kernel@...il.com>, "John
 Fastabend" <john.fastabend@...il.com>
Subject: Re: net/core/sock.c lacks some SO_TIMESTAMPING_NEW support

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/
> > > > 
> > > 
> > > 
> > 
> 
> 

Powered by blists - more mailing lists