* Re: [PATCH bpf-next 1/2] bpf: add get_netns_cookie helper to tracing programs [not found] <20250227182830.90863-1-mahe.tardy@gmail.com> @ 2025-02-27 20:32 ` Martin KaFai Lau 2025-03-03 10:14 ` Mahe Tardy 0 siblings, 1 reply; 6+ messages in thread From: Martin KaFai Lau @ 2025-02-27 20:32 UTC (permalink / raw) To: Mahe Tardy Cc: daniel, john.fastabend, ast, andrii, jolsa, bpf, Network Development 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@gmail.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. The immediate question is whether sock_net(sk) must be non-NULL for tracing. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: add get_netns_cookie helper to tracing programs 2025-02-27 20:32 ` [PATCH bpf-next 1/2] bpf: add get_netns_cookie helper to tracing programs Martin KaFai Lau @ 2025-03-03 10:14 ` Mahe Tardy 2025-03-03 19:14 ` Martin KaFai Lau 0 siblings, 1 reply; 6+ messages in thread From: Mahe Tardy @ 2025-03-03 10:14 UTC (permalink / raw) To: Martin KaFai Lau Cc: daniel, john.fastabend, ast, andrii, jolsa, bpf, Network Development 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@gmail.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. Given we (in Tetragon) historically used tracing programs when no appropriate network hook was available on older kernels I can foresee how it can still be useful in such programs. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: add get_netns_cookie helper to tracing programs 2025-03-03 10:14 ` Mahe Tardy @ 2025-03-03 19:14 ` Martin KaFai Lau 2025-03-06 17:03 ` Mahe Tardy 0 siblings, 1 reply; 6+ messages in thread From: Martin KaFai Lau @ 2025-03-03 19:14 UTC (permalink / raw) To: Mahe Tardy Cc: daniel, john.fastabend, ast, andrii, jolsa, bpf, Network Development 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@gmail.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. > > Given we (in Tetragon) historically used tracing programs when no > appropriate network hook was available on older kernels I can foresee > how it can still be useful in such programs. > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: add get_netns_cookie helper to tracing programs 2025-03-03 19:14 ` Martin KaFai Lau @ 2025-03-06 17:03 ` Mahe Tardy 2025-03-07 23:06 ` Martin KaFai Lau 0 siblings, 1 reply; 6+ messages in thread From: Mahe Tardy @ 2025-03-06 17:03 UTC (permalink / raw) To: Martin KaFai Lau Cc: daniel, john.fastabend, ast, andrii, jolsa, bpf, Network Development 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@gmail.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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: add get_netns_cookie helper to tracing programs 2025-03-06 17:03 ` Mahe Tardy @ 2025-03-07 23:06 ` Martin KaFai Lau 2025-03-08 6:17 ` Yonghong Song 0 siblings, 1 reply; 6+ messages in thread From: Martin KaFai Lau @ 2025-03-07 23:06 UTC (permalink / raw) To: Mahe Tardy Cc: daniel, john.fastabend, ast, andrii, jolsa, bpf, Network Development On 3/6/25 9:03 AM, Mahe Tardy wrote: >>>> 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 Thanks for checking. I took a quick look at the callers of sock_net_set. I suspect "fentry/sk_prot_alloc" and "lsm/sk_alloc" could have a NULL? > 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. I assume net_cookie 0 is invalid, but then it leaks the implementation details of what is a valid cookie in a uapi helper * u64 bpf_get_netns_cookie(void *ctx) * ... * Return * A 8-byte long opaque number Note that, the tracing program can already read most fields of the sk, including sk->sk_net.net->net_cookie. Therefore, what this patch aims to achieve has already been supported in tracing. It can also save a helper call. The only thing that may be missing in your use case is determining the init_net. I don't think reading a global kernel variable has been supported yet. Not sure if init_net must have net_cookie 1. Otherwise, we could consider to add a kfunc to return &init_net, which could be used to compare with sk->sk_net.net. Having a pointer to &init_net might be more useful for other tracing use cases in general. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: add get_netns_cookie helper to tracing programs 2025-03-07 23:06 ` Martin KaFai Lau @ 2025-03-08 6:17 ` Yonghong Song 0 siblings, 0 replies; 6+ messages in thread From: Yonghong Song @ 2025-03-08 6:17 UTC (permalink / raw) To: Martin KaFai Lau, Mahe Tardy Cc: daniel, john.fastabend, ast, andrii, jolsa, bpf, Network Development On 3/7/25 3:06 PM, Martin KaFai Lau wrote: > On 3/6/25 9:03 AM, Mahe Tardy wrote: >>>>> 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 > > Thanks for checking. > > I took a quick look at the callers of sock_net_set. I suspect > "fentry/sk_prot_alloc" and "lsm/sk_alloc" could have a NULL? > >> 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. > > I assume net_cookie 0 is invalid, but then it leaks the implementation > details of what is a valid cookie in a uapi helper > > * u64 bpf_get_netns_cookie(void *ctx) > * ... > * Return > * A 8-byte long opaque number > > Note that, the tracing program can already read most fields of the sk, > including sk->sk_net.net->net_cookie. Therefore, what this patch aims > to achieve has already been supported in tracing. It can also save a > helper call. > > The only thing that may be missing in your use case is determining the > init_net. I don't think reading a global kernel variable has been > supported yet. Not sure if init_net must have net_cookie 1. Otherwise, > we could consider to add a kfunc to return &init_net, which could be > used to compare with sk->sk_net.net. Having a pointer to &init_net > might be more useful for other tracing use cases in general. There is the workaround for this tracing use case. 1. Declare a global variable in the bpf program, e.g. struct net *init_net; 2. After skel_open and before skel_load, find init_net address (from /proc/kallsyms) and assign the address to skel->bss->init_net. 3. In the prog, do struct net *netns = bpf_rdonly_cast(init_net, bpf_core_type_id_kernel(struct net)); bpf_printk("%u\n", netns->net_cookie); There is an effort to add global variables to BTF. See https://lore.kernel.org/bpf/20250207012045.2129841-1-stephen.s.brennan@oracle.com/ The recommended way is to put these global variables in a module to avoid consume too much kernel memory unconditionally. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-03-08 6:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20250227182830.90863-1-mahe.tardy@gmail.com>
2025-02-27 20:32 ` [PATCH bpf-next 1/2] bpf: add get_netns_cookie helper to tracing programs Martin KaFai Lau
2025-03-03 10:14 ` Mahe Tardy
2025-03-03 19:14 ` Martin KaFai Lau
2025-03-06 17:03 ` Mahe Tardy
2025-03-07 23:06 ` Martin KaFai Lau
2025-03-08 6:17 ` Yonghong Song
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).