[PATCH bpf-next 0/3] writable contexts for bpf raw tracepoints

[PATCH bpf-next 0/3] writable contexts for bpf raw tracepoints

[Matt Mullins]

This adds an opt-in interface for tracepoints to expose a writable context to
BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE programs that are attached, while
supporting read-only access from existing BPF_PROG_TYPE_RAW_TRACEPOINT
programs, as well as from non-BPF-based tracepoints.

The initial motivation is to support tracing that can be observed from the
remote end of an NBD socket, e.g. by adding flags to the struct nbd_request
header.  Earlier attempts included adding an NBD-specific tracepoint fd, but in
code review, I was recommended to implement it more generically -- as a result,
this patchset is far simpler than my initial try.

Andrew Hall (1):
  nbd: add tracepoints for send/receive timing

Matt Mullins (2):
  bpf: add writable context for raw tracepoints
  nbd: trace sending nbd requests

 MAINTAINERS                     |   1 +
 drivers/block/nbd.c             |  13 +++
 include/linux/bpf.h             |   2 +
 include/linux/bpf_types.h       |   1 +
 include/linux/tracepoint-defs.h |   1 +
 include/trace/bpf_probe.h       |  27 +++++-
 include/trace/events/nbd.h      | 148 ++++++++++++++++++++++++++++++++
 include/uapi/linux/bpf.h        |   1 +
 kernel/bpf/syscall.c            |   8 +-
 kernel/bpf/verifier.c           |  11 +++
 kernel/trace/bpf_trace.c        |  21 +++++
 11 files changed, 230 insertions(+), 4 deletions(-)
 create mode 100644 include/trace/events/nbd.h

-- 
2.17.1

[PATCH bpf-next 1/3] bpf: add writable context for raw tracepoints

[Matt Mullins]

This is an opt-in interface that allows a tracepoint to provide a safe
buffer that can be written from a BPF_PROG_TYPE_RAW_TRACEPOINT program.
The size of the buffer must be a compile-time constant, and is checked
before allowing a BPF program to attach to a tracepoint that uses this
feature.

The pointer to this buffer will be the first argument of tracepoints
that opt in; the buffer is readable by both BPF_PROG_TYPE_RAW_TRACEPOINT
and BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE programs that attach to such a
tracepoint, but the buffer to which it points may only be written by the
latter.

bpf_probe: assert that writable tracepoint size is correct

Signed-off-by: Matt Mullins <mmullins@fb.com>
---
 include/linux/bpf.h             |  2 ++
 include/linux/bpf_types.h       |  1 +
 include/linux/tracepoint-defs.h |  1 +
 include/trace/bpf_probe.h       | 27 +++++++++++++++++++++++++--
 include/uapi/linux/bpf.h        |  1 +
 kernel/bpf/syscall.c            |  8 ++++++--
 kernel/bpf/verifier.c           | 11 +++++++++++
 kernel/trace/bpf_trace.c        | 21 +++++++++++++++++++++
 8 files changed, 68 insertions(+), 4 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a2132e09dc1c..d3c71fd67476 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -263,6 +263,7 @@ enum bpf_reg_type {
 	PTR_TO_SOCK_COMMON_OR_NULL, /* reg points to sock_common or NULL */
 	PTR_TO_TCP_SOCK,	 /* reg points to struct tcp_sock */
 	PTR_TO_TCP_SOCK_OR_NULL, /* reg points to struct tcp_sock or NULL */
+	PTR_TO_TP_BUFFER,	 /* reg points to a writable raw tp's buffer */
 };
 
 /* The information passed from prog-specific *_is_valid_access
@@ -352,6 +353,7 @@ struct bpf_prog_aux {
 	u32 used_map_cnt;
 	u32 max_ctx_offset;
 	u32 max_pkt_offset;
+	u32 max_tp_access;
 	u32 stack_depth;
 	u32 id;
 	u32 func_cnt; /* used by non-func prog as the number of func progs */
diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
index 08bf2f1fe553..c766108608cb 100644
--- a/include/linux/bpf_types.h
+++ b/include/linux/bpf_types.h
@@ -25,6 +25,7 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_KPROBE, kprobe)
 BPF_PROG_TYPE(BPF_PROG_TYPE_TRACEPOINT, tracepoint)
 BPF_PROG_TYPE(BPF_PROG_TYPE_PERF_EVENT, perf_event)
 BPF_PROG_TYPE(BPF_PROG_TYPE_RAW_TRACEPOINT, raw_tracepoint)
+BPF_PROG_TYPE(BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE, raw_tracepoint_writable)
 #endif
 #ifdef CONFIG_CGROUP_BPF
 BPF_PROG_TYPE(BPF_PROG_TYPE_CGROUP_DEVICE, cg_dev)
diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
index 49ba9cde7e4b..b29950a19205 100644
--- a/include/linux/tracepoint-defs.h
+++ b/include/linux/tracepoint-defs.h
@@ -45,6 +45,7 @@ struct bpf_raw_event_map {
 	struct tracepoint	*tp;
 	void			*bpf_func;
 	u32			num_args;
+	u32			writable_size;
 } __aligned(32);
 
 #endif
diff --git a/include/trace/bpf_probe.h b/include/trace/bpf_probe.h
index 505dae0bed80..d6e556c0a085 100644
--- a/include/trace/bpf_probe.h
+++ b/include/trace/bpf_probe.h
@@ -69,8 +69,7 @@ __bpf_trace_##call(void *__data, proto)					\
  * to make sure that if the tracepoint handling changes, the
  * bpf probe will fail to compile unless it too is updated.
  */
-#undef DEFINE_EVENT
-#define DEFINE_EVENT(template, call, proto, args)			\
+#define __DEFINE_EVENT(template, call, proto, args, size)		\
 static inline void bpf_test_probe_##call(void)				\
 {									\
 	check_trace_callback_type_##call(__bpf_trace_##template);	\
@@ -81,12 +80,36 @@ __bpf_trace_tp_map_##call = {						\
 	.tp		= &__tracepoint_##call,				\
 	.bpf_func	= (void *)__bpf_trace_##template,		\
 	.num_args	= COUNT_ARGS(args),				\
+	.writable_size	= size,						\
 };
 
+#define FIRST(x, ...) x
+
+#undef DEFINE_EVENT_WRITABLE
+#define DEFINE_EVENT_WRITABLE(template, call, proto, args, size)	\
+static inline void bpf_test_buffer_##call(void)				\
+{									\
+	/* BUILD_BUG_ON() is ignored if the code is completely eliminated, but \
+	 * BUILD_BUG_ON_ZERO() uses a different mechanism that is not	\
+	 * dead-code-eliminated.					\
+	 */								\
+	FIRST(proto);							\
+	(void)BUILD_BUG_ON_ZERO(size != sizeof(*FIRST(args)));		\
+}									\
+__DEFINE_EVENT(template, call, PARAMS(proto), PARAMS(args), size)
+
+#undef DEFINE_EVENT
+#define DEFINE_EVENT(template, call, proto, args)			\
+	__DEFINE_EVENT(template, call, PARAMS(proto), PARAMS(args), 0)
 
 #undef DEFINE_EVENT_PRINT
 #define DEFINE_EVENT_PRINT(template, name, proto, args, print)	\
 	DEFINE_EVENT(template, name, PARAMS(proto), PARAMS(args))
 
 #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
