Re: [PATCH 1/6] bpf: Allow ctx access for pointers to scalar

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Wed, Jan 22, 2020 at 10:13:36AM +0100, Jiri Olsa wrote:
> On Wed, Jan 22, 2020 at 02:33:32AM +0000, Yonghong Song wrote:
> > 
> > 
> > On 1/21/20 5:51 PM, Alexei Starovoitov wrote:
> > > On Tue, Jan 21, 2020 at 4:05 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >>
> > >> When accessing the context we allow access to arguments with
> > >> scalar type and pointer to struct. But we omit pointer to scalar
> > >> type, which is the case for many functions and same case as
> > >> when accessing scalar.
> > >>
> > >> Adding the check if the pointer is to scalar type and allow it.
> > >>
> > >> Acked-by: John Fastabend <john.fastabend@gmail.com>
> > >> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > >> ---
> > >>   kernel/bpf/btf.c | 13 ++++++++++++-
> > >>   1 file changed, 12 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > >> index 832b5d7fd892..207ae554e0ce 100644
> > >> --- a/kernel/bpf/btf.c
> > >> +++ b/kernel/bpf/btf.c
> > >> @@ -3668,7 +3668,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;
> > >> @@ -3730,6 +3730,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) || btf_type_is_enum(tp))
> > >> +               /* This is a pointer scalar.
> > >> +                * It is the same as scalar from the verifier safety pov.
> > >> +                */
> > >> +               return true;
> > > 
> > > The reason I didn't do it earlier is I was thinking to represent it
> > > as PTR_TO_BTF_ID as well, so that corresponding u8..u64
> > > access into this memory would still be possible.
> > > I'm trying to analyze the situation that returning a scalar now
> > > and converting to PTR_TO_BTF_ID in the future will keep progs
> > > passing the verifier. Is it really the case?
> > > Could you give a specific example that needs this support?
> > > It will help me understand this backward compatibility concern.
> > > What prog is doing with that 'u32 *' that is seen as scalar ?
> > > It cannot dereference it. Use it as what?
> > 
> > If this is from original bcc code, it will use bpf_probe_read for 
> > dereference. This is what I understand when I first reviewed this patch.
> > But it will be good to get Jiri's confirmation.
> 
> it blocked me from accessing 'filename' argument when I probed
> do_sys_open via trampoline in bcc, like:
> 
> 	KRETFUNC_PROBE(do_sys_open)
> 	{
> 	    const char *filename = (const char *) args[1];
> 
> AFAICS the current code does not allow for trampoline arguments
> being other pointers than to void or struct, the patch should
> detect that the argument is pointer to scalar type and let it
> pass

Got it. I've looked up your bcc patches and I agree that there is no way to
workaround. BTF type argument of that kernel function is 'const char *' and the
verifier will enforce that if bpf program tries to cast it the verifier will
still see 'const char *'. (It's done this way by design). How about we special
case 'char *' in the verifier? Then my concern regarding future extensibility
of 'int *' and 'long *' will go away.
Compilers have a long history special casing 'char *'. In particular signed
char because it's a pointer to null terminated string. I think it's still a
special pointer from pointer aliasing point of view. I think the verifier can
treat it as scalar here too. In the future the verifier will get smarter and
will recognize it as PTR_TO_NULL_STRING while 'u8 *', 'u32 *' will be
PTR_TO_BTF_ID. I think it will solve this particular issue. I like conservative
approach to the verifier improvements: start with strict checking and relax it
on case-by-case. Instead of accepting wide range of cases and cause potential
compatibility issues.