netdev - Re: [PATCH net-next v2 04/12] net-timestamp: add static key to control the whole bpf extension

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: <670fbbf57b606_3422be294a9@willemb.c.googlers.com.notmuch>
Date: Wed, 16 Oct 2024 09:13:25 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>, 
 Martin KaFai Lau <martin.lau@...ux.dev>
Cc: davem@...emloft.net, 
 edumazet@...gle.com, 
 kuba@...nel.org, 
 pabeni@...hat.com, 
 dsahern@...nel.org, 
 willemdebruijn.kernel@...il.com, 
 willemb@...gle.com, 
 ast@...nel.org, 
 daniel@...earbox.net, 
 andrii@...nel.org, 
 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 v2 04/12] net-timestamp: add static key to
 control the whole bpf extension

Jason Xing wrote:
> On Wed, Oct 16, 2024 at 2:31 PM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
> >
> > On 10/15/24 6:04 PM, Jason Xing wrote:
> > > To be honest, I considered how to disable the static key. Like you
> > > said, I failed to find a good chance that I can accurately disable it.
> >
> > It at least needs to be disabled whenever that bpf prog got detached.
> >
> > >
> > >> The bpf prog may be detached also. (IF) it ends up staying with the
> > >> cgroup/sockops interface, it should depend on the existing static key in
> > >> cgroup_bpf_enabled(CGROUP_SOCK_OPS) instead of adding another one.
> >
> > > Are you suggesting that we need to remove the current static key? In
> > > the previous thread, the reason why Willem came up with this idea is,
> > > I think, to avoid affect the non-bpf timestamping feature.
> >
> > Take a look at cgroup_bpf_enabled(CGROUP_SOCK_OPS). There is a static key. I am
> > saying to use that existing key. afaict, the newly added bpf_tstamp_control key
> > is mainly an optimization. Yes, cgroup_bpf_enabled(CGROUP_SOCK_OPS) is less
> > granular but it has the needed accounting to disable whenever the bpf prog got
> > detached, so better just reuse the cgroup_bpf_enabled(CGROUP_SOCK_OPS).
> 
> Good suggestion. Good thing is that I don't need to figure out a
> proper place to disable it any more. I can directly use
> cgroup_bpf_enabled(CGROUP_SOCK_OPS) to test if the timestamp should be
> printed with BPF program loaded.
> 
> BTW, I found that we don't implement how to disable the ip4_min_ttl
> static key. Sometimes, I'm confused whether we have to disable it at a
> certain time.

In this case it would be fine to not disable it at all.

The crux is that it is disabled on the vast majority of machines not
using the feature. If a socket uses the feature, adding the small cost
of the branches on the rest of the system is fine.

Disabling requires refcounting usage. Sometimes the complexity and
cost of that outweights the benefit.