On Sat, Apr 9, 2022 at 2:32 AM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote:
>
> Next commit introduces field type 'kptr' whose kind will not be struct,
> but pointer, and it will not be limited to one offset, but multiple
> ones. Make existing btf_find_struct_field and btf_find_datasec_var
> functions amenable to use for finding kptrs in map value, by moving
> spin_lock and timer specific checks into their own function.
>
> The alignment, and name are checked before the function is called, so it
> is the last point where we can skip field or return an error before the
> next loop iteration happens. The name parameter is now optional, and
> only checked if it is not NULL. Size of the field and type is meant to
> be checked inside the function, and base type will need to be obtained
> by skipping modifiers.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> ---
> kernel/bpf/btf.c | 129 +++++++++++++++++++++++++++++++++++------------
> 1 file changed, 96 insertions(+), 33 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 0918a39279f6..db7bf05adfc5 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3163,71 +3163,126 @@ static void btf_struct_log(struct btf_verifier_env *env,
> btf_verifier_log(env, "size=%u vlen=%u", t->size, btf_type_vlen(t));
> }
>
> +enum {
> + BTF_FIELD_SPIN_LOCK,
> + BTF_FIELD_TIMER,
> +};
> +
> +struct btf_field_info {
> + u32 off;
> +};
> +
> +static int btf_find_field_struct(const struct btf *btf, const struct btf_type *t,
> + u32 off, int sz, struct btf_field_info *info)
> +{
> + if (!__btf_type_is_struct(t))
> + return 0;
> + if (t->size != sz)
> + return 0;
Do we need these two checks? I think in the original version we did
because we were checking this before doing the name comparison, but
now that the name comparison check is first, if the struct name is a
match, then won't these two things always be true (or if not, then it
seems like we should return -EINVAL)? But maybe I'm missing something
here - I'm still getting more familiar with BTF :)
Also, as a side-note: I personally find this function name
"btf_find_field_struct" confusing since there's also the
"btf_find_struct_field" function that exists. I wonder whether we
should just keep the logic inside btf_find_struct_field instead of
putting it in this separate function?
> + if (info->off != -ENOENT)
> + /* only one such field is allowed */
> + return -E2BIG;
In the future, do you plan to add support for multiple fields? I think
this would be useful for dynptrs as well, so just curious what your
plans for this are.
> + info->off = off;
> + return 0;
> +}
> +
> static int btf_find_struct_field(const struct btf *btf, const struct btf_type *t,
> - const char *name, int sz, int align)
> + const char *name, int sz, int align, int field_type,
What are your thoughts on just passing in field_type in place of name,
sz, and align? As in a function signature like:
static int btf_find_struct_field(const struct btf *btf, const struct
btf_type *t, int field_type, struct btf_field_info *info);
where inside btf_find_struct_field when we do the switch statement on
field_type, we can have the name, sz, and align for each of the
different field types there? That to me seems a bit cleaner where the
descriptors for the field types are all in one place (instead of also
in btf_find_spin_lock() and btf_find_timer() functions) and the
function definition for btf_find_struct_field is more straightforward.
At that point, I don't think we'd even need btf_find_spin_lock() and
btf_find_timer() as functions since it'd be just a straightforward
"btf_find_field(btf, t, BTF_FIELD_SPIN_LOCK/BTF_FIELD_TIMER)" call
instead. Curious to hear your thoughts.
nit: should field_type be a u32 since it's an enum? Or should we be
explicit and give the enum a name and define this as something like
"enum btf_field_type type"?
> + struct btf_field_info *info)
> {
> const struct btf_member *member;
> - u32 i, off = -ENOENT;
> + u32 i, off;
> + int ret;
>
> for_each_member(i, t, member) {
> const struct btf_type *member_type = btf_type_by_id(btf,
> member->type);
> - if (!__btf_type_is_struct(member_type))
> - continue;
> - if (member_type->size != sz)
> - continue;
> - if (strcmp(__btf_name_by_offset(btf, member_type->name_off), name))
> - continue;
> - if (off != -ENOENT)
> - /* only one such field is allowed */
> - return -E2BIG;
> +
> off = __btf_member_bit_offset(t, member);
nit: should this be moved to after the strcmp on the name? Since if
the name doesn't match, there's no point in doing this
__btf_member_bit_offset call
> +
> + if (name && strcmp(__btf_name_by_offset(btf, member_type->name_off), name))
I'm confused by the if (name) part of the check. If name is NULL, then
won't this "btf_find_struct_field" function always return the offset
to the first struct? I don't think name will ever be NULL so maybe we
should just remove this? Or do something like if (name) return
-EINVAL; before doing the strcmp?
> + continue;
> if (off % 8)
> /* valid C code cannot generate such BTF */
> return -EINVAL;
> off /= 8;
> if (off % align)
> return -EINVAL;
> +
> + switch (field_type) {
> + case BTF_FIELD_SPIN_LOCK:
> + case BTF_FIELD_TIMER:
> + ret = btf_find_field_struct(btf, member_type, off, sz, info);
nit: I think we can just do "return btf_find_field_struct(btf,
member_type, off, sz, info);" here and remove the "int ret;"
declaration a few lines above.
> + if (ret < 0)
> + return ret;
> + break;
> + default:
> + return -EFAULT;
> + }
> }
> - return off;
> + return 0;
> }
>
> static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
> - const char *name, int sz, int align)
> + const char *name, int sz, int align, int field_type,
> + struct btf_field_info *info)
The same comments for the btf_find_struct_field function also apply to
this function
> {
> const struct btf_var_secinfo *vsi;
> - u32 i, off = -ENOENT;
> + u32 i, off;
> + int ret;
>
> for_each_vsi(i, t, vsi) {
> const struct btf_type *var = btf_type_by_id(btf, vsi->type);
> const struct btf_type *var_type = btf_type_by_id(btf, var->type);
>
> - if (!__btf_type_is_struct(var_type))
> - continue;
> - if (var_type->size != sz)
> + off = vsi->offset;
> +
> + if (name && strcmp(__btf_name_by_offset(btf, var_type->name_off), name))
> continue;
> if (vsi->size != sz)
> continue;
> - if (strcmp(__btf_name_by_offset(btf, var_type->name_off), name))
> - continue;
> - if (off != -ENOENT)
> - /* only one such field is allowed */
> - return -E2BIG;
> - off = vsi->offset;
> if (off % align)
> return -EINVAL;
> +
> + switch (field_type) {
> + case BTF_FIELD_SPIN_LOCK:
> + case BTF_FIELD_TIMER:
> + ret = btf_find_field_struct(btf, var_type, off, sz, info);
> + if (ret < 0)
> + return ret;
> + break;
> + default:
> + return -EFAULT;
> + }
> }
> - return off;
> + return 0;
> }
>
> static int btf_find_field(const struct btf *btf, const struct btf_type *t,
> - const char *name, int sz, int align)
> + int field_type, struct btf_field_info *info)
> {
> + const char *name;
> + int sz, align;
> +
> + switch (field_type) {
> + case BTF_FIELD_SPIN_LOCK:
> + name = "bpf_spin_lock";
> + sz = sizeof(struct bpf_spin_lock);
> + align = __alignof__(struct bpf_spin_lock);
> + break;
> + case BTF_FIELD_TIMER:
> + name = "bpf_timer";
> + sz = sizeof(struct bpf_timer);
> + align = __alignof__(struct bpf_timer);
> + break;
> + default:
> + return -EFAULT;
> + }
>
> if (__btf_type_is_struct(t))
> - return btf_find_struct_field(btf, t, name, sz, align);
> + return btf_find_struct_field(btf, t, name, sz, align, field_type, info);
> else if (btf_type_is_datasec(t))
> - return btf_find_datasec_var(btf, t, name, sz, align);
> + return btf_find_datasec_var(btf, t, name, sz, align, field_type, info);
> return -EINVAL;
> }
>
> @@ -3237,16 +3292,24 @@ static int btf_find_field(const struct btf *btf, const struct btf_type *t,
> */
> int btf_find_spin_lock(const struct btf *btf, const struct btf_type *t)
> {
> - return btf_find_field(btf, t, "bpf_spin_lock",
> - sizeof(struct bpf_spin_lock),
> - __alignof__(struct bpf_spin_lock));
> + struct btf_field_info info = { .off = -ENOENT };
> + int ret;
> +
> + ret = btf_find_field(btf, t, BTF_FIELD_SPIN_LOCK, &info);
I'm confused about why we pass in "struct btf_field_info" as the out
parameter. Maybe I'm missing something here, but why can't
"btf_find_field" just return back the offset?
> + if (ret < 0)
> + return ret;
> + return info.off;
> }
>
> int btf_find_timer(const struct btf *btf, const struct btf_type *t)
> {
> - return btf_find_field(btf, t, "bpf_timer",
> - sizeof(struct bpf_timer),
> - __alignof__(struct bpf_timer));
> + struct btf_field_info info = { .off = -ENOENT };
> + int ret;
> +
> + ret = btf_find_field(btf, t, BTF_FIELD_TIMER, &info);
> + if (ret < 0)
> + return ret;
> + return info.off;
> }
>
> static void __btf_struct_show(const struct btf *btf, const struct btf_type *t,
> --
> 2.35.1
>