+
+#undef DEFINE_EVENT_WRITABLE
+#undef __DEFINE_EVENT
+#undef FIRST
+
 #endif /* CONFIG_BPF_EVENTS */
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 3c38ac9a92a7..c5335d53ce82 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -166,6 +166,7 @@ enum bpf_prog_type {
 	BPF_PROG_TYPE_LIRC_MODE2,
 	BPF_PROG_TYPE_SK_REUSEPORT,
 	BPF_PROG_TYPE_FLOW_DISSECTOR,
+	BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE,
 };
 
 enum bpf_attach_type {
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 62f6bced3a3c..27e2f22879a4 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1720,12 +1720,16 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
 	}
 	raw_tp->btp = btp;
 
-	prog = bpf_prog_get_type(attr->raw_tracepoint.prog_fd,
-				 BPF_PROG_TYPE_RAW_TRACEPOINT);
+	prog = bpf_prog_get(attr->raw_tracepoint.prog_fd);
 	if (IS_ERR(prog)) {
 		err = PTR_ERR(prog);
 		goto out_free_tp;
 	}
+	if (prog->type != BPF_PROG_TYPE_RAW_TRACEPOINT &&
+	    prog->type != BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE) {
+		err = -EINVAL;
+		goto out_put_prog;
+	}
 
 	err = bpf_probe_register(raw_tp->btp, prog);
 	if (err)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ce166a002d16..b6b4a2ca9f0c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2100,6 +2100,17 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
 		err = check_sock_access(env, insn_idx, regno, off, size, t);
 		if (!err && value_regno >= 0)
 			mark_reg_unknown(env, regs, value_regno);
