diff mbox series

[v2,bpf-next,3/4] bpf: Allow using bpf_sk_storage in FENTRY/FEXIT/RAW_TP

Message ID 20201112211313.2587383-1-kafai@fb.com
State New
Headers show
Series bpf: Enable bpf_sk_storage for FENTRY/FEXIT/RAW_TP | expand

Commit Message

Martin KaFai Lau Nov. 12, 2020, 9:13 p.m. UTC
This patch enables the FENTRY/FEXIT/RAW_TP tracing program to use
the bpf_sk_storage_(get|delete) helper, so those tracing programs
can access the sk's bpf_local_storage and the later selftest
will show some examples.

The bpf_sk_storage is currently used in bpf-tcp-cc, tc,
cg sockops...etc which is running either in softirq or
task context.

This patch adds bpf_sk_storage_get_tracing_proto and
bpf_sk_storage_delete_tracing_proto.  They will check
in runtime that the helpers can only be called when serving
softirq or running in a task context.  That should enable
most common tracing use cases on sk.

During the load time, the new tracing_allowed() function
will ensure the tracing prog using the bpf_sk_storage_(get|delete)
helper is not tracing any bpf_sk_storage*() function itself.
The sk is passed as "void *" when calling into bpf_local_storage.

This patch only allows tracing a kernel function.

Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 include/net/bpf_sk_storage.h |  2 +
 kernel/trace/bpf_trace.c     |  5 +++
 net/core/bpf_sk_storage.c    | 74 ++++++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+)

Comments

Jakub Kicinski Nov. 15, 2020, 1:17 a.m. UTC | #1
On Thu, 12 Nov 2020 13:13:13 -0800 Martin KaFai Lau wrote:
> This patch adds bpf_sk_storage_get_tracing_proto and
> bpf_sk_storage_delete_tracing_proto.  They will check
> in runtime that the helpers can only be called when serving
> softirq or running in a task context.  That should enable
> most common tracing use cases on sk.

> +	if (!in_serving_softirq() && !in_task())

This is a curious combination of checks. Would you mind indulging me
with an explanation?
Martin KaFai Lau Nov. 16, 2020, 5:37 p.m. UTC | #2
On Sat, Nov 14, 2020 at 05:17:20PM -0800, Jakub Kicinski wrote:
> On Thu, 12 Nov 2020 13:13:13 -0800 Martin KaFai Lau wrote:
> > This patch adds bpf_sk_storage_get_tracing_proto and
> > bpf_sk_storage_delete_tracing_proto.  They will check
> > in runtime that the helpers can only be called when serving
> > softirq or running in a task context.  That should enable
> > most common tracing use cases on sk.
> 
> > +	if (!in_serving_softirq() && !in_task())
> 
> This is a curious combination of checks. Would you mind indulging me
> with an explanation?
The current lock usage in bpf_local_storage.c is only expected to
run in either of these contexts.
Jakub Kicinski Nov. 16, 2020, 6 p.m. UTC | #3
On Mon, 16 Nov 2020 09:37:34 -0800 Martin KaFai Lau wrote:
> On Sat, Nov 14, 2020 at 05:17:20PM -0800, Jakub Kicinski wrote:
> > On Thu, 12 Nov 2020 13:13:13 -0800 Martin KaFai Lau wrote:  
> > > This patch adds bpf_sk_storage_get_tracing_proto and
> > > bpf_sk_storage_delete_tracing_proto.  They will check
> > > in runtime that the helpers can only be called when serving
> > > softirq or running in a task context.  That should enable
> > > most common tracing use cases on sk.  
> >   
> > > +	if (!in_serving_softirq() && !in_task())  
> > 
> > This is a curious combination of checks. Would you mind indulging me
> > with an explanation?  
> The current lock usage in bpf_local_storage.c is only expected to
> run in either of these contexts.

:)

Locks that can run in any context but preempt disabled or softirq
disabled?

Let me cut to the chase. Are you sure you didn't mean to check
if (irq_count()) ?
Jakub Kicinski Nov. 16, 2020, 6:02 p.m. UTC | #4
On Mon, 16 Nov 2020 10:00:04 -0800 Jakub Kicinski wrote:
> irq_count()

