| Message ID | 20220818152929.402605-3-roberto.sassu@huaweicloud.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | bpf: Add kfuncs for PKCS#7 signature verification | expand |
On Thu, Aug 18, 2022 at 05:29:21PM +0200, roberto.sassu@huaweicloud.com wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > Allow the bpf_dynptr_kern parameter to be specified in kfuncs. Also, ensure > that the dynamic pointer is valid and initialized. > > To properly detect whether a parameter is of the desired type, introduce > the stringify_struct() macro to compare the returned structure name with > the desired name. In addition, protect against structure renames, by > halting the build with BUILD_BUG_ON(), so that developers have to revisit > the code. > > Cc: Joanne Koong <joannelkoong@gmail.com> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > include/linux/bpf_verifier.h | 3 +++ > include/linux/btf.h | 9 +++++++++ > kernel/bpf/btf.c | 18 ++++++++++++++++++ > kernel/bpf/verifier.c | 4 ++-- > 4 files changed, 32 insertions(+), 2 deletions(-) > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > index 2e3bad8640dc..55876fbdbae2 100644 > --- a/include/linux/bpf_verifier.h > +++ b/include/linux/bpf_verifier.h > @@ -560,6 +560,9 @@ int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state > u32 regno); > int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg, > u32 regno, u32 mem_size); > +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, > + struct bpf_reg_state *reg, > + enum bpf_arg_type arg_type); > > /* this lives here instead of in bpf.h because it needs to dereference tgt_prog */ > static inline u64 bpf_trampoline_compute_key(const struct bpf_prog *tgt_prog, > diff --git a/include/linux/btf.h b/include/linux/btf.h > index ad93c2d9cc1c..f546d368ac5d 100644 > --- a/include/linux/btf.h > +++ b/include/linux/btf.h > @@ -52,6 +52,15 @@ > #define KF_SLEEPABLE (1 << 5) /* kfunc may sleep */ > #define KF_DESTRUCTIVE (1 << 6) /* kfunc performs destructive actions */ > > +/* > + * Return the name of the passed struct, if exists, or halt the build if for > + * example the structure gets renamed. In this way, developers have to revisit > + * the code using that structure name, and update it accordingly. > + */ > +#define stringify_struct(x) \ > + ({ BUILD_BUG_ON(sizeof(struct x) < 0); \ > + __stringify(x); }) > + > struct btf; > struct btf_member; > struct btf_type; > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index e49b3b6d48ad..26cb548420af 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -6362,15 +6362,20 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > if (is_kfunc) { > bool arg_mem_size = i + 1 < nargs && is_kfunc_arg_mem_size(btf, &args[i + 1], ®s[regno + 1]); > + bool arg_dynptr = btf_type_is_struct(ref_t) && > + !strcmp(ref_tname, > + stringify_struct(bpf_dynptr_kern)); > > /* Permit pointer to mem, but only when argument > * type is pointer to scalar, or struct composed > * (recursively) of scalars. > * When arg_mem_size is true, the pointer can be > * void *. > + * Also permit initialized dynamic pointers. > */ > if (!btf_type_is_scalar(ref_t) && > !__btf_type_is_scalar_struct(log, btf, ref_t, 0) && > + !arg_dynptr && > (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) { > bpf_log(log, > "arg#%d pointer type %s %s must point to %sscalar, or struct with scalar\n", > @@ -6378,6 +6383,19 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > return -EINVAL; > } > > + if (arg_dynptr) { > + if (!is_dynptr_reg_valid_init(env, reg, > + ARG_PTR_TO_DYNPTR)) { > + bpf_log(log, > + "arg#%d pointer type %s %s must be initialized\n", > + i, btf_type_str(ref_t), > + ref_tname); > + return -EINVAL; > + } > + > + continue; > + } > + > /* Check for mem, len pair */ > if (arg_mem_size) { > if (check_kfunc_mem_size_reg(env, ®s[regno + 1], regno + 1)) { > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 2c1f8069f7b7..aa834e7bb296 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -779,8 +779,8 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_ > return true; > } > > -static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg, > - enum bpf_arg_type arg_type) > +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg, > + enum bpf_arg_type arg_type) > { > struct bpf_func_state *state = func(env, reg); > int spi = get_spi(reg->off); > -- > 2.25.1 > Might be niticking but generally I'd consider splitting exports as commits of their own. BR, Jarko
On Thu, Aug 25, 2022 at 9:54 PM Jarkko Sakkinen <jarkko@kernel.org> wrote: > > > > -static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg, > > - enum bpf_arg_type arg_type) > > +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg, > > + enum bpf_arg_type arg_type) > > { > > struct bpf_func_state *state = func(env, reg); > > int spi = get_spi(reg->off); > > -- > > 2.25.1 > > > > Might be niticking but generally I'd consider splitting > exports as commits of their own. -static bool +bool into a separate commit? I guess it makes sense for people whose salary depends on number of commits. We don't play these games.
On Thu, Aug 25, 2022 at 10:16:14PM -0700, Alexei Starovoitov wrote: > On Thu, Aug 25, 2022 at 9:54 PM Jarkko Sakkinen <jarkko@kernel.org> wrote: > > > > > > -static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg, > > > - enum bpf_arg_type arg_type) > > > +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg, > > > + enum bpf_arg_type arg_type) > > > { > > > struct bpf_func_state *state = func(env, reg); > > > int spi = get_spi(reg->off); > > > -- > > > 2.25.1 > > > > > > > Might be niticking but generally I'd consider splitting > > exports as commits of their own. > > -static bool > +bool > > into a separate commit? > > I guess it makes sense for people whose salary depends on > number of commits. > We don't play these games. What kind of argument is that anyway. BR, Jarkko
On Fri, Aug 26, 2022 at 08:46:14AM +0300, Jarkko Sakkinen wrote: > On Thu, Aug 25, 2022 at 10:16:14PM -0700, Alexei Starovoitov wrote: > > On Thu, Aug 25, 2022 at 9:54 PM Jarkko Sakkinen <jarkko@kernel.org> wrote: > > > > > > > > -static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg, > > > > - enum bpf_arg_type arg_type) > > > > +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg, > > > > + enum bpf_arg_type arg_type) > > > > { > > > > struct bpf_func_state *state = func(env, reg); > > > > int spi = get_spi(reg->off); > > > > -- > > > > 2.25.1 > > > > > > > > > > Might be niticking but generally I'd consider splitting > > > exports as commits of their own. > > > > -static bool > > +bool > > > > into a separate commit? > > > > I guess it makes sense for people whose salary depends on > > number of commits. > > We don't play these games. > > What kind of argument is that anyway. "Separate each *logical change* into a separate patch." [*] To add, generally any user space visible space should be an isolated patch. Please, stop posting nonsense. [*] https://www.kernel.org/doc/html/v5.19/process/submitting-patches.html#separate-your-changes BR, Jarkko
On Fri, Aug 26, 2022 at 05:43:50PM +0300, Jarkko Sakkinen wrote: > On Fri, Aug 26, 2022 at 08:46:14AM +0300, Jarkko Sakkinen wrote: > > On Thu, Aug 25, 2022 at 10:16:14PM -0700, Alexei Starovoitov wrote: > > > On Thu, Aug 25, 2022 at 9:54 PM Jarkko Sakkinen <jarkko@kernel.org> wrote: > > > > > > > > > > -static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg, > > > > > - enum bpf_arg_type arg_type) > > > > > +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg, > > > > > + enum bpf_arg_type arg_type) > > > > > { > > > > > struct bpf_func_state *state = func(env, reg); > > > > > int spi = get_spi(reg->off); > > > > > -- > > > > > 2.25.1 > > > > > > > > > > > > > Might be niticking but generally I'd consider splitting > > > > exports as commits of their own. > > > > > > -static bool > > > +bool > > > > > > into a separate commit? > > > > > > I guess it makes sense for people whose salary depends on > > > number of commits. > > > We don't play these games. > > > > What kind of argument is that anyway. > > "Separate each *logical change* into a separate patch." [*] > > To add, generally any user space visible space should be an ~~~~~ change BR, Jarkko
On Fri, 2022-08-26 at 17:43 +0300, Jarkko Sakkinen wrote: > On Fri, Aug 26, 2022 at 08:46:14AM +0300, Jarkko Sakkinen wrote: > > On Thu, Aug 25, 2022 at 10:16:14PM -0700, Alexei Starovoitov wrote: > > > On Thu, Aug 25, 2022 at 9:54 PM Jarkko Sakkinen < > > > jarkko@kernel.org> wrote: > > > > > -static bool is_dynptr_reg_valid_init(struct bpf_verifier_env > > > > > *env, struct bpf_reg_state *reg, > > > > > - enum bpf_arg_type > > > > > arg_type) > > > > > +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, > > > > > struct bpf_reg_state *reg, > > > > > + enum bpf_arg_type arg_type) > > > > > { > > > > > struct bpf_func_state *state = func(env, reg); > > > > > int spi = get_spi(reg->off); > > > > > -- > > > > > 2.25.1 > > > > > > > > > > > > > Might be niticking but generally I'd consider splitting > > > > exports as commits of their own. > > > > > > -static bool > > > +bool > > > > > > into a separate commit? > > > > > > I guess it makes sense for people whose salary depends on > > > number of commits. > > > We don't play these games. > > > > What kind of argument is that anyway. > > "Separate each *logical change* into a separate patch." [*] The logical change, as per the patch subject, is allowing the possibility of including eBPF dynamic pointers in a kfunc definition. It requires to call an existing function that was already defined elsewhere. Maybe I'm wrong, but I don't see only exporting a function definition to an include file as a logical change. To me, the changes in this patch are clearly connected. Or even better, they tell why the function definition has been exported, that would not appear if moving the function definition is a standalone patch. > > To add, generally any user space visible space should be an > isolated patch. As far as I understood, definitions visible to user space should be in include/uapi. > > Please, stop posting nonsense. If I may, saying this does not encourage people to try to submit their code. I feel it is a bit strong, and I kindly ask you to express your opinion in a more gentle way. Thanks Roberto
On Fri, Aug 26, 2022 at 05:34:57PM +0200, Roberto Sassu wrote: > On Fri, 2022-08-26 at 17:43 +0300, Jarkko Sakkinen wrote: > > On Fri, Aug 26, 2022 at 08:46:14AM +0300, Jarkko Sakkinen wrote: > > > On Thu, Aug 25, 2022 at 10:16:14PM -0700, Alexei Starovoitov wrote: > > > > On Thu, Aug 25, 2022 at 9:54 PM Jarkko Sakkinen < > > > > jarkko@kernel.org> wrote: > > > > > > -static bool is_dynptr_reg_valid_init(struct bpf_verifier_env > > > > > > *env, struct bpf_reg_state *reg, > > > > > > - enum bpf_arg_type > > > > > > arg_type) > > > > > > +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, > > > > > > struct bpf_reg_state *reg, > > > > > > + enum bpf_arg_type arg_type) > > > > > > { > > > > > > struct bpf_func_state *state = func(env, reg); > > > > > > int spi = get_spi(reg->off); > > > > > > -- > > > > > > 2.25.1 > > > > > > > > > > > > > > > > Might be niticking but generally I'd consider splitting > > > > > exports as commits of their own. > > > > > > > > -static bool > > > > +bool > > > > > > > > into a separate commit? > > > > > > > > I guess it makes sense for people whose salary depends on > > > > number of commits. > > > > We don't play these games. > > > > > > What kind of argument is that anyway. > > > > "Separate each *logical change* into a separate patch." [*] > > The logical change, as per the patch subject, is allowing the > possibility of including eBPF dynamic pointers in a kfunc definition. > It requires to call an existing function that was already defined > elsewhere. > > Maybe I'm wrong, but I don't see only exporting a function definition > to an include file as a logical change. To me, the changes in this > patch are clearly connected. Or even better, they tell why the function > definition has been exported, that would not appear if moving the > function definition is a standalone patch. > > > > > To add, generally any user space visible space should be an > > isolated patch. > > As far as I understood, definitions visible to user space should be in > include/uapi. It does change e.g. the output of kallsyms. It's not ABI but it's still user space visble. > > > > > Please, stop posting nonsense. > > If I may, saying this does not encourage people to try to submit their > code. I feel it is a bit strong, and I kindly ask you to express your > opinion in a more gentle way. I agree. That's why I was wondering what is this nonsense about salary and games. BR, Jarkko
On Fri, Aug 26, 2022 at 07:32:54PM +0300, Jarkko Sakkinen wrote: > On Fri, Aug 26, 2022 at 05:34:57PM +0200, Roberto Sassu wrote: > > On Fri, 2022-08-26 at 17:43 +0300, Jarkko Sakkinen wrote: > > > On Fri, Aug 26, 2022 at 08:46:14AM +0300, Jarkko Sakkinen wrote: > > > > On Thu, Aug 25, 2022 at 10:16:14PM -0700, Alexei Starovoitov wrote: > > > > > On Thu, Aug 25, 2022 at 9:54 PM Jarkko Sakkinen < > > > > > jarkko@kernel.org> wrote: > > > > > > > -static bool is_dynptr_reg_valid_init(struct bpf_verifier_env > > > > > > > *env, struct bpf_reg_state *reg, > > > > > > > - enum bpf_arg_type > > > > > > > arg_type) > > > > > > > +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, > > > > > > > struct bpf_reg_state *reg, > > > > > > > + enum bpf_arg_type arg_type) > > > > > > > { > > > > > > > struct bpf_func_state *state = func(env, reg); > > > > > > > int spi = get_spi(reg->off); > > > > > > > -- > > > > > > > 2.25.1 > > > > > > > > > > > > > > > > > > > Might be niticking but generally I'd consider splitting > > > > > > exports as commits of their own. > > > > > > > > > > -static bool > > > > > +bool > > > > > > > > > > into a separate commit? > > > > > > > > > > I guess it makes sense for people whose salary depends on > > > > > number of commits. > > > > > We don't play these games. > > > > > > > > What kind of argument is that anyway. > > > > > > "Separate each *logical change* into a separate patch." [*] > > > > The logical change, as per the patch subject, is allowing the > > possibility of including eBPF dynamic pointers in a kfunc definition. > > It requires to call an existing function that was already defined > > elsewhere. > > > > Maybe I'm wrong, but I don't see only exporting a function definition > > to an include file as a logical change. To me, the changes in this > > patch are clearly connected. Or even better, they tell why the function > > definition has been exported, that would not appear if moving the > > function definition is a standalone patch. > > > > > > > > To add, generally any user space visible space should be an > > > isolated patch. > > > > As far as I understood, definitions visible to user space should be in > > include/uapi. > > It does change e.g. the output of kallsyms. > > It's not ABI but it's still user space visble. > > > > > > > > > Please, stop posting nonsense. > > > > If I may, saying this does not encourage people to try to submit their > > code. I feel it is a bit strong, and I kindly ask you to express your > > opinion in a more gentle way. > > I agree. That's why I was wondering what is this nonsense > about salary and games. Please denote that I started my review with "Might be nitpicking...". It's neither particularly disencouraging nor enforcing for anyone. The blast that came after that, on the other hand, IMHO meets exactly those standards. BR, Jarkko
On 8/26/2022 6:41 PM, Jarkko Sakkinen wrote: > On Fri, Aug 26, 2022 at 07:32:54PM +0300, Jarkko Sakkinen wrote: >> On Fri, Aug 26, 2022 at 05:34:57PM +0200, Roberto Sassu wrote: >>> On Fri, 2022-08-26 at 17:43 +0300, Jarkko Sakkinen wrote: >>>> On Fri, Aug 26, 2022 at 08:46:14AM +0300, Jarkko Sakkinen wrote: >>>>> On Thu, Aug 25, 2022 at 10:16:14PM -0700, Alexei Starovoitov wrote: >>>>>> On Thu, Aug 25, 2022 at 9:54 PM Jarkko Sakkinen < >>>>>> jarkko@kernel.org> wrote: >>>>>>>> -static bool is_dynptr_reg_valid_init(struct bpf_verifier_env >>>>>>>> *env, struct bpf_reg_state *reg, >>>>>>>> - enum bpf_arg_type >>>>>>>> arg_type) >>>>>>>> +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, >>>>>>>> struct bpf_reg_state *reg, >>>>>>>> + enum bpf_arg_type arg_type) >>>>>>>> { >>>>>>>> struct bpf_func_state *state = func(env, reg); >>>>>>>> int spi = get_spi(reg->off); >>>>>>>> -- >>>>>>>> 2.25.1 >>>>>>>> >>>>>>> >>>>>>> Might be niticking but generally I'd consider splitting >>>>>>> exports as commits of their own. >>>>>> >>>>>> -static bool >>>>>> +bool >>>>>> >>>>>> into a separate commit? >>>>>> >>>>>> I guess it makes sense for people whose salary depends on >>>>>> number of commits. >>>>>> We don't play these games. >>>>> >>>>> What kind of argument is that anyway. >>>> >>>> "Separate each *logical change* into a separate patch." [*] >>> >>> The logical change, as per the patch subject, is allowing the >>> possibility of including eBPF dynamic pointers in a kfunc definition. >>> It requires to call an existing function that was already defined >>> elsewhere. >>> >>> Maybe I'm wrong, but I don't see only exporting a function definition >>> to an include file as a logical change. To me, the changes in this >>> patch are clearly connected. Or even better, they tell why the function >>> definition has been exported, that would not appear if moving the >>> function definition is a standalone patch. >>> >>>> >>>> To add, generally any user space visible space should be an >>>> isolated patch. >>> >>> As far as I understood, definitions visible to user space should be in >>> include/uapi. >> >> It does change e.g. the output of kallsyms. >> >> It's not ABI but it's still user space visble. >> >>> >>>> >>>> Please, stop posting nonsense. >>> >>> If I may, saying this does not encourage people to try to submit their >>> code. I feel it is a bit strong, and I kindly ask you to express your >>> opinion in a more gentle way. >> >> I agree. That's why I was wondering what is this nonsense >> about salary and games. > > Please denote that I started my review with "Might be nitpicking...". > > It's neither particularly disencouraging nor enforcing for anyone. Thanks for clarifying. Yes, it is not. Sorry, I misunderstood. Roberto
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 2e3bad8640dc..55876fbdbae2 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -560,6 +560,9 @@ int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state u32 regno); int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg, u32 regno, u32 mem_size); +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, + struct bpf_reg_state *reg, + enum bpf_arg_type arg_type); /* this lives here instead of in bpf.h because it needs to dereference tgt_prog */ static inline u64 bpf_trampoline_compute_key(const struct bpf_prog *tgt_prog, diff --git a/include/linux/btf.h b/include/linux/btf.h index ad93c2d9cc1c..f546d368ac5d 100644 --- a/include/linux/btf.h +++ b/include/linux/btf.h @@ -52,6 +52,15 @@ #define KF_SLEEPABLE (1 << 5) /* kfunc may sleep */ #define KF_DESTRUCTIVE (1 << 6) /* kfunc performs destructive actions */ +/* + * Return the name of the passed struct, if exists, or halt the build if for + * example the structure gets renamed. In this way, developers have to revisit + * the code using that structure name, and update it accordingly. + */ +#define stringify_struct(x) \ + ({ BUILD_BUG_ON(sizeof(struct x) < 0); \ + __stringify(x); }) + struct btf; struct btf_member; struct btf_type; diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index e49b3b6d48ad..26cb548420af 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -6362,15 +6362,20 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, if (is_kfunc) { bool arg_mem_size = i + 1 < nargs && is_kfunc_arg_mem_size(btf, &args[i + 1], ®s[regno + 1]); + bool arg_dynptr = btf_type_is_struct(ref_t) && + !strcmp(ref_tname, + stringify_struct(bpf_dynptr_kern)); /* Permit pointer to mem, but only when argument * type is pointer to scalar, or struct composed * (recursively) of scalars. * When arg_mem_size is true, the pointer can be * void *. + * Also permit initialized dynamic pointers. */ if (!btf_type_is_scalar(ref_t) && !__btf_type_is_scalar_struct(log, btf, ref_t, 0) && + !arg_dynptr && (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) { bpf_log(log, "arg#%d pointer type %s %s must point to %sscalar, or struct with scalar\n", @@ -6378,6 +6383,19 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, return -EINVAL; } + if (arg_dynptr) { + if (!is_dynptr_reg_valid_init(env, reg, + ARG_PTR_TO_DYNPTR)) { + bpf_log(log, + "arg#%d pointer type %s %s must be initialized\n", + i, btf_type_str(ref_t), + ref_tname); + return -EINVAL; + } + + continue; + } + /* Check for mem, len pair */ if (arg_mem_size) { if (check_kfunc_mem_size_reg(env, ®s[regno + 1], regno + 1)) { diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 2c1f8069f7b7..aa834e7bb296 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -779,8 +779,8 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_ return true; } -static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg, - enum bpf_arg_type arg_type) +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg, + enum bpf_arg_type arg_type) { struct bpf_func_state *state = func(env, reg); int spi = get_spi(reg->off);