netdev - Re: [PATCH net-next 0/9] net-timestamp: bpf extension to equip applications transparently

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6b10ed31-c53f-4f99-9c23-e1ba34aa0905@linux.dev>
Date: Wed, 9 Oct 2024 10:27:59 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Jason Xing <kerneljasonxing@...il.com>,
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, dsahern@...nel.org, willemb@...gle.com, ast@...nel.org,
 daniel@...earbox.net, andrii@...nel.org, martin.lau@...ux.dev,
 eddyz87@...il.com, song@...nel.org, yonghong.song@...ux.dev,
 john.fastabend@...il.com, kpsingh@...nel.org, sdf@...ichev.me,
 haoluo@...gle.com, jolsa@...nel.org, bpf@...r.kernel.org,
 netdev@...r.kernel.org, Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next 0/9] net-timestamp: bpf extension to equip
 applications transparently

On 09/10/2024 02:05, Jason Xing wrote:
> On Wed, Oct 9, 2024 at 7:22 AM Jason Xing <kerneljasonxing@...il.com> wrote:
>>
>> On Wed, Oct 9, 2024 at 2:44 AM Willem de Bruijn
>> <willemdebruijn.kernel@...il.com> wrote:
>>>
>>> Jason Xing wrote:
>>>> From: Jason Xing <kernelxing@...cent.com>
>>>>
>>>> A few weeks ago, I planned to extend SO_TIMESTMAMPING feature by using
>>>> tracepoint to print information (say, tstamp) so that we can
>>>> transparently equip applications with this feature and require no
>>>> modification in user side.
>>>>
>>>> Later, we discussed at netconf and agreed that we can use bpf for better
>>>> extension, which is mainly suggested by John Fastabend and Willem de
>>>> Bruijn. Many thanks here! So I post this series to see if we have a
>>>> better solution to extend.
>>>>
>>>> This approach relies on existing SO_TIMESTAMPING feature, for tx path,
>>>> users only needs to pass certain flags through bpf program to make sure
>>>> the last skb from each sendmsg() has timestamp related controlled flag.
>>>> For rx path, we have to use bpf_setsockopt() to set the sk->sk_tsflags
>>>> and wait for the moment when recvmsg() is called.
>>>
>>> As you mention, overall I am very supportive of having a way to add
>>> timestamping by adminstrators, without having to rebuild applications.
>>> BPF hooks seem to be the right place for this.
>>>
>>> There is existing kprobe/kretprobe/kfunc support. Supporting
>>> SO_TIMESTAMPING directly may be useful due to its targeted feature
>>> set, and correlation between measurements for the same data in the
>>> stream.
>>>
>>>> After this series, we could step by step implement more advanced
>>>> functions/flags already in SO_TIMESTAMPING feature for bpf extension.
>>>
>>> My main implementation concern is where this API overlaps with the
>>> existing user API, and how they might conflict. A few questions in the
>>> patches.
>>
>> Agreed. That's also what I'm concerned about. So I decided to ask for
>> related experts' help.
>>
>> How to deal with it without interfering with the existing apps in the
>> right way is the key problem.
> 
> What I try to implement is let the bpf program have the highest
> precedence. It's similar to RTO min, see the commit as an example:
> 
> commit f086edef71be7174a16c1ed67ac65a085cda28b1
> Author: Kevin Yang <yyd@...gle.com>
> Date:   Mon Jun 3 21:30:54 2024 +0000
> 
>      tcp: add sysctl_tcp_rto_min_us
> 
>      Adding a sysctl knob to allow user to specify a default
>      rto_min at socket init time, other than using the hard
>      coded 200ms default rto_min.
> 
>      Note that the rto_min route option has the highest precedence
>      for configuring this setting, followed by the TCP_BPF_RTO_MIN
>      socket option, followed by the tcp_rto_min_us sysctl.
> 
> It includes three cases, 1) route option, 2) bpf option, 3) sysctl.
> The first priority can override others. It doesn't have a good
> chance/point to restore the icsk_rto_min field if users want to
> shutdown the bpf program because it is set in
> bpf_sol_tcp_setsockopt().

rto_min example is slightly different. With tcp_rto_min the doesn't
expect any data to come back to user space while for timestamping the
app may be confused directly by providing more data, or by not providing
expected data. I believe some hint about requestor of the data is needed
here. It will also help to solve the problem of populating sk_err_queue
mentioned by Martin.