Received-SPF: pass (google.com: domain of linux-kernel+bounces-30221-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 18 Jan 2024 09:46:16 -0500
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: =?UTF-8?B?SsO2cm4tVGhvcmJlbiBIaW56?= <j-t.hinz@alumni.tu-berlin.de>, 
 Willem de Bruijn <willemdebruijn.kernel@gmail.com>, 
 bpf@vger.kernel.org, 
 linux-kernel@vger.kernel.org, 
 netdev@vger.kernel.org, 
 linux-kselftest@vger.kernel.org
Cc: Alexei Starovoitov <ast@kernel.org>, 
 Daniel Borkmann <daniel@iogearbox.net>, 
 Andrii Nakryiko <andrii@kernel.org>, 
 Martin KaFai Lau <martin.lau@linux.dev>, 
 "David S. Miller" <davem@davemloft.net>, 
 Eric Dumazet <edumazet@google.com>, 
 Jakub Kicinski <kuba@kernel.org>, 
 Paolo Abeni <pabeni@redhat.com>, 
 Shuah Khan <shuah@kernel.org>, 
 Arnd Bergmann <arnd@arndb.de>, 
 Deepa Dinamani <deepa.kernel@gmail.com>
Message-ID: <65a939b85a763_1c8cde29423@willemb.c.googlers.com.notmuch>
In-Reply-To: <f4e27abc6741c175b4b1baf1331c30aaedeab290.camel@alumni.tu-berlin.de>
References: <20240115134110.11624-1-j-t.hinz@alumni.tu-berlin.de>
 <65a69e1be51ef_380df0294d9@willemb.c.googlers.com.notmuch>
 <f4e27abc6741c175b4b1baf1331c30aaedeab290.camel@alumni.tu-berlin.de>
Subject: Re: [PATCH bpf-next] bpf: Allow setting SO_TIMESTAMPING* with
 bpf_setsockopt()
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable

J=C3=B6rn-Thorben Hinz wrote:
> On Tue, 2024-01-16 at 10:17 -0500, Willem de Bruijn wrote:
> > J=C3=B6rn-Thorben Hinz wrote:
> > > A BPF application, e.g., a TCP congestion control, might benefit
> > > from or
> > > even require precise (=3Dhardware) packet timestamps. These
> > > timestamps are
> > > already available through __sk_buff.hwtstamp and
> > > bpf_sock_ops.skb_hwtstamp, but could not be requested: BPF programs=

> > > were
> > > not allowed to set SO_TIMESTAMPING* on sockets.
> > > =

> > > 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.
> > > =

> > > This patch had previously been submitted in a two-part series
> > > (first
> > > link below). The second patch has been independently applied in
> > > commit
> > > 7f6ca95d16b9 ("net: Implement missing
> > > getsockopt(SO_TIMESTAMPING_NEW)")
> > > (second link below).
> > > =

> > > On the earlier submission, there was the open question whether to
> > > only
> > > allow, thus enforce, SO_TIMESTAMPING_NEW in this patch:
> > > =

> > > For a BPF program, this won't make a difference: A timestamp, when
> > > accessed through the fields mentioned above, is directly read from
> > > skb_shared_info.hwtstamps, independent of the places where NEW/OLD
> > > is
> > > relevant. See bpf_convert_ctx_access() besides others.
> > > =

> > > I am unsure, though, when it comes to the interconnection of user
> > > space
> > > and BPF "space", when both are interested in the timestamps. I
> > > think it
> > > would cause an unsolvable conflict when user space is bound to use
> > > SO_TIMESTAMPING_OLD with a BPF program only allowed to set
> > > SO_TIMESTAMPING_NEW *on the same socket*? Please correct me if I'm
> > > mistaken.
> > =

> > The difference between OLD and NEW only affects the system calls. It
> > is not reflected in how the data is stored in the skb, or how BPF can=

> > read the data. A process setting SO_TIMESTAMPING_OLD will still allow=

> > BPF to read data using SO_TIMESTAMPING_NEW.
> > =

> > But, he one place where I see a conflict is in setting sock_flag
> > SOCK_TSTAMP_NEW. That affects what getsockopt returns and which cmsg
> > is written:
> > =

> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 if (sock_flag(sk, SOCK_TSTAMP_NEW))
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 put=
_cmsg_scm_timestamping64(msg, tss);
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 else
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 put=
_cmsg_scm_timestamping(msg, tss);
> > =

> > So a process could issue setsockopt SO_TIMESTAMPING_OLD followed by
> > a BPF program that issues setsockopt SO_TIMESTAMPING_NEW and this
> > would flip SOCK_TSTAMP_NEW.
> > =

> > Just allowing BPF to set SO_TIMESTAMPING_OLD does not fix it, as it
> > just adds the inverse case.
> Thanks for elaborating on this. I see I only thought of half the
> possible conflicting situations.
> =

> > =

> > A related problem is how does the BPF program know which of the two
> > variants to set. The BPF program is usually compiled and loaded
> > independently of the running process.
> True, that is an additional challenge. And with respect to CO-RE, I
> think a really portable BPF program could (or at least should) not even=

> decide on NEW or OLD at compile time.
> =

> > =

> > Perhaps one option is to fail the setsockop if it would flip
> > sock_flag SOCK_TSTAMP_NEW. But only if called from BPF, as else it
> > changes existing ABI.
> > =

> > Then a BPF program can attempt to set SO_TIMESTAMPING NEW, be
> > prepared to handle a particular errno, and retry with
> > SO_TIMESTAMPING_OLD.
> Hmm, would be possible, yes. But sounds like a weird and unexpected
> special-case behavior to the occasional BPF user.

Agreed. So perhaps we're back to where we say: this is a new feature
for BPF, only support it on modern environments that use
SO_TIMESTAMPING_NEW?