Date: Thu, 18 Jan 2024 12:04:41 +0100
From: Jörn-Thorben Hinz <j-t.hinz@...mni.tu-berlin.de>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>, bpf@...r.kernel.org,
  linux-kernel@...r.kernel.org, netdev@...r.kernel.org, 
 linux-kselftest@...r.kernel.org
Cc: Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann
 <daniel@...earbox.net>,  Andrii Nakryiko <andrii@...nel.org>, Martin KaFai
 Lau <martin.lau@...ux.dev>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo
 Abeni <pabeni@...hat.com>, Shuah Khan <shuah@...nel.org>,  Arnd Bergmann
 <arnd@...db.de>, Deepa Dinamani <deepa.kernel@...il.com>
Subject: Re: [PATCH bpf-next] bpf: Allow setting SO_TIMESTAMPING* with
 bpf_setsockopt()

On Tue, 2024-01-16 at 10:17 -0500, Willem de Bruijn wrote:
> Jörn-Thorben Hinz wrote:
> > A BPF application, e.g., a TCP congestion control, might benefit
> > from or
> > even require precise (=hardware) 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:
> 
>                 if (sock_flag(sk, SOCK_TSTAMP_NEW))
>                         put_cmsg_scm_timestamping64(msg, tss);
>                 else
>                         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.

> 
> 
> 
>  
> > Link:
> > https://lore.kernel.org/lkml/20230703175048.151683-1-jthinz@mailbox.tu-berlin.de/
> > Link:
> > https://lore.kernel.org/all/20231221231901.67003-1-jthinz@mailbox.tu-berlin.de/
> > Cc: Arnd Bergmann <arnd@...db.de>
> > Cc: Deepa Dinamani <deepa.kernel@...il.com>
> > Cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>
> > Signed-off-by: Jörn-Thorben Hinz <j-t.hinz@...mni.tu-berlin.de>
> 
> 

Powered by blists - more mailing lists