Re: Re: [PATCH 1/5] bpf: Allow non struct type for btf ctx access

[Jiri Olsa <jolsa@redhat.com>]

On Tue, Jan 07, 2020 at 06:28:11PM +0000, Yonghong Song wrote:

SNIP

> >> index ed2075884724..650df4ed346e 100644
> >> --- a/kernel/bpf/btf.c
> >> +++ b/kernel/bpf/btf.c
> >> @@ -3633,7 +3633,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> >>    		    const struct bpf_prog *prog,
> >>    		    struct bpf_insn_access_aux *info)
> >>    {
> >> -	const struct btf_type *t = prog->aux->attach_func_proto;
> >> +	const struct btf_type *tp, *t = prog->aux->attach_func_proto;
> >>    	struct bpf_prog *tgt_prog = prog->aux->linked_prog;
> >>    	struct btf *btf = bpf_prog_get_target_btf(prog);
> >>    	const char *tname = prog->aux->attach_func_name;
> >> @@ -3695,6 +3695,17 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> >>    		 */
> >>    		return true;
> >>    
> >> +	tp = btf_type_by_id(btf, t->type);
> >> +	/* skip modifiers */
> >> +	while (btf_type_is_modifier(tp))
> >> +		tp = btf_type_by_id(btf, tp->type);
> >> +
> >> +	if (btf_type_is_int(tp))
> >> +		/* This is a pointer scalar.
> >> +		 * It is the same as scalar from the verifier safety pov.
> >> +		 */
> >> +		return true;
> > 
> > This should work since:
> >      - the int pointer will be treated as a scalar later on
> >      - bpf_probe_read() will be used to read the contents
> > 
> > I am wondering whether we should add proper verifier support
> > to allow pointer to int ctx access. There, users do not need
> > to use bpf_probe_read() to dereference the pointer.
> > 
> > Discussed with Martin, maybe somewhere in check_ptr_to_btf_access(),
> > before btf_struct_access(), checking if it is a pointer to int/enum,
> > it should just allow and return SCALAR_VALUE.
> 
> double checked check_ptr_to_btf_access() and btf_struct_access().
> btf_struct_access() already returns SCALAR_VALUE for pointer to 
> int/enum. So verifier change is probably not needed.

ok, great

> 
> In your above code, could you do
>     btf_type_is_int(t) || btf_type_is_enum(t)
> which will cover pointer to enum as well?

sure, I'll include that

thanks,
jirka