On Thu, Mar 17, 2022 at 05:29:44PM +0530, Kumar Kartikeya Dwivedi wrote:
> Next commit's field type 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 BTF ID pointers in map value, by taking a 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. This is important, because we'll be
> potentially reallocating memory inside this function in next commit, so
> being able to do that when everything else is in order is going to be
> more convenient.
>
> The name parameter is now optional, and only checked if it is not NULL.
>
> The size must be checked in the function, because in case of PTR it will
> instead point to the underlying BTF ID it is pointing to (or modifiers),
> so the check becomes wrong to do outside of function, and the base type
> has to be obtained by removing modifiers.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> ---
> kernel/bpf/btf.c | 120 +++++++++++++++++++++++++++++++++--------------
> 1 file changed, 86 insertions(+), 34 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 17b9adcd88d3..5b2824332880 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3161,71 +3161,109 @@ 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;
> + if (info->off != -ENOENT)
> + /* only one such field is allowed */
> + return -E2BIG;
> + 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,
> + 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);
> +
> + if (name && strcmp(__btf_name_by_offset(btf, member_type->name_off), name))
> + 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:
Since spin_lock vs timer is passed into btf_find_struct_field() as field_type
argument there is no need to pass name, sz, align from the caller.
Pls make btf_find_spin_lock() to pass BTF_FIELD_SPIN_LOCK only
and in the above code do something like:
switch (field_type) {
case BTF_FIELD_SPIN_LOCK:
name = "bpf_spin_lock";
sz = ...
break;
case BTF_FIELD_TIMER:
name = "bpf_timer";
sz = ...
break;
}
switch (field_type) {
case BTF_FIELD_SPIN_LOCK:
case BTF_FIELD_TIMER:
if (!__btf_type_is_struct(member_type))
continue;
if (strcmp(__btf_name_by_offset(btf, member_type->name_off), name))
...
btf_find_field_struct(btf, member_type, off, sz, info);
}
It will cleanup the later patch which passes NULL, sizeof(u64), alignof(u64)
only to pass something into the function.
With above suggestion it wouldn't need to pass dummy args. BTF_FIELD_KPTR will be enough.
> + ret = btf_find_field_struct(btf, member_type, off, sz, info);
> + 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)
> {
> 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)
> + const char *name, int sz, int align, int field_type,
> + struct btf_field_info *info)
> {
> -
> 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;
> }
>
> @@ -3235,16 +3273,30 @@ 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, "bpf_spin_lock",
> + sizeof(struct bpf_spin_lock),
> + __alignof__(struct bpf_spin_lock),
> + BTF_FIELD_SPIN_LOCK, &info);
> + 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, "bpf_timer",
> + sizeof(struct bpf_timer),
> + __alignof__(struct bpf_timer),
> + 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
>
--
