| Message ID | 20220911122328.306188-2-shmulik.ladkani@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Support setting variable-length tunnel options | expand |
On 9/11/22 5:23 AM, Shmulik Ladkani wrote: > This allows kernel code dealing with dynptrs obtain dynptr's available > size and current (w. proper offset) data pointer. > > Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com> Acked-by: Yonghong Song <yhs@fb.com>
Hi, On 9/11/2022 8:23 PM, Shmulik Ladkani wrote: > This allows kernel code dealing with dynptrs obtain dynptr's available > size and current (w. proper offset) data pointer. > > Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com> SNIP > + > +static inline void *bpf_dynptr_get_data(struct bpf_dynptr_kern *ptr) > +{ > + return ptr->data ? ptr->data + ptr->offset : NULL; > +} Have one dummy question here. Is ptr->data == NULL is possible ? According to the function prototype of bpf_dynptr_from_mem(), data can not be NULL. And IMO in order to simplify the usage of bpf_dynptr_kernel, we need to ensure ptr->data should be not NULL, else will need to add a NULL checking for every access of bpf_dynptr_kernel in kernel. > > #ifdef CONFIG_BPF_LSM > void bpf_cgroup_atype_get(u32 attach_btf_id, int cgroup_atype); > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > index fc08035f14ed..824864ac82d1 100644 > --- a/kernel/bpf/helpers.c > +++ b/kernel/bpf/helpers.c > @@ -1408,7 +1408,7 @@ static void bpf_dynptr_set_type(struct bpf_dynptr_kern *ptr, enum bpf_dynptr_typ > ptr->size |= type << DYNPTR_TYPE_SHIFT; > } > > -static u32 bpf_dynptr_get_size(struct bpf_dynptr_kern *ptr) > +u32 bpf_dynptr_get_size(struct bpf_dynptr_kern *ptr) > { > return ptr->size & DYNPTR_SIZE_MASK; > }
On Wed, 21 Sept 2022 at 10:46, Hou Tao <houtao1@huawei.com> wrote: > > Hi, > > On 9/11/2022 8:23 PM, Shmulik Ladkani wrote: > > This allows kernel code dealing with dynptrs obtain dynptr's available > > size and current (w. proper offset) data pointer. > > > > Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com> > SNIP > > + > > +static inline void *bpf_dynptr_get_data(struct bpf_dynptr_kern *ptr) > > +{ > > + return ptr->data ? ptr->data + ptr->offset : NULL; > > +} > Have one dummy question here. Is ptr->data == NULL is possible ? According to > the function prototype of bpf_dynptr_from_mem(), data can not be NULL. And IMO > in order to simplify the usage of bpf_dynptr_kernel, we need to ensure ptr->data > should be not NULL, else will need to add a NULL checking for every access of > bpf_dynptr_kernel in kernel. Yes, ptr->data can always be NULL. And I think at this point if you're accepting dynptr from BPF programs, the ship has sailed on ensuring it is always non-NULL (and honestly I don't know if there's a huge advantage to it vs the amount of verifier work that would be needed to enable this). For an example, see how bpf_ringbuf_reserve_dynptr sets it to NULL on error. Verifier still tracks it as valid initialized dynptr, but later operations will simply fail or return without doing anything at runtime.
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 48ae05099f36..a2f16e3cb0fa 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -2631,6 +2631,19 @@ void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data, enum bpf_dynptr_type type, u32 offset, u32 size); void bpf_dynptr_set_null(struct bpf_dynptr_kern *ptr); int bpf_dynptr_check_size(u32 size); +#ifdef CONFIG_BPF_SYSCALL +u32 bpf_dynptr_get_size(struct bpf_dynptr_kern *ptr); +#else +static inline u32 bpf_dynptr_get_size(struct bpf_dynptr_kern *ptr) +{ + return 0; +} +#endif + +static inline void *bpf_dynptr_get_data(struct bpf_dynptr_kern *ptr) +{ + return ptr->data ? ptr->data + ptr->offset : NULL; +} #ifdef CONFIG_BPF_LSM void bpf_cgroup_atype_get(u32 attach_btf_id, int cgroup_atype); diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index fc08035f14ed..824864ac82d1 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -1408,7 +1408,7 @@ static void bpf_dynptr_set_type(struct bpf_dynptr_kern *ptr, enum bpf_dynptr_typ ptr->size |= type << DYNPTR_TYPE_SHIFT; } -static u32 bpf_dynptr_get_size(struct bpf_dynptr_kern *ptr) +u32 bpf_dynptr_get_size(struct bpf_dynptr_kern *ptr) { return ptr->size & DYNPTR_SIZE_MASK; }
This allows kernel code dealing with dynptrs obtain dynptr's available size and current (w. proper offset) data pointer. Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com> --- v5: - fix bpf_dynptr_get_data's incorrect usage of bpf_dynptr_kern's size spotted by Joanne Koong <joannelkoong@gmail.com> v6: - Simplify bpf_dynptr_get_data's interface and make it inline suggested by John Fastabend <john.fastabend@gmail.com> v7: - Fix undefined reference to `bpf_dynptr_get_size' when CONFIG_BPF_SYSCALL is unset, Reported-by: kernel test robot <lkp@intel.com> --- include/linux/bpf.h | 13 +++++++++++++ kernel/bpf/helpers.c | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-)