Umpf. I meant (in_irq() || in_nmi()), don't care about sirq.
Martin KaFai Lau Nov. 16, 2020, 6:37 p.m. UTC | #5
On Mon, Nov 16, 2020 at 10:00:04AM -0800, Jakub Kicinski wrote:
> On Mon, 16 Nov 2020 09:37:34 -0800 Martin KaFai Lau wrote:
> > On Sat, Nov 14, 2020 at 05:17:20PM -0800, Jakub Kicinski wrote:
> > > On Thu, 12 Nov 2020 13:13:13 -0800 Martin KaFai Lau wrote:  
> > > > This patch adds bpf_sk_storage_get_tracing_proto and
> > > > bpf_sk_storage_delete_tracing_proto.  They will check
> > > > in runtime that the helpers can only be called when serving
> > > > softirq or running in a task context.  That should enable
> > > > most common tracing use cases on sk.  
> > >   
> > > > +	if (!in_serving_softirq() && !in_task())  
> > > 
> > > This is a curious combination of checks. Would you mind indulging me
> > > with an explanation?  
> > The current lock usage in bpf_local_storage.c is only expected to
> > run in either of these contexts.
> 
> :)
> 
> Locks that can run in any context but preempt disabled or softirq
> disabled?
Not exactly. e.g. running from irq won't work.

> 
> Let me cut to the chase. Are you sure you didn't mean to check
> if (irq_count()) ?
so, no.

From preempt.h:

/*
 * ...
 * in_interrupt() - We're in NMI,IRQ,SoftIRQ context or have BH disabled
 * ...
 */
#define in_interrupt()          (irq_count())
Jakub Kicinski Nov. 16, 2020, 6:43 p.m. UTC | #6
On Mon, 16 Nov 2020 10:37:49 -0800 Martin KaFai Lau wrote:
> On Mon, Nov 16, 2020 at 10:00:04AM -0800, Jakub Kicinski wrote:
> > Locks that can run in any context but preempt disabled or softirq
> > disabled?  
> Not exactly. e.g. running from irq won't work.
> 
> > Let me cut to the chase. Are you sure you didn't mean to check
> > if (irq_count()) ?  
> so, no.
> 
> From preempt.h:
> 
> /*
>  * ...
>  * in_interrupt() - We're in NMI,IRQ,SoftIRQ context or have BH disabled
>  * ...
>  */
> #define in_interrupt()          (irq_count())

Right, as I said in my correction (in_irq() || in_nmi()).

Just to spell it out AFAIU in_serving_softirq() will return true when
softirq is active and interrupted by a hard irq or an NMI.
Martin KaFai Lau Nov. 16, 2020, 6:51 p.m. UTC | #7
On Mon, Nov 16, 2020 at 10:43:40AM -0800, Jakub Kicinski wrote:
> On Mon, 16 Nov 2020 10:37:49 -0800 Martin KaFai Lau wrote:
> > On Mon, Nov 16, 2020 at 10:00:04AM -0800, Jakub Kicinski wrote:
> > > Locks that can run in any context but preempt disabled or softirq
> > > disabled?  
> > Not exactly. e.g. running from irq won't work.
> > 
> > > Let me cut to the chase. Are you sure you didn't mean to check
> > > if (irq_count()) ?  
> > so, no.
> > 
> > From preempt.h:
> > 
> > /*
> >  * ...
> >  * in_interrupt() - We're in NMI,IRQ,SoftIRQ context or have BH disabled
> >  * ...
> >  */
> > #define in_interrupt()          (irq_count())
> 
> Right, as I said in my correction (in_irq() || in_nmi()).
> 
> Just to spell it out AFAIU in_serving_softirq() will return true when
> softirq is active and interrupted by a hard irq or an NMI.
I see what you have been getting at now.

Good point. will post a fix.
diff mbox series

Patch

diff --git a/include/net/bpf_sk_storage.h b/include/net/bpf_sk_storage.h
index 3c516dd07caf..0e85713f56df 100644
--- a/include/net/bpf_sk_storage.h
+++ b/include/net/bpf_sk_storage.h
@@ -20,6 +20,8 @@  void bpf_sk_storage_free(struct sock *sk);
 
 extern const struct bpf_func_proto bpf_sk_storage_get_proto;
 extern const struct bpf_func_proto bpf_sk_storage_delete_proto;
+extern const struct bpf_func_proto bpf_sk_storage_get_tracing_proto;
+extern const struct bpf_func_proto bpf_sk_storage_delete_tracing_proto;
 
 struct bpf_local_storage_elem;
 struct bpf_sk_storage_diag;
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index e4515b0f62a8..cfce60ad1cb5 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -16,6 +16,7 @@ 
 #include <linux/syscalls.h>
 #include <linux/error-injection.h>
 #include <linux/btf_ids.h>
