Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 20 Jan 2023 10:28:15 +0530
From:   Kumar Kartikeya Dwivedi <memxor@gmail.com>
To:     David Vernet <void@manifault.com>
Cc:     bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
        andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
        yhs@meta.com, john.fastabend@gmail.com, kpsingh@kernel.org,
        sdf@google.com, haoluo@google.com, jolsa@kernel.org,
        linux-kernel@vger.kernel.org, kernel-team@meta.com, tj@kernel.org
Subject: Re: [PATCH bpf-next 2/8] bpf: Allow trusted args to walk struct when
 checking BTF IDs
Message-ID: <20230120045815.4b7dc6obdt4uzy6a@apollo>
References: <20230119235833.2948341-1-void@manifault.com>
 <20230119235833.2948341-3-void@manifault.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230119235833.2948341-3-void@manifault.com>
Precedence: bulk

On Fri, Jan 20, 2023 at 05:28:27AM IST, David Vernet wrote:
> When validating BTF types for KF_TRUSTED_ARGS kfuncs, the verifier
> currently enforces that the top-level type must match when calling
> the kfunc. In other words, the verifier does not allow the BPF program
> to pass a bitwise equivalent struct, despite it being functionally safe.
> For example, if you have the following type:
>
> struct  nf_conn___init {
> 	struct nf_conn ct;
> };
>
> It would be safe to pass a struct nf_conn___init to a kfunc expecting a
> struct nf_conn.

Just running bpf_nf selftest would have shown this is false.

> Being able to do this will be useful for certain types
> of kfunc / kptrs enabled by BPF. For example, in a follow-on patch, a
> series of kfuncs will be added which allow programs to do bitwise
> queries on cpumasks that are either allocated by the program (in which
> case they'll be a 'struct bpf_cpumask' type that wraps a cpumask_t as
> its first element), or a cpumask that was allocated by the main kernel
> (in which case it will just be a straight cpumask_t, as in
>  task->cpus_ptr).
>
> Having the two types of cpumasks allows us to distinguish between the
> two for when a cpumask is read-only vs. mutatable. A struct bpf_cpumask
> can be mutated by e.g. bpf_cpumask_clear(), whereas a regular cpumask_t
> cannot be. On the other hand, a struct bpf_cpumask can of course be
> queried in the exact same manner as a cpumask_t, with e.g.
> bpf_cpumask_test_cpu().
>
> If we were to enforce that top level types match, then a user that's
> passing a struct bpf_cpumask to a read-only cpumask_t argument would
> have to cast with something like bpf_cast_to_kern_ctx() (which itself
> would need to be updated to expect the alias, and currently it only
> accommodates a single alias per prog type). Additionally, not specifying
> KF_TRUSTED_ARGS is not an option, as some kfuncs take one argument as a
> struct bpf_cpumask *, and another as a struct cpumask *
> (i.e. cpumask_t).
>
> In order to enable this, this patch relaxes the constraint that a
> KF_TRUSTED_ARGS kfunc must have strict type matching. In order to
> try and be conservative and match existing behavior / expectations, this
> patch also enforces strict type checking for acquire kfuncs. We were
> already enforcing it for release kfuncs, so this should also improve the
> consistency of the semantics for kfuncs.
>

What you want is to simply follow type at off = 0 (but still enforce the off = 0
requirement). This is something which is currently done for bpf_sk_release (for
struct sk_common) in check_reg_type, but it is not safe in general to just open
this up for all cases. I suggest encoding this particular requirement in the
argument, and simply using triple underscore variant of the type for the special
'read_only' requirement. This will allow you to use same type in your BPF C
program, while allowing verifier to see them as two different types in kfunc
parameters. Then just relax type following for the particular argument so that
one can pass cpumask_t___ro to kfunc expecting cpumask_t (but only at off = 0,
it just visits first member after failing match on top level type). off = 0
check is still necessary.

So offset checks still need to be according to OBJ_RELEASE but you can relax
strict_type_match bool for the particular arg when calling btf_struct_ids_match.

All code in your tests will then deal with a cpumask_t type only, including in
kfunc declarations. Same as bpf_nf selftests which don't cast from/to
nf_conn___init and only deal with nf_conn pointers even though semantics differ
depending on how it is used and passed around. Overall more convenient and
simple to use.