netdev - Re: [PATCH bpf-next 1/2] bpf: add get_netns

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: <Z8nVRtg7XwkOHjuv@MTARDY-M-GJC6>
Date: Thu, 6 Mar 2025 18:03:02 +0100
From: Mahe Tardy <mahe.tardy@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: daniel@...earbox.net, john.fastabend@...il.com, ast@...nel.org,
	andrii@...nel.org, jolsa@...nel.org, bpf@...r.kernel.org,
	Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next 1/2] bpf: add get_netns_cookie helper to tracing
 programs

On Mon, Mar 03, 2025 at 11:14:53AM -0800, Martin KaFai Lau wrote:
> On 3/3/25 2:14 AM, Mahe Tardy wrote:
> > On Thu, Feb 27, 2025 at 12:32:43PM -0800, Martin KaFai Lau wrote:
> > > On 2/27/25 10:28 AM, Mahe Tardy wrote:
> > > > This is needed in the context of Cilium and Tetragon to retrieve netns
> > > > cookie from hostns when traffic leaves Pod, so that we can correlate
> > > > skb->sk's netns cookie.
> > > > 
> > > > Signed-off-by: Mahe Tardy <mahe.tardy@...il.com>
> > > > ---
> > > > This is a follow-up of c221d3744ad3 ("bpf: add get_netns_cookie helper
> > > > to cgroup_skb programs") and eb62f49de7ec ("bpf: add get_netns_cookie
> > > > helper to tc programs"), adding this helper respectively to cgroup_skb
> > > > and tcx programs.
> > > > 
> > > > I looked up a patch doing a similar thing c5dbb89fc2ac ("bpf: Expose
> > > > bpf_get_socket_cookie to tracing programs") and there was an item about
> > > > "sleepable context". It seems it indeed concerns tracing and LSM progs
> > > > from reading 1e6c62a88215 ("bpf: Introduce sleepable BPF programs"). Is
> > > > this needed here?
> > > 
> > > Regarding sleepable, I think the bpf_get_netns_cookie_sock is only reading,
> > > should be fine.
> > 
> > Ok thank you.
> > 
> > > The immediate question is whether sock_net(sk) must be non-NULL for tracing.
> > 
> > We discussed this offline with Daniel Borkmann and we think that it
> > might not be the question. The get_netns_cookie(NULL) call allows us to
> > compare against get_netns_cookie(sock) to see whether the sock's netns
> > is equal to the init netns and thus dispatch different logic.
> 
> bpf_get_netns_cookie(NULL) should be fine.
> 
> I meant to ask if sock_net(sk) may return NULL for a non NULL sk. Please check.

Oh sorry for the confusion, I investigated with my humble kernel
knowledge: essentially sock_net(sk) is doing sk->sk_net->net, retrieving
the net struct representing the network namespace, to later extract the
cookie, and thus dereference the returned pointer (here is the concern).
The sk_net intermediary (in reality __sk_common.skc_net) is here because
of the possibility of switching on/off network namespaces via
CONFIG_NET_NS. It's a possible_net_t type containing (or not) the struct
net pointer, explaining why we use write/read_pnet to no-op or return
the global net ns. 

Now by adding this helper to tracing progs, it allows to call this
function in any function entry or function exit, but unlike kprobes,
it's not possible to just hook at an obvious arbitrary point in the code
where the net ns would be NULL in the sock struct. With that in mind, I
failed to crash the kernel tracing a function (some candidates were
inlined). I mostly grepped for sock_net_set, but I lack the knowledge to
guarantee that this could not happen right now or in the future. Maybe
that would be just safer to add a check and return 0 in that case if
that's ok? Not sure since the helper returns an 8-byte long opaque
number which thus includes 0 as a valid value.