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@xxxxxxxxxxxxxxxxxxxx/ > > Link: > > https://lore.kernel.org/all/20231221231901.67003-1-jthinz@xxxxxxxxxxxxxxxxxxxx/ > > Cc: Arnd Bergmann <arnd@xxxxxxxx> > > Cc: Deepa Dinamani <deepa.kernel@xxxxxxxxx> > > Cc: Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx> > > Signed-off-by: Jörn-Thorben Hinz <j-t.hinz@xxxxxxxxxxxxxxxxxxx> > >