Re: [PATCH bpf-next] bpf: Allow setting SO_TIMESTAMPING* with bpf_setsockopt()

[Willem de Bruijn]

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.

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.

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.



 
> Link: https://lore.kernel.org/lkml/20230703175048.151683-1-jthinz@xxxxxxxxxxxxxxxxxxxx/
> Link: https://lore.kernel.org/all/20231221231901.67003-1-jthinz@xxxxxxxxxxxxxxxxxxx/
> 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>