+	} else if (reg->type == PTR_TO_TP_BUFFER) {
+		if (off < 0) {
+			verbose(env,
+				"R%d invalid tracepoint buffer access: off=%d, size=%d",
+				value_regno, off, size);
+			return -EACCES;
+		}
+		if (off + size > env->prog->aux->max_tp_access)
+			env->prog->aux->max_tp_access = off + size;
+		if (t == BPF_READ && value_regno >= 0)
+			mark_reg_unknown(env, regs, value_regno);
 	} else {
 		verbose(env, "R%d invalid mem access '%s'\n", regno,
 			reg_type_str[reg->type]);
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index d64c00afceb5..a2dd79dc6871 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -909,6 +909,24 @@ const struct bpf_verifier_ops raw_tracepoint_verifier_ops = {
 const struct bpf_prog_ops raw_tracepoint_prog_ops = {
 };
 
+static bool raw_tp_writable_prog_is_valid_access(int off, int size,
+						 enum bpf_access_type type,
+						 const struct bpf_prog *prog,
+						 struct bpf_insn_access_aux *info)
+{
+	if (off == 0 && size == sizeof(u64))
+		info->reg_type = PTR_TO_TP_BUFFER;
+	return raw_tp_prog_is_valid_access(off, size, type, prog, info);
+}
+
+const struct bpf_verifier_ops raw_tracepoint_writable_verifier_ops = {
+	.get_func_proto  = raw_tp_prog_func_proto,
+	.is_valid_access = raw_tp_writable_prog_is_valid_access,
+};
+
+const struct bpf_prog_ops raw_tracepoint_writable_prog_ops = {
+};
+
 static bool pe_prog_is_valid_access(int off, int size, enum bpf_access_type type,
 				    const struct bpf_prog *prog,
 				    struct bpf_insn_access_aux *info)
@@ -1198,6 +1216,9 @@ static int __bpf_probe_register(struct bpf_raw_event_map *btp, struct bpf_prog *
 	if (prog->aux->max_ctx_offset > btp->num_args * sizeof(u64))
 		return -EINVAL;
 
+	if (prog->aux->max_tp_access > btp->writable_size)
+		return -EINVAL;
+
 	return tracepoint_probe_register(tp, (void *)btp->bpf_func, prog);
 }
 
-- 
2.17.1

[PATCH bpf-next 3/3] nbd: add tracepoints for send/receive timing

[Matt Mullins]

From: Andrew Hall <hall@fb.com>

This adds four tracepoints to nbd, enabling separate tracing of payload
and header sending/receipt.

In the send path for headers that have already been sent, we also
explicitly initialize the handle so it can be referenced by the later
tracepoint.

Signed-off-by: Andrew Hall <hall@fb.com>
Signed-off-by: Matt Mullins <mmullins@fb.com>
---
 drivers/block/nbd.c        |  8 ++++
 include/trace/events/nbd.h | 92 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 100 insertions(+)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 7393d04d255c..d3d914620f66 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -513,6 +513,10 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
 	if (sent) {
 		if (sent >= sizeof(request)) {
 			skip = sent - sizeof(request);
+
+			// initialize handle for tracing purposes
+			handle = nbd_cmd_handle(cmd);
+
 			goto send_pages;
 		}
 		iov_iter_advance(&from, sent);
@@ -536,6 +540,7 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
 		(unsigned long long)blk_rq_pos(req) << 9, blk_rq_bytes(req));
 	result = sock_xmit(nbd, index, 1, &from,
 			(type == NBD_CMD_WRITE) ? MSG_MORE : 0, &sent);
+	trace_nbd_header_sent(req, handle);
 	if (result <= 0) {
 		if (was_interrupted(result)) {
 			/* If we havne't sent anything we can just return BUSY,
@@ -608,6 +613,7 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
 		bio = next;
 	}
 out:
+	trace_nbd_payload_sent(req, handle);
 	nsock->pending = NULL;
 	nsock->sent = 0;
 	return 0;
@@ -655,6 +661,7 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index)
 			tag, req);
 		return ERR_PTR(-ENOENT);
 	}
+	trace_nbd_header_received(req, handle);
 	cmd = blk_mq_rq_to_pdu(req);
 
 	mutex_lock(&cmd->lock);
@@ -708,6 +715,7 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index)
 		}
 	}
 out:
+	trace_nbd_payload_received(req, handle);
 	mutex_unlock(&cmd->lock);
 	return ret ? ERR_PTR(ret) : cmd;
 }
diff --git a/include/trace/events/nbd.h b/include/trace/events/nbd.h
index 5928255ed02e..eef476fef95a 100644
--- a/include/trace/events/nbd.h
+++ b/include/trace/events/nbd.h
@@ -7,6 +7,98 @@
 
 #include <linux/tracepoint.h>
 
+TRACE_EVENT(nbd_header_sent,
+
+	TP_PROTO(struct request *req, u64 handle),
+
+	TP_ARGS(req, handle),
+
+	TP_STRUCT__entry(
+		__field(struct request *, req)
+		__field(u64, handle)
+	),
+
+	TP_fast_assign(
+		__entry->req = req;
+		__entry->handle = handle;
+	),
+
+	TP_printk(
+		"nbd header sent: request %p, handle 0x%016llx",
+		__entry->req,
+		__entry->handle
+	)
+);
+
+TRACE_EVENT(nbd_payload_sent,
+
+	TP_PROTO(struct request *req, u64 handle),
+
+	TP_ARGS(req, handle),
+
+	TP_STRUCT__entry(
+		__field(struct request *, req)
+		__field(u64, handle)
+	),
+
+	TP_fast_assign(
+		__entry->req = req;
+		__entry->handle = handle;
+	),
+
+	TP_printk(
+		"nbd payload sent: request %p, handle 0x%016llx",
+		__entry->req,
+		__entry->handle
+	)
+);
+
+TRACE_EVENT(nbd_header_received,
+
+	TP_PROTO(struct request *req, u64 handle),
+
+	TP_ARGS(req, handle),
+
+	TP_STRUCT__entry(
+		__field(struct request *, req)
+		__field(u64, handle)
+	),
+
+	TP_fast_assign(
+		__entry->req = req;
+		__entry->handle = handle;
+	),
+
+	TP_printk(
+		"nbd header received: request %p, handle 0x%016llx",
+		__entry->req,
+		__entry->handle
+	)
+);
+
+TRACE_EVENT(nbd_payload_received,
+
+	TP_PROTO(struct request *req, u64 handle),
+
+	TP_ARGS(req, handle),
+
+	TP_STRUCT__entry(
+		__field(struct request *, req)
+		__field(u64, handle)
+	),
+
+	TP_fast_assign(
+		__entry->req = req;
+		__entry->handle = handle;
+	),
+
+	TP_printk(
+		"nbd payload received: request %p, handle 0x%016llx",
+		__entry->req,
+		__entry->handle
+	)
+);
+
 DECLARE_EVENT_CLASS(nbd_send_request,
 
 	TP_PROTO(struct nbd_request *nbd_request, int index,
-- 
2.17.1

Re: [PATCH bpf-next 1/3] bpf: add writable context for raw tracepoints

[Daniel Borkmann]

On 03/29/2019 01:07 AM, Matt Mullins wrote:
> This is an opt-in interface that allows a tracepoint to provide a safe
> buffer that can be written from a BPF_PROG_TYPE_RAW_TRACEPOINT program.
> The size of the buffer must be a compile-time constant, and is checked
> before allowing a BPF program to attach to a tracepoint that uses this
> feature.
> 
> The pointer to this buffer will be the first argument of tracepoints
> that opt in; the buffer is readable by both BPF_PROG_TYPE_RAW_TRACEPOINT
> and BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE programs that attach to such a
> tracepoint, but the buffer to which it points may only be written by the
> latter.
> 
> bpf_probe: assert that writable tracepoint size is correct

Maybe also add a kselftest into bpf test suite to i) demo it and ii) make
sure it's continuously been tested by bots running the suite?

> Signed-off-by: Matt Mullins <mmullins@fb.com>
> ---
>  include/linux/bpf.h             |  2 ++
>  include/linux/bpf_types.h       |  1 +
>  include/linux/tracepoint-defs.h |  1 +
>  include/trace/bpf_probe.h       | 27 +++++++++++++++++++++++++--
>  include/uapi/linux/bpf.h        |  1 +
>  kernel/bpf/syscall.c            |  8 ++++++--
>  kernel/bpf/verifier.c           | 11 +++++++++++
>  kernel/trace/bpf_trace.c        | 21 +++++++++++++++++++++
>  8 files changed, 68 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index a2132e09dc1c..d3c71fd67476 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -263,6 +263,7 @@ enum bpf_reg_type {
>  	PTR_TO_SOCK_COMMON_OR_NULL, /* reg points to sock_common or NULL */
>  	PTR_TO_TCP_SOCK,	 /* reg points to struct tcp_sock */
>  	PTR_TO_TCP_SOCK_OR_NULL, /* reg points to struct tcp_sock or NULL */
> +	PTR_TO_TP_BUFFER,	 /* reg points to a writable raw tp's buffer */
>  };
>  
>  /* The information passed from prog-specific *_is_valid_access
> @@ -352,6 +353,7 @@ struct bpf_prog_aux {
>  	u32 used_map_cnt;
>  	u32 max_ctx_offset;
>  	u32 max_pkt_offset;
> +	u32 max_tp_access;
>  	u32 stack_depth;
>  	u32 id;
>  	u32 func_cnt; /* used by non-func prog as the number of func progs */
> diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> index 08bf2f1fe553..c766108608cb 100644
> --- a/include/linux/bpf_types.h
> +++ b/include/linux/bpf_types.h
> @@ -25,6 +25,7 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_KPROBE, kprobe)
>  BPF_PROG_TYPE(BPF_PROG_TYPE_TRACEPOINT, tracepoint)
>  BPF_PROG_TYPE(BPF_PROG_TYPE_PERF_EVENT, perf_event)
>  BPF_PROG_TYPE(BPF_PROG_TYPE_RAW_TRACEPOINT, raw_tracepoint)
> +BPF_PROG_TYPE(BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE, raw_tracepoint_writable)
>  #endif
>  #ifdef CONFIG_CGROUP_BPF
>  BPF_PROG_TYPE(BPF_PROG_TYPE_CGROUP_DEVICE, cg_dev)
> diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
> index 49ba9cde7e4b..b29950a19205 100644
> --- a/include/linux/tracepoint-defs.h
> +++ b/include/linux/tracepoint-defs.h
> @@ -45,6 +45,7 @@ struct bpf_raw_event_map {
>  	struct tracepoint	*tp;
>  	void			*bpf_func;
>  	u32			num_args;
> +	u32			writable_size;
>  } __aligned(32);
>  
>  #endif
> diff --git a/include/trace/bpf_probe.h b/include/trace/bpf_probe.h
> index 505dae0bed80..d6e556c0a085 100644
> --- a/include/trace/bpf_probe.h
> +++ b/include/trace/bpf_probe.h
> @@ -69,8 +69,7 @@ __bpf_trace_##call(void *__data, proto)					\
>   * to make sure that if the tracepoint handling changes, the
>   * bpf probe will fail to compile unless it too is updated.
>   */
> -#undef DEFINE_EVENT
> -#define DEFINE_EVENT(template, call, proto, args)			\
> +#define __DEFINE_EVENT(template, call, proto, args, size)		\
>  static inline void bpf_test_probe_##call(void)				\
>  {									\
>  	check_trace_callback_type_##call(__bpf_trace_##template);	\
> @@ -81,12 +80,36 @@ __bpf_trace_tp_map_##call = {						\
>  	.tp		= &__tracepoint_##call,				\
>  	.bpf_func	= (void *)__bpf_trace_##template,		\
>  	.num_args	= COUNT_ARGS(args),				\
> +	.writable_size	= size,						\
>  };
>  
> +#define FIRST(x, ...) x
> +
> +#undef DEFINE_EVENT_WRITABLE
> +#define DEFINE_EVENT_WRITABLE(template, call, proto, args, size)	\
> +static inline void bpf_test_buffer_##call(void)				\
> +{									\
> +	/* BUILD_BUG_ON() is ignored if the code is completely eliminated, but \
> +	 * BUILD_BUG_ON_ZERO() uses a different mechanism that is not	\
> +	 * dead-code-eliminated.					\
> +	 */								\
> +	FIRST(proto);							\
> +	(void)BUILD_BUG_ON_ZERO(size != sizeof(*FIRST(args)));		\
> +}									\
> +__DEFINE_EVENT(template, call, PARAMS(proto), PARAMS(args), size)
> +
> +#undef DEFINE_EVENT
> +#define DEFINE_EVENT(template, call, proto, args)			\
> +	__DEFINE_EVENT(template, call, PARAMS(proto), PARAMS(args), 0)
>  
>  #undef DEFINE_EVENT_PRINT
>  #define DEFINE_EVENT_PRINT(template, name, proto, args, print)	\
>  	DEFINE_EVENT(template, name, PARAMS(proto), PARAMS(args))
>  
>  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> +
> +#undef DEFINE_EVENT_WRITABLE
> +#undef __DEFINE_EVENT
> +#undef FIRST
> +
>  #endif /* CONFIG_BPF_EVENTS */
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 3c38ac9a92a7..c5335d53ce82 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -166,6 +166,7 @@ enum bpf_prog_type {
>  	BPF_PROG_TYPE_LIRC_MODE2,
>  	BPF_PROG_TYPE_SK_REUSEPORT,
>  	BPF_PROG_TYPE_FLOW_DISSECTOR,
> +	BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE,
>  };
>  
>  enum bpf_attach_type {
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 62f6bced3a3c..27e2f22879a4 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1720,12 +1720,16 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
>  	}
>  	raw_tp->btp = btp;
>  
> -	prog = bpf_prog_get_type(attr->raw_tracepoint.prog_fd,
> -				 BPF_PROG_TYPE_RAW_TRACEPOINT);
> +	prog = bpf_prog_get(attr->raw_tracepoint.prog_fd);
>  	if (IS_ERR(prog)) {
>  		err = PTR_ERR(prog);
>  		goto out_free_tp;
>  	}
> +	if (prog->type != BPF_PROG_TYPE_RAW_TRACEPOINT &&
> +	    prog->type != BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE) {

I don't think we'd gain a lot by making this an extra prog type which can do the
same as BPF_PROG_TYPE_RAW_TRACEPOINT modulo optional writing. Why not integrating
this directly into BPF_PROG_TYPE_RAW_TRACEPOINT then? The actual opt-in comes from
the DEFINE_EVENT_WRITABLE(), not from the prog type.

> +		err = -EINVAL;
> +		goto out_put_prog;
> +	}
>  
>  	err = bpf_probe_register(raw_tp->btp, prog);
>  	if (err)
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ce166a002d16..b6b4a2ca9f0c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2100,6 +2100,17 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
>  		err = check_sock_access(env, insn_idx, regno, off, size, t);
>  		if (!err && value_regno >= 0)
>  			mark_reg_unknown(env, regs, value_regno);
> +	} else if (reg->type == PTR_TO_TP_BUFFER) {
> +		if (off < 0) {
> +			verbose(env,
> +				"R%d invalid tracepoint buffer access: off=%d, size=%d",
> +				value_regno, off, size);
> +			return -EACCES;
> +		}
> +		if (off + size > env->prog->aux->max_tp_access)
> +			env->prog->aux->max_tp_access = off + size;
> +		if (t == BPF_READ && value_regno >= 0)
> +			mark_reg_unknown(env, regs, value_regno);

This should also disallow variable access into the reg, I presume (see check_ctx_reg())?
Or is there a clear rationale for having it enabled?

>  	} else {
>  		verbose(env, "R%d invalid mem access '%s'\n", regno,
>  			reg_type_str[reg->type]);
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index d64c00afceb5..a2dd79dc6871 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -909,6 +909,24 @@ const struct bpf_verifier_ops raw_tracepoint_verifier_ops = {
>  const struct bpf_prog_ops raw_tracepoint_prog_ops = {
>  };
>  
> +static bool raw_tp_writable_prog_is_valid_access(int off, int size,
> +						 enum bpf_access_type type,
> +						 const struct bpf_prog *prog,
> +						 struct bpf_insn_access_aux *info)
> +{
> +	if (off == 0 && size == sizeof(u64))
> +		info->reg_type = PTR_TO_TP_BUFFER;
> +	return raw_tp_prog_is_valid_access(off, size, type, prog, info);
> +}
> +
> +const struct bpf_verifier_ops raw_tracepoint_writable_verifier_ops = {
> +	.get_func_proto  = raw_tp_prog_func_proto,
> +	.is_valid_access = raw_tp_writable_prog_is_valid_access,
> +};
> +
> +const struct bpf_prog_ops raw_tracepoint_writable_prog_ops = {
> +};
> +
>  static bool pe_prog_is_valid_access(int off, int size, enum bpf_access_type type,
>  				    const struct bpf_prog *prog,
>  				    struct bpf_insn_access_aux *info)
> @@ -1198,6 +1216,9 @@ static int __bpf_probe_register(struct bpf_raw_event_map *btp, struct bpf_prog *
>  	if (prog->aux->max_ctx_offset > btp->num_args * sizeof(u64))
>  		return -EINVAL;
>  
> +	if (prog->aux->max_tp_access > btp->writable_size)
> +		return -EINVAL;
> +
>  	return tracepoint_probe_register(tp, (void *)btp->bpf_func, prog);
>  }
>  
> 

Re: [PATCH bpf-next 1/3] bpf: add writable context for raw tracepoints

[Matt Mullins]

On Mon, 2019-04-01 at 22:40 +0200, Daniel Borkmann wrote:
> On 03/29/2019 01:07 AM, Matt Mullins wrote:
> > This is an opt-in interface that allows a tracepoint to provide a safe
> > buffer that can be written from a BPF_PROG_TYPE_RAW_TRACEPOINT program.
> > The size of the buffer must be a compile-time constant, and is checked
> > before allowing a BPF program to attach to a tracepoint that uses this
> > feature.
> > 
> > The pointer to this buffer will be the first argument of tracepoints
> > that opt in; the buffer is readable by both BPF_PROG_TYPE_RAW_TRACEPOINT
> > and BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE programs that attach to such a
> > tracepoint, but the buffer to which it points may only be written by the
> > latter.
> > 
> > bpf_probe: assert that writable tracepoint size is correct
> 
> Maybe also add a kselftest into bpf test suite to i) demo it and ii) make
> sure it's continuously been tested by bots running the suite?

Will do.

> 
> > Signed-off-by: Matt Mullins <mmullins@fb.com>
> > ---
> >  include/linux/bpf.h             |  2 ++
> >  include/linux/bpf_types.h       |  1 +
> >  include/linux/tracepoint-defs.h |  1 +
> >  include/trace/bpf_probe.h       | 27 +++++++++++++++++++++++++--
> >  include/uapi/linux/bpf.h        |  1 +
> >  kernel/bpf/syscall.c            |  8 ++++++--
> >  kernel/bpf/verifier.c           | 11 +++++++++++
> >  kernel/trace/bpf_trace.c        | 21 +++++++++++++++++++++
> >  8 files changed, 68 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index a2132e09dc1c..d3c71fd67476 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -263,6 +263,7 @@ enum bpf_reg_type {
> >  	PTR_TO_SOCK_COMMON_OR_NULL, /* reg points to sock_common or NULL */
> >  	PTR_TO_TCP_SOCK,	 /* reg points to struct tcp_sock */
> >  	PTR_TO_TCP_SOCK_OR_NULL, /* reg points to struct tcp_sock or NULL */
> > +	PTR_TO_TP_BUFFER,	 /* reg points to a writable raw tp's buffer */
> >  };
> >  
> >  /* The information passed from prog-specific *_is_valid_access
> > @@ -352,6 +353,7 @@ struct bpf_prog_aux {
> >  	u32 used_map_cnt;
> >  	u32 max_ctx_offset;
> >  	u32 max_pkt_offset;
> > +	u32 max_tp_access;
> >  	u32 stack_depth;
> >  	u32 id;
> >  	u32 func_cnt; /* used by non-func prog as the number of func progs */
> > diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> > index 08bf2f1fe553..c766108608cb 100644
> > --- a/include/linux/bpf_types.h
> > +++ b/include/linux/bpf_types.h
> > @@ -25,6 +25,7 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_KPROBE, kprobe)
> >  BPF_PROG_TYPE(BPF_PROG_TYPE_TRACEPOINT, tracepoint)
> >  BPF_PROG_TYPE(BPF_PROG_TYPE_PERF_EVENT, perf_event)
> >  BPF_PROG_TYPE(BPF_PROG_TYPE_RAW_TRACEPOINT, raw_tracepoint)
> > +BPF_PROG_TYPE(BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE, raw_tracepoint_writable)
> >  #endif
> >  #ifdef CONFIG_CGROUP_BPF
> >  BPF_PROG_TYPE(BPF_PROG_TYPE_CGROUP_DEVICE, cg_dev)
> > diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
> > index 49ba9cde7e4b..b29950a19205 100644
> > --- a/include/linux/tracepoint-defs.h
> > +++ b/include/linux/tracepoint-defs.h
> > @@ -45,6 +45,7 @@ struct bpf_raw_event_map {
> >  	struct tracepoint	*tp;
> >  	void			*bpf_func;
> >  	u32			num_args;
> > +	u32			writable_size;
> >  } __aligned(32);
> >  
> >  #endif
> > diff --git a/include/trace/bpf_probe.h b/include/trace/bpf_probe.h
> > index 505dae0bed80..d6e556c0a085 100644
> > --- a/include/trace/bpf_probe.h
> > +++ b/include/trace/bpf_probe.h
> > @@ -69,8 +69,7 @@ __bpf_trace_##call(void *__data, proto)					\
> >   * to make sure that if the tracepoint handling changes, the
> >   * bpf probe will fail to compile unless it too is updated.
> >   */
> > -#undef DEFINE_EVENT
> > -#define DEFINE_EVENT(template, call, proto, args)			\
> > +#define __DEFINE_EVENT(template, call, proto, args, size)		\
> >  static inline void bpf_test_probe_##call(void)				\
> >  {									\
> >  	check_trace_callback_type_##call(__bpf_trace_##template);	\
> > @@ -81,12 +80,36 @@ __bpf_trace_tp_map_##call = {						\
> >  	.tp		= &__tracepoint_##call,				\
> >  	.bpf_func	= (void *)__bpf_trace_##template,		\
> >  	.num_args	= COUNT_ARGS(args),				\
> > +	.writable_size	= size,						\
> >  };
> >  
> > +#define FIRST(x, ...) x
> > +
> > +#undef DEFINE_EVENT_WRITABLE
> > +#define DEFINE_EVENT_WRITABLE(template, call, proto, args, size)	\
> > +static inline void bpf_test_buffer_##call(void)				\
> > +{									\
> > +	/* BUILD_BUG_ON() is ignored if the code is completely eliminated, but \
> > +	 * BUILD_BUG_ON_ZERO() uses a different mechanism that is not	\
> > +	 * dead-code-eliminated.					\
> > +	 */								\
> > +	FIRST(proto);							\
> > +	(void)BUILD_BUG_ON_ZERO(size != sizeof(*FIRST(args)));		\
> > +}									\
> > +__DEFINE_EVENT(template, call, PARAMS(proto), PARAMS(args), size)
> > +
> > +#undef DEFINE_EVENT
> > +#define DEFINE_EVENT(template, call, proto, args)			\
> > +	__DEFINE_EVENT(template, call, PARAMS(proto), PARAMS(args), 0)
> >  
> >  #undef DEFINE_EVENT_PRINT
> >  #define DEFINE_EVENT_PRINT(template, name, proto, args, print)	\
> >  	DEFINE_EVENT(template, name, PARAMS(proto), PARAMS(args))
> >  
> >  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> > +
> > +#undef DEFINE_EVENT_WRITABLE
> > +#undef __DEFINE_EVENT
> > +#undef FIRST
> > +
> >  #endif /* CONFIG_BPF_EVENTS */
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 3c38ac9a92a7..c5335d53ce82 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -166,6 +166,7 @@ enum bpf_prog_type {
> >  	BPF_PROG_TYPE_LIRC_MODE2,
> >  	BPF_PROG_TYPE_SK_REUSEPORT,
> >  	BPF_PROG_TYPE_FLOW_DISSECTOR,
> > +	BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE,
> >  };
> >  
> >  enum bpf_attach_type {
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 62f6bced3a3c..27e2f22879a4 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -1720,12 +1720,16 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
> >  	}
> >  	raw_tp->btp = btp;
> >  
> > -	prog = bpf_prog_get_type(attr->raw_tracepoint.prog_fd,
> > -				 BPF_PROG_TYPE_RAW_TRACEPOINT);
> > +	prog = bpf_prog_get(attr->raw_tracepoint.prog_fd);
> >  	if (IS_ERR(prog)) {
> >  		err = PTR_ERR(prog);
> >  		goto out_free_tp;
> >  	}
> > +	if (prog->type != BPF_PROG_TYPE_RAW_TRACEPOINT &&
> > +	    prog->type != BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE) {
> 
> I don't think we'd gain a lot by making this an extra prog type which can do the
> same as BPF_PROG_TYPE_RAW_TRACEPOINT modulo optional writing. Why not integrating
> this directly into BPF_PROG_TYPE_RAW_TRACEPOINT then? The actual opt-in comes from
> the DEFINE_EVENT_WRITABLE(), not from the prog type.

I did that to separate the hook into
raw_tp_writable_prog_is_valid_access, which (compared to
raw_tp_prog_is_valid_access):

  1) permits writes, and
  2) encodes the assumption than the context begins with the pointer to
