| Message ID | 20220908000254.3079129-2-joannelkoong@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Dynptr convenience helpers | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for bpf-next, async |
| netdev/apply | fail | Patch does not apply to bpf-next |
| bpf/vmtest-bpf-next-PR | fail | merge-conflict |
On Thu, Sep 8, 2022 at 1:10 AM Joanne Koong <joannelkoong@gmail.com> wrote: > > Add a new helper bpf_dynptr_data_rdonly > > void *bpf_dynptr_data_rdonly(struct bpf_dynptr *ptr, u32 offset, u32 len); > > which gets a read-only pointer to the underlying dynptr data. > > This is equivalent to bpf_dynptr_data(), except the pointer returned is > read-only, which allows this to support both read-write and read-only > dynptrs. > > One example where this will be useful is for skb dynptrs where the > program type only allows read-only access to packet data. This API will > provide a way to obtain a data slice that can be used for direct reads. > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > --- > include/uapi/linux/bpf.h | 15 +++++++++++++++ > kernel/bpf/helpers.c | 32 ++++++++++++++++++++++++++------ > kernel/bpf/verifier.c | 7 +++++-- > tools/include/uapi/linux/bpf.h | 15 +++++++++++++++ > 4 files changed, 61 insertions(+), 8 deletions(-) > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index c55c23f25c0f..cce3356765fc 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -5439,6 +5439,20 @@ union bpf_attr { > * *flags* is currently unused, it must be 0 for now. > * Return > * 0 on success, -EINVAL if flags is not 0. > + * > + * void *bpf_dynptr_data_rdonly(struct bpf_dynptr *ptr, u32 offset, u32 len) > + * Description > + * Get a read-only pointer to the underlying dynptr data. > + * > + * This is equivalent to **bpf_dynptr_data**\ () except the > + * pointer returned is read-only, which allows this to support > + * both read-write and read-only dynptrs. For more details on using > + * the API, please refer to **bpf_dynptr_data**\ (). > + * Return > + * Read-only pointer to the underlying dynptr data, NULL if the > + * dynptr is invalid or if the offset and length is out of bounds > + * or in a paged buffer for skb-type dynptrs or across fragments > + * for xdp-type dynptrs. > */ > #define __BPF_FUNC_MAPPER(FN) \ > FN(unspec), \ > @@ -5652,6 +5666,7 @@ union bpf_attr { > FN(ktime_get_tai_ns), \ > FN(dynptr_from_skb), \ > FN(dynptr_from_xdp), \ > + FN(dynptr_data_rdonly), \ > /* */ > > /* integer value in 'imm' field of BPF_CALL instruction selects which helper > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > index befafae34a63..30a59c9e5df3 100644 > --- a/kernel/bpf/helpers.c > +++ b/kernel/bpf/helpers.c > @@ -1572,7 +1572,7 @@ static const struct bpf_func_proto bpf_dynptr_write_proto = { > .arg5_type = ARG_ANYTHING, > }; > > -BPF_CALL_3(bpf_dynptr_data, struct bpf_dynptr_kern *, ptr, u32, offset, u32, len) > +void *__bpf_dynptr_data(struct bpf_dynptr_kern *ptr, u32 offset, u32 len, bool writable) > { > enum bpf_dynptr_type type; > void *data; > @@ -1585,7 +1585,7 @@ BPF_CALL_3(bpf_dynptr_data, struct bpf_dynptr_kern *, ptr, u32, offset, u32, len > if (err) > return 0; Let's return NULL for void* type. > > - if (bpf_dynptr_is_rdonly(ptr)) > + if (writable && bpf_dynptr_is_rdonly(ptr)) > return 0; ditto > > type = bpf_dynptr_get_type(ptr); > @@ -1610,13 +1610,31 @@ BPF_CALL_3(bpf_dynptr_data, struct bpf_dynptr_kern *, ptr, u32, offset, u32, len > /* if the requested data in across fragments, then it cannot > * be accessed directly - bpf_xdp_pointer will return NULL > */ > - return (unsigned long)bpf_xdp_pointer(ptr->data, > - ptr->offset + offset, len); > + return bpf_xdp_pointer(ptr->data, ptr->offset + offset, len); > default: > - WARN_ONCE(true, "bpf_dynptr_data: unknown dynptr type %d\n", type); > + WARN_ONCE(true, "__bpf_dynptr_data: unknown dynptr type %d\n", type); Let's use __func__ so we don't have to change this again. WARN_ONCE(true, "%s: unknown dynptr type %d\n", __func__, type); Thanks, Song [...]
On Fri, Sep 9, 2022 at 8:29 AM Song Liu <song@kernel.org> wrote: > > On Thu, Sep 8, 2022 at 1:10 AM Joanne Koong <joannelkoong@gmail.com> wrote: > > > > Add a new helper bpf_dynptr_data_rdonly > > > > void *bpf_dynptr_data_rdonly(struct bpf_dynptr *ptr, u32 offset, u32 len); > > > > which gets a read-only pointer to the underlying dynptr data. > > > > This is equivalent to bpf_dynptr_data(), except the pointer returned is > > read-only, which allows this to support both read-write and read-only > > dynptrs. > > > > One example where this will be useful is for skb dynptrs where the > > program type only allows read-only access to packet data. This API will > > provide a way to obtain a data slice that can be used for direct reads. > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > > --- > > include/uapi/linux/bpf.h | 15 +++++++++++++++ > > kernel/bpf/helpers.c | 32 ++++++++++++++++++++++++++------ > > kernel/bpf/verifier.c | 7 +++++-- > > tools/include/uapi/linux/bpf.h | 15 +++++++++++++++ > > 4 files changed, 61 insertions(+), 8 deletions(-) > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > index c55c23f25c0f..cce3356765fc 100644 > > --- a/include/uapi/linux/bpf.h > > +++ b/include/uapi/linux/bpf.h > > @@ -5439,6 +5439,20 @@ union bpf_attr { > > * *flags* is currently unused, it must be 0 for now. > > * Return > > * 0 on success, -EINVAL if flags is not 0. > > + * > > + * void *bpf_dynptr_data_rdonly(struct bpf_dynptr *ptr, u32 offset, u32 len) > > + * Description > > + * Get a read-only pointer to the underlying dynptr data. > > + * > > + * This is equivalent to **bpf_dynptr_data**\ () except the > > + * pointer returned is read-only, which allows this to support > > + * both read-write and read-only dynptrs. For more details on using > > + * the API, please refer to **bpf_dynptr_data**\ (). > > + * Return > > + * Read-only pointer to the underlying dynptr data, NULL if the > > + * dynptr is invalid or if the offset and length is out of bounds > > + * or in a paged buffer for skb-type dynptrs or across fragments > > + * for xdp-type dynptrs. > > */ > > #define __BPF_FUNC_MAPPER(FN) \ > > FN(unspec), \ > > @@ -5652,6 +5666,7 @@ union bpf_attr { > > FN(ktime_get_tai_ns), \ > > FN(dynptr_from_skb), \ > > FN(dynptr_from_xdp), \ > > + FN(dynptr_data_rdonly), \ > > /* */ > > > > /* integer value in 'imm' field of BPF_CALL instruction selects which helper > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > > index befafae34a63..30a59c9e5df3 100644 > > --- a/kernel/bpf/helpers.c > > +++ b/kernel/bpf/helpers.c > > @@ -1572,7 +1572,7 @@ static const struct bpf_func_proto bpf_dynptr_write_proto = { > > .arg5_type = ARG_ANYTHING, > > }; > > > > -BPF_CALL_3(bpf_dynptr_data, struct bpf_dynptr_kern *, ptr, u32, offset, u32, len) > > +void *__bpf_dynptr_data(struct bpf_dynptr_kern *ptr, u32 offset, u32 len, bool writable) > > { > > enum bpf_dynptr_type type; > > void *data; > > @@ -1585,7 +1585,7 @@ BPF_CALL_3(bpf_dynptr_data, struct bpf_dynptr_kern *, ptr, u32, offset, u32, len > > if (err) > > return 0; > > Let's return NULL for void* type. > > > > > - if (bpf_dynptr_is_rdonly(ptr)) > > + if (writable && bpf_dynptr_is_rdonly(ptr)) > > return 0; > ditto > > > > type = bpf_dynptr_get_type(ptr); > > @@ -1610,13 +1610,31 @@ BPF_CALL_3(bpf_dynptr_data, struct bpf_dynptr_kern *, ptr, u32, offset, u32, len > > /* if the requested data in across fragments, then it cannot > > * be accessed directly - bpf_xdp_pointer will return NULL > > */ > > - return (unsigned long)bpf_xdp_pointer(ptr->data, > > - ptr->offset + offset, len); > > + return bpf_xdp_pointer(ptr->data, ptr->offset + offset, len); > > default: > > - WARN_ONCE(true, "bpf_dynptr_data: unknown dynptr type %d\n", type); > > + WARN_ONCE(true, "__bpf_dynptr_data: unknown dynptr type %d\n", type); > > Let's use __func__ so we don't have to change this again. > > WARN_ONCE(true, "%s: unknown dynptr type %d\n", __func__, type); WARN includes file and line automatically. Do we really need func here too?
On Wed, 7 Sep 2022 17:02:47 -0700 Joanne Koong <joannelkoong@gmail.com> wrote: > Add a new helper bpf_dynptr_data_rdonly > > void *bpf_dynptr_data_rdonly(struct bpf_dynptr *ptr, u32 offset, u32 len); > > which gets a read-only pointer to the underlying dynptr data. > > This is equivalent to bpf_dynptr_data(), except the pointer returned is > read-only, which allows this to support both read-write and read-only > dynptrs. > > One example where this will be useful is for skb dynptrs where the > program type only allows read-only access to packet data. This API will > provide a way to obtain a data slice that can be used for direct reads. > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> Reviewed-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
On Fri, Sep 9, 2022 at 4:32 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Fri, Sep 9, 2022 at 8:29 AM Song Liu <song@kernel.org> wrote: > > > > On Thu, Sep 8, 2022 at 1:10 AM Joanne Koong <joannelkoong@gmail.com> wrote: > > > [...] > > > > Let's return NULL for void* type. > > > > > > > > - if (bpf_dynptr_is_rdonly(ptr)) > > > + if (writable && bpf_dynptr_is_rdonly(ptr)) > > > return 0; > > ditto > > > > > > type = bpf_dynptr_get_type(ptr); > > > @@ -1610,13 +1610,31 @@ BPF_CALL_3(bpf_dynptr_data, struct bpf_dynptr_kern *, ptr, u32, offset, u32, len > > > /* if the requested data in across fragments, then it cannot > > > * be accessed directly - bpf_xdp_pointer will return NULL > > > */ > > > - return (unsigned long)bpf_xdp_pointer(ptr->data, > > > - ptr->offset + offset, len); > > > + return bpf_xdp_pointer(ptr->data, ptr->offset + offset, len); > > > default: > > > - WARN_ONCE(true, "bpf_dynptr_data: unknown dynptr type %d\n", type); > > > + WARN_ONCE(true, "__bpf_dynptr_data: unknown dynptr type %d\n", type); > > > > Let's use __func__ so we don't have to change this again. > > > > WARN_ONCE(true, "%s: unknown dynptr type %d\n", __func__, type); > > WARN includes file and line automatically. > Do we really need func here too? I think func is helpful when I read a slightly different version of the code and line number changes. (Well, maybe I shouldn't do this.) I won't mind if we remove it. We just shouldn't hard code it manually. Thanks, Song
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index c55c23f25c0f..cce3356765fc 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -5439,6 +5439,20 @@ union bpf_attr { * *flags* is currently unused, it must be 0 for now. * Return * 0 on success, -EINVAL if flags is not 0. + * + * void *bpf_dynptr_data_rdonly(struct bpf_dynptr *ptr, u32 offset, u32 len) + * Description + * Get a read-only pointer to the underlying dynptr data. + * + * This is equivalent to **bpf_dynptr_data**\ () except the + * pointer returned is read-only, which allows this to support + * both read-write and read-only dynptrs. For more details on using + * the API, please refer to **bpf_dynptr_data**\ (). + * Return + * Read-only pointer to the underlying dynptr data, NULL if the + * dynptr is invalid or if the offset and length is out of bounds + * or in a paged buffer for skb-type dynptrs or across fragments + * for xdp-type dynptrs. */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -5652,6 +5666,7 @@ union bpf_attr { FN(ktime_get_tai_ns), \ FN(dynptr_from_skb), \ FN(dynptr_from_xdp), \ + FN(dynptr_data_rdonly), \ /* */ /* integer value in 'imm' field of BPF_CALL instruction selects which helper diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index befafae34a63..30a59c9e5df3 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -1572,7 +1572,7 @@ static const struct bpf_func_proto bpf_dynptr_write_proto = { .arg5_type = ARG_ANYTHING, }; -BPF_CALL_3(bpf_dynptr_data, struct bpf_dynptr_kern *, ptr, u32, offset, u32, len) +void *__bpf_dynptr_data(struct bpf_dynptr_kern *ptr, u32 offset, u32 len, bool writable) { enum bpf_dynptr_type type; void *data; @@ -1585,7 +1585,7 @@ BPF_CALL_3(bpf_dynptr_data, struct bpf_dynptr_kern *, ptr, u32, offset, u32, len if (err) return 0; - if (bpf_dynptr_is_rdonly(ptr)) + if (writable && bpf_dynptr_is_rdonly(ptr)) return 0; type = bpf_dynptr_get_type(ptr); @@ -1610,13 +1610,31 @@ BPF_CALL_3(bpf_dynptr_data, struct bpf_dynptr_kern *, ptr, u32, offset, u32, len /* if the requested data in across fragments, then it cannot * be accessed directly - bpf_xdp_pointer will return NULL */ - return (unsigned long)bpf_xdp_pointer(ptr->data, - ptr->offset + offset, len); + return bpf_xdp_pointer(ptr->data, ptr->offset + offset, len); default: - WARN_ONCE(true, "bpf_dynptr_data: unknown dynptr type %d\n", type); + WARN_ONCE(true, "__bpf_dynptr_data: unknown dynptr type %d\n", type); return 0; } - return (unsigned long)(data + ptr->offset + offset); + return data + ptr->offset + offset; +} + +BPF_CALL_3(bpf_dynptr_data_rdonly, struct bpf_dynptr_kern *, ptr, u32, offset, u32, len) +{ + return (unsigned long)__bpf_dynptr_data(ptr, offset, len, false); +} + +static const struct bpf_func_proto bpf_dynptr_data_rdonly_proto = { + .func = bpf_dynptr_data_rdonly, + .gpl_only = false, + .ret_type = RET_PTR_TO_DYNPTR_MEM_OR_NULL | MEM_RDONLY, + .arg1_type = ARG_PTR_TO_DYNPTR, + .arg2_type = ARG_ANYTHING, + .arg3_type = ARG_CONST_ALLOC_SIZE_OR_ZERO, +}; + +BPF_CALL_3(bpf_dynptr_data, struct bpf_dynptr_kern *, ptr, u32, offset, u32, len) +{ + return (unsigned long)__bpf_dynptr_data(ptr, offset, len, true); } static const struct bpf_func_proto bpf_dynptr_data_proto = { @@ -1698,6 +1716,8 @@ bpf_base_func_proto(enum bpf_func_id func_id) return &bpf_dynptr_write_proto; case BPF_FUNC_dynptr_data: return &bpf_dynptr_data_proto; + case BPF_FUNC_dynptr_data_rdonly: + return &bpf_dynptr_data_rdonly_proto; default: break; } diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index b1f66a1cc690..c312d931359d 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -506,7 +506,8 @@ static bool is_ptr_cast_function(enum bpf_func_id func_id) static bool is_dynptr_ref_function(enum bpf_func_id func_id) { - return func_id == BPF_FUNC_dynptr_data; + return func_id == BPF_FUNC_dynptr_data || + func_id == BPF_FUNC_dynptr_data_rdonly; } static bool helper_multiple_ref_obj_use(enum bpf_func_id func_id, @@ -7398,6 +7399,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn } break; case BPF_FUNC_dynptr_data: + case BPF_FUNC_dynptr_data_rdonly: { struct bpf_reg_state *reg; @@ -7495,7 +7497,8 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn mark_reg_known_zero(env, regs, BPF_REG_0); regs[BPF_REG_0].type = PTR_TO_MEM | ret_flag; regs[BPF_REG_0].mem_size = meta.mem_size; - if (func_id == BPF_FUNC_dynptr_data) { + if (func_id == BPF_FUNC_dynptr_data || + func_id == BPF_FUNC_dynptr_data_rdonly) { if (dynptr_type == BPF_DYNPTR_TYPE_SKB) regs[BPF_REG_0].type |= DYNPTR_TYPE_SKB; else if (dynptr_type == BPF_DYNPTR_TYPE_XDP) diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index c55c23f25c0f..cce3356765fc 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -5439,6 +5439,20 @@ union bpf_attr { * *flags* is currently unused, it must be 0 for now. * Return * 0 on success, -EINVAL if flags is not 0. + * + * void *bpf_dynptr_data_rdonly(struct bpf_dynptr *ptr, u32 offset, u32 len) + * Description + * Get a read-only pointer to the underlying dynptr data. + * + * This is equivalent to **bpf_dynptr_data**\ () except the + * pointer returned is read-only, which allows this to support + * both read-write and read-only dynptrs. For more details on using + * the API, please refer to **bpf_dynptr_data**\ (). + * Return + * Read-only pointer to the underlying dynptr data, NULL if the + * dynptr is invalid or if the offset and length is out of bounds + * or in a paged buffer for skb-type dynptrs or across fragments + * for xdp-type dynptrs. */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -5652,6 +5666,7 @@ union bpf_attr { FN(ktime_get_tai_ns), \ FN(dynptr_from_skb), \ FN(dynptr_from_xdp), \ + FN(dynptr_data_rdonly), \ /* */ /* integer value in 'imm' field of BPF_CALL instruction selects which helper
Add a new helper bpf_dynptr_data_rdonly void *bpf_dynptr_data_rdonly(struct bpf_dynptr *ptr, u32 offset, u32 len); which gets a read-only pointer to the underlying dynptr data. This is equivalent to bpf_dynptr_data(), except the pointer returned is read-only, which allows this to support both read-write and read-only dynptrs. One example where this will be useful is for skb dynptrs where the program type only allows read-only access to packet data. This API will provide a way to obtain a data slice that can be used for direct reads. Signed-off-by: Joanne Koong <joannelkoong@gmail.com> --- include/uapi/linux/bpf.h | 15 +++++++++++++++ kernel/bpf/helpers.c | 32 ++++++++++++++++++++++++++------ kernel/bpf/verifier.c | 7 +++++-- tools/include/uapi/linux/bpf.h | 15 +++++++++++++++ 4 files changed, 61 insertions(+), 8 deletions(-)