On Mon, 2019-04-22 at 21:17 +0000, Yonghong Song wrote:
>
> On 4/22/19 12:23 PM, Matt Mullins wrote:
> > On Mon, 2019-04-22 at 18:12 +0000, Yonghong Song wrote:
> > >
> > > 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@xxxxxx>
> > > > ---
> > > > 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)?
> >
> > As far as I can tell, pointers are always 64 bits wide from the
> > perspective of the eBPF instruction set. I think the proper fixup is
> > in include/trace/events/nbd.h ... I should use a u64 instead of a
> > pointer type.
>
> u64 is okay. You may want to double check tracepoint definition to
> ensure the assign to the first argument converting to u64 as well to
> avoid potential garbage. It would be good if this is enforced during
> compilation time.
Now that I've looked into this further, this is already handled in
include/trace/bpf_probe.h:
#undef DECLARE_EVENT_CLASS
#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
static notrace void \
__bpf_trace_##call(void *__data, proto) \
{ \
struct bpf_prog *prog = __data; \
CONCATENATE(bpf_trace_run, COUNT_ARGS(args))(prog, CAST_TO_U64(args)); \
}
The 32-bit value of the struct nbd_request pointer will be zero-
extended to 64-bits before the BPF program sees it, so there won't be
any garbage in the upper half of the register. I'm going to leave the
trace_* functions taking the pointer as-is, so that I can keep the
compile-time checks that writable_size == sizeof(*first_argument).
> > > Should the first argument for raw_tp_writable_prog be always
> > > PTR_TO_TP_BUFFER?
> >
> > That is the purpose of this patch series, yes. My initial attempt at
> > this tried to add it to the end of the context structure instead, and
> > that ended up being quite complex to track.
>
> So `size == sizeof(u64)` can be removed, off == 0 just implies
> reg_type PTR_TO_TP_BUFFER?
I can't get rid of the size check, because I can emit an opcode like
0: (71) r6 = *(u8 *)(r1 +0)
and I don't want to accidentally mark a value as PTR_TO_TP_BUFFER
unless it is a whole, valid pointer.
> > > > + 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);
> > > > }
> > > >
> > > >