that writable buffer

I'm not sure those are appropriate for all users of
BPF_PROG_TYPE_RAW_TRACEPOINT, but I can't immediately point out any
harm in doing so -- some dereferences of ctx that have historically
returned a SCALAR_VALUE would end up tagged as a PTR_TO_TP_BUFFER, but
they still won't be able to access through that pointer unless they're
attached to the right tracepoint.

I'll try to unify the two and see what I get.

> 
> > +		err = -EINVAL;
> > +		goto out_put_prog;
> > +	}
> >  
> >  	err = bpf_probe_register(raw_tp->btp, prog);
> >  	if (err)
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index ce166a002d16..b6b4a2ca9f0c 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -2100,6 +2100,17 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
> >  		err = check_sock_access(env, insn_idx, regno, off, size, t);
> >  		if (!err && value_regno >= 0)
> >  			mark_reg_unknown(env, regs, value_regno);
> > +	} else if (reg->type == PTR_TO_TP_BUFFER) {
> > +		if (off < 0) {
> > +			verbose(env,
> > +				"R%d invalid tracepoint buffer access: off=%d, size=%d",
> > +				value_regno, off, size);
> > +			return -EACCES;
> > +		}
> > +		if (off + size > env->prog->aux->max_tp_access)
> > +			env->prog->aux->max_tp_access = off + size;
> > +		if (t == BPF_READ && value_regno >= 0)
> > +			mark_reg_unknown(env, regs, value_regno);
> 
> This should also disallow variable access into the reg, I presume (see check_ctx_reg())?
> Or is there a clear rationale for having it enabled?

Nope, that was an oversight from an (incorrect) assumption that
arithmetic would be disallowed on a PTR_TO_TP_BUFFER.  I'll fix this in
v2.

> 
> >  	} else {
> >  		verbose(env, "R%d invalid mem access '%s'\n", regno,
> >  			reg_type_str[reg->type]);
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index d64c00afceb5..a2dd79dc6871 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -909,6 +909,24 @@ const struct bpf_verifier_ops raw_tracepoint_verifier_ops = {
> >  const struct bpf_prog_ops raw_tracepoint_prog_ops = {
> >  };
> >  
> > +static bool raw_tp_writable_prog_is_valid_access(int off, int size,
> > +						 enum bpf_access_type type,
> > +						 const struct bpf_prog *prog,
> > +						 struct bpf_insn_access_aux *info)
> > +{
> > +	if (off == 0 && size == sizeof(u64))
> > +		info->reg_type = PTR_TO_TP_BUFFER;
> > +	return raw_tp_prog_is_valid_access(off, size, type, prog, info);
> > +}
> > +
> > +const struct bpf_verifier_ops raw_tracepoint_writable_verifier_ops = {
> > +	.get_func_proto  = raw_tp_prog_func_proto,
> > +	.is_valid_access = raw_tp_writable_prog_is_valid_access,
> > +};
> > +
> > +const struct bpf_prog_ops raw_tracepoint_writable_prog_ops = {
> > +};
> > +
> >  static bool pe_prog_is_valid_access(int off, int size, enum bpf_access_type type,
> >  				    const struct bpf_prog *prog,
> >  				    struct bpf_insn_access_aux *info)
> > @@ -1198,6 +1216,9 @@ static int __bpf_probe_register(struct bpf_raw_event_map *btp, struct bpf_prog *
> >  	if (prog->aux->max_ctx_offset > btp->num_args * sizeof(u64))
> >  		return -EINVAL;
> >  
> > +	if (prog->aux->max_tp_access > btp->writable_size)
> > +		return -EINVAL;
> > +
> >  	return tracepoint_probe_register(tp, (void *)btp->bpf_func, prog);
> >  }
> >  
> > 
> 
> 

Re: [PATCH bpf-next 1/3] bpf: add writable context for raw tracepoints

[Alexei Starovoitov]

On Wed, Apr 03, 2019 at 06:39:12PM +0000, Matt Mullins wrote:
> On Mon, 2019-04-01 at 22:40 +0200, Daniel Borkmann wrote:
> > On 03/29/2019 01:07 AM, Matt Mullins wrote:
> > > This is an opt-in interface that allows a tracepoint to provide a safe
> > > buffer that can be written from a BPF_PROG_TYPE_RAW_TRACEPOINT program.
> > > The size of the buffer must be a compile-time constant, and is checked
> > > before allowing a BPF program to attach to a tracepoint that uses this
> > > feature.
> > > 
> > > The pointer to this buffer will be the first argument of tracepoints
> > > that opt in; the buffer is readable by both BPF_PROG_TYPE_RAW_TRACEPOINT
> > > and BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE programs that attach to such a
> > > tracepoint, but the buffer to which it points may only be written by the
> > > latter.
> > > 
> > > bpf_probe: assert that writable tracepoint size is correct
> > 
> > Maybe also add a kselftest into bpf test suite to i) demo it and ii) make
> > sure it's continuously been tested by bots running the suite?
> 
> Will do.
> 
> > 
> > > Signed-off-by: Matt Mullins <mmullins@fb.com>
> > > ---
> > >  include/linux/bpf.h             |  2 ++
> > >  include/linux/bpf_types.h       |  1 +
> > >  include/linux/tracepoint-defs.h |  1 +
> > >  include/trace/bpf_probe.h       | 27 +++++++++++++++++++++++++--
> > >  include/uapi/linux/bpf.h        |  1 +
> > >  kernel/bpf/syscall.c            |  8 ++++++--
> > >  kernel/bpf/verifier.c           | 11 +++++++++++
> > >  kernel/trace/bpf_trace.c        | 21 +++++++++++++++++++++
> > >  8 files changed, 68 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index a2132e09dc1c..d3c71fd67476 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -263,6 +263,7 @@ enum bpf_reg_type {
> > >  	PTR_TO_SOCK_COMMON_OR_NULL, /* reg points to sock_common or NULL */
> > >  	PTR_TO_TCP_SOCK,	 /* reg points to struct tcp_sock */
> > >  	PTR_TO_TCP_SOCK_OR_NULL, /* reg points to struct tcp_sock or NULL */
> > > +	PTR_TO_TP_BUFFER,	 /* reg points to a writable raw tp's buffer */
> > >  };
> > >  
> > >  /* The information passed from prog-specific *_is_valid_access
> > > @@ -352,6 +353,7 @@ struct bpf_prog_aux {
> > >  	u32 used_map_cnt;
> > >  	u32 max_ctx_offset;
> > >  	u32 max_pkt_offset;
> > > +	u32 max_tp_access;
> > >  	u32 stack_depth;
> > >  	u32 id;
> > >  	u32 func_cnt; /* used by non-func prog as the number of func progs */
> > > diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> > > index 08bf2f1fe553..c766108608cb 100644
> > > --- a/include/linux/bpf_types.h
> > > +++ b/include/linux/bpf_types.h
> > > @@ -25,6 +25,7 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_KPROBE, kprobe)
> > >  BPF_PROG_TYPE(BPF_PROG_TYPE_TRACEPOINT, tracepoint)
> > >  BPF_PROG_TYPE(BPF_PROG_TYPE_PERF_EVENT, perf_event)
> > >  BPF_PROG_TYPE(BPF_PROG_TYPE_RAW_TRACEPOINT, raw_tracepoint)
> > > +BPF_PROG_TYPE(BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE, raw_tracepoint_writable)
> > >  #endif
> > >  #ifdef CONFIG_CGROUP_BPF
> > >  BPF_PROG_TYPE(BPF_PROG_TYPE_CGROUP_DEVICE, cg_dev)
> > > diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
> > > index 49ba9cde7e4b..b29950a19205 100644
> > > --- a/include/linux/tracepoint-defs.h
> > > +++ b/include/linux/tracepoint-defs.h
> > > @@ -45,6 +45,7 @@ struct bpf_raw_event_map {
> > >  	struct tracepoint	*tp;
> > >  	void			*bpf_func;
> > >  	u32			num_args;
> > > +	u32			writable_size;
> > >  } __aligned(32);
> > >  
> > >  #endif
> > > diff --git a/include/trace/bpf_probe.h b/include/trace/bpf_probe.h
> > > index 505dae0bed80..d6e556c0a085 100644
> > > --- a/include/trace/bpf_probe.h
> > > +++ b/include/trace/bpf_probe.h
> > > @@ -69,8 +69,7 @@ __bpf_trace_##call(void *__data, proto)					\
> > >   * to make sure that if the tracepoint handling changes, the
> > >   * bpf probe will fail to compile unless it too is updated.
> > >   */
> > > -#undef DEFINE_EVENT
> > > -#define DEFINE_EVENT(template, call, proto, args)			\
> > > +#define __DEFINE_EVENT(template, call, proto, args, size)		\
> > >  static inline void bpf_test_probe_##call(void)				\
> > >  {									\
> > >  	check_trace_callback_type_##call(__bpf_trace_##template);	\
> > > @@ -81,12 +80,36 @@ __bpf_trace_tp_map_##call = {						\
> > >  	.tp		= &__tracepoint_##call,				\
> > >  	.bpf_func	= (void *)__bpf_trace_##template,		\
> > >  	.num_args	= COUNT_ARGS(args),				\
> > > +	.writable_size	= size,						\
> > >  };
> > >  
> > > +#define FIRST(x, ...) x
> > > +
> > > +#undef DEFINE_EVENT_WRITABLE
> > > +#define DEFINE_EVENT_WRITABLE(template, call, proto, args, size)	\
> > > +static inline void bpf_test_buffer_##call(void)				\
> > > +{									\
> > > +	/* BUILD_BUG_ON() is ignored if the code is completely eliminated, but \
> > > +	 * BUILD_BUG_ON_ZERO() uses a different mechanism that is not	\
> > > +	 * dead-code-eliminated.					\
> > > +	 */								\
> > > +	FIRST(proto);							\
> > > +	(void)BUILD_BUG_ON_ZERO(size != sizeof(*FIRST(args)));		\
> > > +}									\
> > > +__DEFINE_EVENT(template, call, PARAMS(proto), PARAMS(args), size)
> > > +
> > > +#undef DEFINE_EVENT
> > > +#define DEFINE_EVENT(template, call, proto, args)			\
> > > +	__DEFINE_EVENT(template, call, PARAMS(proto), PARAMS(args), 0)
> > >  
> > >  #undef DEFINE_EVENT_PRINT
> > >  #define DEFINE_EVENT_PRINT(template, name, proto, args, print)	\
> > >  	DEFINE_EVENT(template, name, PARAMS(proto), PARAMS(args))
> > >  
> > >  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> > > +
> > > +#undef DEFINE_EVENT_WRITABLE
> > > +#undef __DEFINE_EVENT
> > > +#undef FIRST
> > > +
> > >  #endif /* CONFIG_BPF_EVENTS */
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index 3c38ac9a92a7..c5335d53ce82 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -166,6 +166,7 @@ enum bpf_prog_type {
> > >  	BPF_PROG_TYPE_LIRC_MODE2,
> > >  	BPF_PROG_TYPE_SK_REUSEPORT,
> > >  	BPF_PROG_TYPE_FLOW_DISSECTOR,
> > > +	BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE,
> > >  };
> > >  
> > >  enum bpf_attach_type {
> > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > > index 62f6bced3a3c..27e2f22879a4 100644
> > > --- a/kernel/bpf/syscall.c
> > > +++ b/kernel/bpf/syscall.c
> > > @@ -1720,12 +1720,16 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
> > >  	}
> > >  	raw_tp->btp = btp;
> > >  
> > > -	prog = bpf_prog_get_type(attr->raw_tracepoint.prog_fd,
> > > -				 BPF_PROG_TYPE_RAW_TRACEPOINT);
> > > +	prog = bpf_prog_get(attr->raw_tracepoint.prog_fd);
> > >  	if (IS_ERR(prog)) {
> > >  		err = PTR_ERR(prog);
> > >  		goto out_free_tp;
> > >  	}
> > > +	if (prog->type != BPF_PROG_TYPE_RAW_TRACEPOINT &&
> > > +	    prog->type != BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE) {
> > 
> > I don't think we'd gain a lot by making this an extra prog type which can do the
> > same as BPF_PROG_TYPE_RAW_TRACEPOINT modulo optional writing. Why not integrating
> > this directly into BPF_PROG_TYPE_RAW_TRACEPOINT then? The actual opt-in comes from
> > the DEFINE_EVENT_WRITABLE(), not from the prog type.
> 
> I did that to separate the hook into
> raw_tp_writable_prog_is_valid_access, which (compared to
> raw_tp_prog_is_valid_access):
> 
>   1) permits writes, and
>   2) encodes the assumption than the context begins with the pointer to
> that writable buffer
> 
> I'm not sure those are appropriate for all users of
> BPF_PROG_TYPE_RAW_TRACEPOINT, but I can't immediately point out any
> harm in doing so -- some dereferences of ctx that have historically
> returned a SCALAR_VALUE would end up tagged as a PTR_TO_TP_BUFFER, but
> they still won't be able to access through that pointer unless they're
> attached to the right tracepoint.
> 
> I'll try to unify the two and see what I get.

