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

[Yonghong Song]

On 4/22/19 4:01 PM, Matt Mullins wrote:
> 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.

We could reject if off == 0 && size != 8 to allow only whole pointer 
access. I cannot think that user will use r6 in a meaningful except the 
hash. But if you have a valid use case to permit such access, I
am okay with that.

> 
>>>>> +	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);
>>>>>     }
>>>>>     
>>>>>