netdev - Re: [PATCH bpf-next 3/7] bpf: add netns cookie and enable it for bpf cgroup hooks

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: <99b27ef9-d8c5-c00b-2562-1b385ac205d0@iogearbox.net>
Date:   Sat, 28 Mar 2020 03:16:06 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     m@...bda.lt, joe@...d.net.nz, bpf@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next 3/7] bpf: add netns cookie and enable it for bpf
 cgroup hooks

On 3/28/20 2:48 AM, Alexei Starovoitov wrote:
> On Fri, Mar 27, 2020 at 04:58:52PM +0100, Daniel Borkmann wrote:
>> + *
>> + * u64 bpf_get_netns_cookie(void *ctx)
>> + * 	Description
>> + * 		Retrieve the cookie (generated by the kernel) of the network
>> + * 		namespace the input *ctx* is associated with. The network
>> + * 		namespace cookie remains stable for its lifetime and provides
>> + * 		a global identifier that can be assumed unique. If *ctx* is
>> + * 		NULL, then the helper returns the cookie for the initial
>> + * 		network namespace. The cookie itself is very similar to that
>> + * 		of bpf_get_socket_cookie() helper, but for network namespaces
>> + * 		instead of sockets.
> 
> All new helpers in this patch and few others are missing 'flags' argument.
> Yes. It's kinda hard right now to come up with a use case for the flags,
> since all helpers look kinda trivial, simple, and single purpose.
> But the same thing happened with bpf_send_signal(). It felt that there is no
> way to extend it. Yet later we had to add bpf_send_signal_thread() which could
> have been handled with flags if they were there. So please add flags to all new
> helpers though it might seem redundant.

We have very similar helpers for almost 2yrs now, that is, bpf_get_socket_cookie()
and bpf_skb_ancestor_cgroup_id(). Both no extra 'unused flags' arg and they are
simple enough to do exactly what we expect them to do, I also haven't seen any
reason to extend them further so far (otherwise we have have new ones by now). The
two added here are very much analogue to this, so breaking this consistency is
super ugly just to add empty flags now. :/ Given the timeframe we have these by now
and given they do one simple thing, what is the harm to add a new helper in future
iff really needed rather than uglifying with flags now (I would understand it for
complex helpers though where we use this practice)? Just recently we added bpf_jiffies64()
(5576b991e9c1 ("bpf: Add BPF_FUNC_jiffies64")); no flag either and it has similar
simplicity.

Thanks,
Daniel