I think combining raw_tp prog type with raw_tp_writeable is possible,
but too cumbersome to use from user pov.
Since such raw_tp will be accepted at prog load time,
but only at the time of attach it will be rejected in __bpf_probe_register.

raw_tp_writable_prog_is_valid_access() doesn't know future attach point.
we cannot use bpf_attr.expected_attach_type here.
That's why it simply does:
+       if (off == 0 && size == sizeof(u64))
+               info->reg_type = PTR_TO_TP_BUFFER;

essentially hard coding first argument of writeable tp to be the buffer.

TP invocation is also new.
Like in patch 2:
trace_nbd_send_request(&request, nbd->index, blk_mq_rq_from_pdu(cmd));

Patches 1,2,3 actually look fine to me,
but I agree that selftests are necessary.

Matt, could you add new writeable tracepoint somewhere in bpf_test_run()
and corresponding selftest?

Thanks

Re: [PATCH bpf-next 1/3] bpf: add writable context for raw tracepoints

[Matt Mullins]

On Thu, 2019-04-04 at 18:17 -0700, Alexei Starovoitov wrote:
> On Wed, Apr 03, 2019 at 06:39:12PM +0000, Matt Mullins wrote:
> > On Mon, 2019-04-01 at 22:40 +0200, Daniel Borkmann wrote:
> > > On 03/29/2019 01:07 AM, Matt Mullins wrote:
> > > > This is an opt-in interface that allows a tracepoint to provide a safe
> > > > buffer that can be written from a BPF_PROG_TYPE_RAW_TRACEPOINT program.
> > > > The size of the buffer must be a compile-time constant, and is checked
> > > > before allowing a BPF program to attach to a tracepoint that uses this
> > > > feature.
> > > > 
> > > > The pointer to this buffer will be the first argument of tracepoints
> > > > that opt in; the buffer is readable by both BPF_PROG_TYPE_RAW_TRACEPOINT
> > > > and BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE programs that attach to such a
> > > > tracepoint, but the buffer to which it points may only be written by the
> > > > latter.
> > > > 
> > > > bpf_probe: assert that writable tracepoint size is correct
> > > 
> > > Maybe also add a kselftest into bpf test suite to i) demo it and ii) make
> > > sure it's continuously been tested by bots running the suite?
> > 
> > Will do.
> > 
> > > 
> > > > Signed-off-by: Matt Mullins <mmullins@fb.com>
> > > > ---
> > > >  include/linux/bpf.h             |  2 ++
> > > >  include/linux/bpf_types.h       |  1 +
> > > >  include/linux/tracepoint-defs.h |  1 +
> > > >  include/trace/bpf_probe.h       | 27 +++++++++++++++++++++++++--
> > > >  include/uapi/linux/bpf.h        |  1 +
> > > >  kernel/bpf/syscall.c            |  8 ++++++--
> > > >  kernel/bpf/verifier.c           | 11 +++++++++++
> > > >  kernel/trace/bpf_trace.c        | 21 +++++++++++++++++++++
> > > >  8 files changed, 68 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > > index a2132e09dc1c..d3c71fd67476 100644
> > > > --- a/include/linux/bpf.h
> > > > +++ b/include/linux/bpf.h
> > > > @@ -263,6 +263,7 @@ enum bpf_reg_type {
> > > >  	PTR_TO_SOCK_COMMON_OR_NULL, /* reg points to sock_common or NULL */
> > > >  	PTR_TO_TCP_SOCK,	 /* reg points to struct tcp_sock */
> > > >  	PTR_TO_TCP_SOCK_OR_NULL, /* reg points to struct tcp_sock or NULL */
> > > > +	PTR_TO_TP_BUFFER,	 /* reg points to a writable raw tp's buffer */
> > > >  };
> > > >  
> > > >  /* The information passed from prog-specific *_is_valid_access
> > > > @@ -352,6 +353,7 @@ struct bpf_prog_aux {
> > > >  	u32 used_map_cnt;
> > > >  	u32 max_ctx_offset;
> > > >  	u32 max_pkt_offset;
> > > > +	u32 max_tp_access;
> > > >  	u32 stack_depth;
> > > >  	u32 id;
> > > >  	u32 func_cnt; /* used by non-func prog as the number of func progs */
> > > > diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> > > > index 08bf2f1fe553..c766108608cb 100644
> > > > --- a/include/linux/bpf_types.h
> > > > +++ b/include/linux/bpf_types.h
> > > > @@ -25,6 +25,7 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_KPROBE, kprobe)
> > > >  BPF_PROG_TYPE(BPF_PROG_TYPE_TRACEPOINT, tracepoint)
> > > >  BPF_PROG_TYPE(BPF_PROG_TYPE_PERF_EVENT, perf_event)
> > > >  BPF_PROG_TYPE(BPF_PROG_TYPE_RAW_TRACEPOINT, raw_tracepoint)
> > > > +BPF_PROG_TYPE(BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE, raw_tracepoint_writable)
> > > >  #endif
> > > >  #ifdef CONFIG_CGROUP_BPF
> > > >  BPF_PROG_TYPE(BPF_PROG_TYPE_CGROUP_DEVICE, cg_dev)
> > > > diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
> > > > index 49ba9cde7e4b..b29950a19205 100644
> > > > --- a/include/linux/tracepoint-defs.h
> > > > +++ b/include/linux/tracepoint-defs.h
> > > > @@ -45,6 +45,7 @@ struct bpf_raw_event_map {
> > > >  	struct tracepoint	*tp;
> > > >  	void			*bpf_func;
> > > >  	u32			num_args;
> > > > +	u32			writable_size;
> > > >  } __aligned(32);
> > > >  
> > > >  #endif
> > > > diff --git a/include/trace/bpf_probe.h b/include/trace/bpf_probe.h
> > > > index 505dae0bed80..d6e556c0a085 100644
> > > > --- a/include/trace/bpf_probe.h
> > > > +++ b/include/trace/bpf_probe.h
> > > > @@ -69,8 +69,7 @@ __bpf_trace_##call(void *__data, proto)					\
> > > >   * to make sure that if the tracepoint handling changes, the
> > > >   * bpf probe will fail to compile unless it too is updated.
> > > >   */
> > > > -#undef DEFINE_EVENT
> > > > -#define DEFINE_EVENT(template, call, proto, args)			\
> > > > +#define __DEFINE_EVENT(template, call, proto, args, size)		\
> > > >  static inline void bpf_test_probe_##call(void)				\
> > > >  {									\
> > > >  	check_trace_callback_type_##call(__bpf_trace_##template);	\
> > > > @@ -81,12 +80,36 @@ __bpf_trace_tp_map_##call = {						\
> > > >  	.tp		= &__tracepoint_##call,				\
> > > >  	.bpf_func	= (void *)__bpf_trace_##template,		\
> > > >  	.num_args	= COUNT_ARGS(args),				\
> > > > +	.writable_size	= size,						\
> > > >  };
> > > >  
> > > > +#define FIRST(x, ...) x
> > > > +
> > > > +#undef DEFINE_EVENT_WRITABLE
> > > > +#define DEFINE_EVENT_WRITABLE(template, call, proto, args, size)	\
> > > > +static inline void bpf_test_buffer_##call(void)				\
> > > > +{									\
> > > > +	/* BUILD_BUG_ON() is ignored if the code is completely eliminated, but \
> > > > +	 * BUILD_BUG_ON_ZERO() uses a different mechanism that is not	\
> > > > +	 * dead-code-eliminated.					\
> > > > +	 */								\
> > > > +	FIRST(proto);							\
> > > > +	(void)BUILD_BUG_ON_ZERO(size != sizeof(*FIRST(args)));		\
> > > > +}									\
> > > > +__DEFINE_EVENT(template, call, PARAMS(proto), PARAMS(args), size)
> > > > +
> > > > +#undef DEFINE_EVENT
> > > > +#define DEFINE_EVENT(template, call, proto, args)			\
> > > > +	__DEFINE_EVENT(template, call, PARAMS(proto), PARAMS(args), 0)
> > > >  
> > > >  #undef DEFINE_EVENT_PRINT
> > > >  #define DEFINE_EVENT_PRINT(template, name, proto, args, print)	\
> > > >  	DEFINE_EVENT(template, name, PARAMS(proto), PARAMS(args))
> > > >  
> > > >  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> > > > +
> > > > +#undef DEFINE_EVENT_WRITABLE
> > > > +#undef __DEFINE_EVENT
> > > > +#undef FIRST
> > > > +
> > > >  #endif /* CONFIG_BPF_EVENTS */
> > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > > index 3c38ac9a92a7..c5335d53ce82 100644
> > > > --- a/include/uapi/linux/bpf.h
> > > > +++ b/include/uapi/linux/bpf.h
> > > > @@ -166,6 +166,7 @@ enum bpf_prog_type {
> > > >  	BPF_PROG_TYPE_LIRC_MODE2,
> > > >  	BPF_PROG_TYPE_SK_REUSEPORT,
> > > >  	BPF_PROG_TYPE_FLOW_DISSECTOR,
> > > > +	BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE,
> > > >  };
> > > >  
> > > >  enum bpf_attach_type {
> > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > > > index 62f6bced3a3c..27e2f22879a4 100644
> > > > --- a/kernel/bpf/syscall.c
> > > > +++ b/kernel/bpf/syscall.c
> > > > @@ -1720,12 +1720,16 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
> > > >  	}
> > > >  	raw_tp->btp = btp;
> > > >  
> > > > -	prog = bpf_prog_get_type(attr->raw_tracepoint.prog_fd,
> > > > -				 BPF_PROG_TYPE_RAW_TRACEPOINT);
> > > > +	prog = bpf_prog_get(attr->raw_tracepoint.prog_fd);
> > > >  	if (IS_ERR(prog)) {
> > > >  		err = PTR_ERR(prog);
> > > >  		goto out_free_tp;
> > > >  	}
> > > > +	if (prog->type != BPF_PROG_TYPE_RAW_TRACEPOINT &&
> > > > +	    prog->type != BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE) {
> > > 
> > > I don't think we'd gain a lot by making this an extra prog type which can do the
> > > same as BPF_PROG_TYPE_RAW_TRACEPOINT modulo optional writing. Why not integrating
> > > this directly into BPF_PROG_TYPE_RAW_TRACEPOINT then? The actual opt-in comes from
> > > the DEFINE_EVENT_WRITABLE(), not from the prog type.
> > 
> > I did that to separate the hook into
> > raw_tp_writable_prog_is_valid_access, which (compared to
> > raw_tp_prog_is_valid_access):
> > 
> >   1) permits writes, and
> >   2) encodes the assumption than the context begins with the pointer to
> > that writable buffer
> > 
> > I'm not sure those are appropriate for all users of
> > BPF_PROG_TYPE_RAW_TRACEPOINT, but I can't immediately point out any
> > harm in doing so -- some dereferences of ctx that have historically
> > returned a SCALAR_VALUE would end up tagged as a PTR_TO_TP_BUFFER, but
> > they still won't be able to access through that pointer unless they're
> > attached to the right tracepoint.
> > 
> > I'll try to unify the two and see what I get.
> 
> I think combining raw_tp prog type with raw_tp_writeable is possible,
> but too cumbersome to use from user pov.
> Since such raw_tp will be accepted at prog load time,
> but only at the time of attach it will be rejected in __bpf_probe_register.
> 
> raw_tp_writable_prog_is_valid_access() doesn't know future attach point.
> we cannot use bpf_attr.expected_attach_type here.

We could use any load-time flag of sorts, but I overlooked
expected_attach_type since that should match the enum bpf_attach_type
values, and we don't use BPF_PROG_ATTACH for tracepoints.

> That's why it simply does:
> +       if (off == 0 && size == sizeof(u64))
> +               info->reg_type = PTR_TO_TP_BUFFER;
> 
> essentially hard coding first argument of writeable tp to be the buffer.

It appears to be possible to xdo this to all
BPF_PROG_TYPE_RAW_TRACEPOINTs, but I think it's clearer to keep a
separate bpf_prog_type for programs that assume ctx[0] is the pointer
to the buffer.

> 
> TP invocation is also new.
> Like in patch 2:
> trace_nbd_send_request(&request, nbd->index, blk_mq_rq_from_pdu(cmd));
> 
> Patches 1,2,3 actually look fine to me,
> but I agree that selftests are necessary.
> 
> Matt, could you add new writeable tracepoint somewhere in bpf_test_run()
> and corresponding selftest?

Yep, I've just got that working, will be included in v2 (along with
some other minor bugfixes).

> 
> Thanks
>