+#include <net/bpf_sk_storage.h>
 
 #include <uapi/linux/bpf.h>
 #include <uapi/linux/btf.h>
@@ -1735,6 +1736,10 @@  tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_skc_to_tcp_request_sock_proto;
 	case BPF_FUNC_skc_to_udp6_sock:
 		return &bpf_skc_to_udp6_sock_proto;
+	case BPF_FUNC_sk_storage_get:
+		return &bpf_sk_storage_get_tracing_proto;
+	case BPF_FUNC_sk_storage_delete:
+		return &bpf_sk_storage_delete_tracing_proto;
 #endif
 	case BPF_FUNC_seq_printf:
 		return prog->expected_attach_type == BPF_TRACE_ITER ?
diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index fd416678f236..359908a7d3c1 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -6,6 +6,7 @@ 
 #include <linux/types.h>
 #include <linux/spinlock.h>
 #include <linux/bpf.h>
+#include <linux/btf.h>
 #include <linux/btf_ids.h>
 #include <linux/bpf_local_storage.h>
 #include <net/bpf_sk_storage.h>
@@ -378,6 +379,79 @@  const struct bpf_func_proto bpf_sk_storage_delete_proto = {
 	.arg2_type	= ARG_PTR_TO_BTF_ID_SOCK_COMMON,
 };
 
+static bool bpf_sk_storage_tracing_allowed(const struct bpf_prog *prog)
+{
+	const struct btf *btf_vmlinux;
+	const struct btf_type *t;
+	const char *tname;
+	u32 btf_id;
+
+	if (prog->aux->dst_prog)
+		return false;
+
+	/* Ensure the tracing program is not tracing
+	 * any bpf_sk_storage*() function and also
+	 * use the bpf_sk_storage_(get|delete) helper.
+	 */
+	switch (prog->expected_attach_type) {
+	case BPF_TRACE_RAW_TP:
+		/* bpf_sk_storage has no trace point */
+		return true;
+	case BPF_TRACE_FENTRY:
+	case BPF_TRACE_FEXIT:
+		btf_vmlinux = bpf_get_btf_vmlinux();
+		btf_id = prog->aux->attach_btf_id;
+		t = btf_type_by_id(btf_vmlinux, btf_id);
+		tname = btf_name_by_offset(btf_vmlinux, t->name_off);
+		return !!strncmp(tname, "bpf_sk_storage",
+				 strlen("bpf_sk_storage"));
+	default:
+		return false;
+	}
+
+	return false;
+}
+
+BPF_CALL_4(bpf_sk_storage_get_tracing, struct bpf_map *, map, struct sock *, sk,
+	   void *, value, u64, flags)
+{
+	if (!in_serving_softirq() && !in_task())
+		return (unsigned long)NULL;
+
+	return (unsigned long)____bpf_sk_storage_get(map, sk, value, flags);
+}
+
+BPF_CALL_2(bpf_sk_storage_delete_tracing, struct bpf_map *, map,
+	   struct sock *, sk)
+{
+	if (!in_serving_softirq() && !in_task())
+		return -EPERM;
+
+	return ____bpf_sk_storage_delete(map, sk);
+}
+
+const struct bpf_func_proto bpf_sk_storage_get_tracing_proto = {
+	.func		= bpf_sk_storage_get_tracing,
+	.gpl_only	= false,
+	.ret_type	= RET_PTR_TO_MAP_VALUE_OR_NULL,
+	.arg1_type	= ARG_CONST_MAP_PTR,
+	.arg2_type	= ARG_PTR_TO_BTF_ID,
+	.arg2_btf_id	= &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
+	.arg3_type	= ARG_PTR_TO_MAP_VALUE_OR_NULL,
+	.arg4_type	= ARG_ANYTHING,
+	.allowed	= bpf_sk_storage_tracing_allowed,
+};
+
+const struct bpf_func_proto bpf_sk_storage_delete_tracing_proto = {
+	.func		= bpf_sk_storage_delete_tracing,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_CONST_MAP_PTR,
+	.arg2_type	= ARG_PTR_TO_BTF_ID,
+	.arg2_btf_id	= &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
+	.allowed	= bpf_sk_storage_tracing_allowed,
+};
+
 struct bpf_sk_storage_diag {
 	u32 nr_maps;
 	struct bpf_map *maps[];