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-berli... Link: https://lore.kernel.org/all/20231221231901.67003-1-jthinz@mailbox.tu-berlin.... Cc: Arnd Bergmann arnd@arndb.de Cc: Deepa Dinamani deepa.kernel@gmail.com Cc: Willem de Bruijn willemdebruijn.kernel@gmail.com Signed-off-by: Jörn-Thorben Hinz j-t.hinz@alumni.tu-berlin.de