Re: [PATCH bpf-next 2/8] bpf: Allow trusted args to walk struct when checking BTF IDs

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: David Vernet <void@manifault.com>
Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com>,
	bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Martin KaFai Lau <martin.lau@linux.dev>,
	Song Liu <song@kernel.org>, Yonghong Song <yhs@meta.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@kernel.org>,
	Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>,
	Jiri Olsa <jolsa@kernel.org>, LKML <linux-kernel@vger.kernel.org>,
	Kernel Team <kernel-team@meta.com>, Tejun Heo <tj@kernel.org>
Subject: Re: [PATCH bpf-next 2/8] bpf: Allow trusted args to walk struct when checking BTF IDs
Date: Fri, 20 Jan 2023 08:17:18 -0800	[thread overview]
Message-ID: <CAADnVQ+sn9imVimk2DnjxtTfmzddSehT2ucAcatK5rhMTd46fw@mail.gmail.com> (raw)
In-Reply-To: <Y8qyovnr2bkEpldc@maniforge.lan>

On Fri, Jan 20, 2023 at 7:26 AM David Vernet <void@manifault.com> wrote:
> > >
> > > Yes. Agree. I used unfortunate example in the previous reply with nf_conn___init.
> > > I meant to say:
> > >
> > >  For definition:
> > >  struct nf_conn_init {
> > >     struct nf_conn ct;
> > >  };
> > >  if a kfunc accepts a pointer to nf_conn it should always accept a pointer to nf_conn_init
> > >  for both read and write, because in C that's valid and safe type cast.
> > >
> > > Meainng that C rules apply.
> > > Our triple underscore is special, because it's the "same type".
> > > In the 2nd part of my reply I'm proposing to use the whole suffix "___init" to indicate that.
> > > I think you're arguing that just "___" part is enough to enforce strict match.
> > > Matching foo___flavor with foo should not be allowed.
> > > While passing struct foo_flavor {struct foo;} into a kfunc that accepts 'struct foo'
> > > is safe.
> > > If so, I'm fine with such approach.
> >
> > Alright, I'll spin v2 to treat any type with name___.* as a disallowed
> > alias, and update the documentation to mention it. I was originally
> > going to push back and say that we should just use a single alias like
> > __nocast to keep things simple, but it doesn't feel generalizable
> > enough.
>
> On second thought, unless you guys feel strongly, I'll just check
> ___init. The resulting code is going to be a lot of tricky string
> manipulation / math otherwise. Not _terrible_, but I'd prefer to avoid
> adding it until we have a concrete use-case. And I expect this could be
> implemented much simpler using something like tags, once gcc has support
> for it.

There is bpf_core_is_flavor_sep() that makes it easy to check,
but thinking more about it we probably should stick to strict "___init"
suffix for now, since flavors can appear in progs too and they
are equivalent to corresponding types in the kernel.
The nf_conn___init is kernel only type.
The verifier sees that bpf_xdp_ct_alloc kfunc returns it,
but when we export this kfunc to bpf prog it returns nf_conn.
We probably should pick some other suffix without "___" to distinguish
from flavors. bpf prog should not use nf_conn___init.