| Message ID | 20220317115957.3193097-3-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Introduce typed pointer support in BPF maps | expand |
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@gmail.com> > --- > 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 > --
On Sat, Mar 19, 2022 at 11:25:34PM IST, Alexei Starovoitov wrote: > 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@gmail.com> > > --- > > 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; > } Would doing this in btf_find_field be better? Then we set these once instead of doing it twice in btf_find_struct_field, and btf_find_datasec_var. > 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 > > > > -- -- Kartikeya
On Sun, Mar 20, 2022 at 01:01:16AM IST, Kumar Kartikeya Dwivedi wrote: > On Sat, Mar 19, 2022 at 11:25:34PM IST, Alexei Starovoitov wrote: > > 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@gmail.com> > > > --- > > > 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; > > } > > Would doing this in btf_find_field be better? Then we set these once instead of > doing it twice in btf_find_struct_field, and btf_find_datasec_var. > > > 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. > > Just to be clear, for the kptr case we still use size and align, only name is optional. size is used for datasec_var call, align is used in both struct_field and datasec_var. So I'm not sure whether moving it around has much effect, instead of the caller it will now be set based on field_type inside btf_find_field. > > > + 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 > > > > > > > -- > > -- > Kartikeya -- Kartikeya
On Sun, Mar 20, 2022 at 01:36:41AM +0530, Kumar Kartikeya Dwivedi wrote: > > > > 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; > > > } > > > > Would doing this in btf_find_field be better? Then we set these once instead of > > doing it twice in btf_find_struct_field, and btf_find_datasec_var. yeah. probably. > > > > > 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. > > > > > Just to be clear, for the kptr case we still use size and align, only name is > optional. size is used for datasec_var call, align is used in both struct_field > and datasec_var. So I'm not sure whether moving it around has much effect, > instead of the caller it will now be set based on field_type inside > btf_find_field. There is no use case to do BTF_FIELD_KPTR, sizeof(u64) and BTF_FIELD_KPTR, sizeof(u32), right? So best to avoid such mistakes. In other words consider every function to be a uapi. Not in a way that it can never change, but from pov that you wouldn't want the user space to specify all details for the kernel when BTF_FIELD_KPTR is enough to figure out the rest.
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: + 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,
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@gmail.com> --- kernel/bpf/btf.c | 120 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 86 insertions(+), 34 deletions(-)