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: <CAL+tcoAvOjwpvZKPPaMEWPj8YxU9G_dp_=KeJPnb6xRyq_H63w@mail.gmail.com>
Date: Wed, 16 Oct 2024 21:22:19 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: Martin KaFai Lau <martin.lau@...ux.dev>, 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, 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

On Wed, Oct 16, 2024 at 9:13 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> 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.

Thanks for the explanation. I will take Martin's advice and use the
CGROUP_SOCK_OPS static key. So I don't have to take efforts to
implement the inc/dec/enable/disable the static key

Thanks,
Jason