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

Yonghong Song
2019-04-22T18:13:21Z

On 4/19/19 2:04 PM, 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.
>
> 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 | 31 +++++++++++++++++++++++++++++++
> kernel/trace/bpf_trace.c | 21 +++++++++++++++++++++
> 8 files changed, 88 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 */
> };
>
[...]
> /* truncate register to smaller size (in bytes)
> * must be called with size < BPF_REG_SIZE
> */
> @@ -2100,6 +2127,10 @@ 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) {
> + err = check_tp_buffer_access(env, reg, regno, off, size);
> + if (!err && 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;

on 32bit system, the first argument pointer size could be sizeof(u32)?
Should the first argument for raw_tp_writable_prog be always
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);
> }
>
>