On Wed, Aug 10, 2022 at 3:44 PM Roberto Sassu <[email protected]> wrote:
>
> > From: Jarkko Sakkinen [mailto:[email protected]]
> > Sent: Tuesday, August 9, 2022 6:55 PM
> > On Tue, Aug 09, 2022 at 03:45:54PM +0200, Roberto Sassu wrote:
> > > From: Benjamin Tissoires <[email protected]>
> > >
> > > From: Benjamin Tissoires <[email protected]>
> > >
> > > This allows to declare a kfunc as sleepable and prevents its use in
> > > a non sleepable program.
> >
> > Nit: "Declare a kfunc as sleepable and prevent its use in a
> > non-sleepable program."
> >
> > It's missing the part *how* the patch accomplishes its goals.
>
> I will add:
>
> If an eBPF program is going to call a kfunc declared as sleepable,
> eBPF will look at the eBPF program flags. If BPF_F_SLEEPABLE is
> not set, execution of that program is denied.
All those changes are looking good to me.
Thanks a lot for keeping pushing on this patch :)
Cheers,
Benjamin
>
> Roberto
>
> > > Signed-off-by: Benjamin Tissoires <[email protected]>
> > > Co-developed-by: Yosry Ahmed <[email protected]>
> > > Signed-off-by: Yosry Ahmed <[email protected]>
> > > Signed-off-by: Hao Luo <[email protected]>
> > > ---
> > > Documentation/bpf/kfuncs.rst | 6 ++++++
> > > include/linux/btf.h | 1 +
> > > kernel/bpf/btf.c | 9 +++++++++
> > > 3 files changed, 16 insertions(+)
> > >
> > > diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
> > > index c0b7dae6dbf5..c8b21de1c772 100644
> > > --- a/Documentation/bpf/kfuncs.rst
> > > +++ b/Documentation/bpf/kfuncs.rst
> > > @@ -146,6 +146,12 @@ that operate (change some property, perform some
> > operation) on an object that
> > > was obtained using an acquire kfunc. Such kfuncs need an unchanged pointer
> > to
> > > ensure the integrity of the operation being performed on the expected object.
> > >
> > > +2.4.6 KF_SLEEPABLE flag
> > > +-----------------------
> > > +
> > > +The KF_SLEEPABLE flag is used for kfuncs that may sleep. Such kfuncs can
> > only
> > > +be called by sleepable BPF programs (BPF_F_SLEEPABLE).
> > > +
> > > 2.5 Registering the kfuncs
> > > --------------------------
> > >
> > > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > > index cdb376d53238..976cbdd2981f 100644
> > > --- a/include/linux/btf.h
> > > +++ b/include/linux/btf.h
> > > @@ -49,6 +49,7 @@
> > > * for this case.
> > > */
> > > #define KF_TRUSTED_ARGS (1 << 4) /* kfunc only takes trusted pointer
> > arguments */
> > > +#define KF_SLEEPABLE (1 << 5) /* kfunc may sleep */
> > >
> > > struct btf;
> > > struct btf_member;
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index 7e64447659f3..d3e4c86b8fcd 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -6175,6 +6175,7 @@ static int btf_check_func_arg_match(struct
> > bpf_verifier_env *env,
> > > {
> > > enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
> > > bool rel = false, kptr_get = false, trusted_arg = false;
> > > + bool sleepable = false;
> > > struct bpf_verifier_log *log = &env->log;
> > > u32 i, nargs, ref_id, ref_obj_id = 0;
> > > bool is_kfunc = btf_is_kernel(btf);
> > > @@ -6212,6 +6213,7 @@ static int btf_check_func_arg_match(struct
> > bpf_verifier_env *env,
> > > rel = kfunc_flags & KF_RELEASE;
> > > kptr_get = kfunc_flags & KF_KPTR_GET;
> > > trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
> > > + sleepable = kfunc_flags & KF_SLEEPABLE;
> > > }
> > >
> > > /* check that BTF function arguments match actual types that the
> > > @@ -6419,6 +6421,13 @@ static int btf_check_func_arg_match(struct
> > bpf_verifier_env *env,
> > > func_name);
> > > return -EINVAL;
> > > }
> > > +
> > > + if (sleepable && !env->prog->aux->sleepable) {
> > > + bpf_log(log, "kernel function %s is sleepable but the program is
> > not\n",
> > > + func_name);
> > > + return -EINVAL;
> > > + }
> > > +
> > > /* returns argument register number > 0 in case of reference release
> > kfunc */
> > > return rel ? ref_regno : 0;
> > > }
> > > --
> > > 2.25.1
> > >
> >
> > BR, Jarkko
>
On 8/10/22 4:25 PM, Benjamin Tissoires wrote:
> On Wed, Aug 10, 2022 at 3:44 PM Roberto Sassu <[email protected]> wrote:
>>> From: Jarkko Sakkinen [mailto:[email protected]]
>>> Sent: Tuesday, August 9, 2022 6:55 PM
>>> On Tue, Aug 09, 2022 at 03:45:54PM +0200, Roberto Sassu wrote:
>>>> From: Benjamin Tissoires <[email protected]>
>>>>
>>>> From: Benjamin Tissoires <[email protected]>
>>>>
>>>> This allows to declare a kfunc as sleepable and prevents its use in
>>>> a non sleepable program.
>>>
>>> Nit: "Declare a kfunc as sleepable and prevent its use in a
>>> non-sleepable program."
>>>
>>> It's missing the part *how* the patch accomplishes its goals.
>>
>> I will add:
>>
>> If an eBPF program is going to call a kfunc declared as sleepable,
>> eBPF will look at the eBPF program flags. If BPF_F_SLEEPABLE is
>> not set, execution of that program is denied.
>
> All those changes are looking good to me.
>
> Thanks a lot for keeping pushing on this patch :)
This single one from the series got already applied here:
https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=fa96b24204af42274ec13dfb2f2e6990d7510e55
>>>> Signed-off-by: Benjamin Tissoires <[email protected]>
>>>> Co-developed-by: Yosry Ahmed <[email protected]>
>>>> Signed-off-by: Yosry Ahmed <[email protected]>
>>>> Signed-off-by: Hao Luo <[email protected]>
>>>> ---
>>>> Documentation/bpf/kfuncs.rst | 6 ++++++
>>>> include/linux/btf.h | 1 +
>>>> kernel/bpf/btf.c | 9 +++++++++
>>>> 3 files changed, 16 insertions(+)
>>>>
>>>> diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
>>>> index c0b7dae6dbf5..c8b21de1c772 100644
>>>> --- a/Documentation/bpf/kfuncs.rst
>>>> +++ b/Documentation/bpf/kfuncs.rst
>>>> @@ -146,6 +146,12 @@ that operate (change some property, perform some
>>> operation) on an object that
>>>> was obtained using an acquire kfunc. Such kfuncs need an unchanged pointer
>>> to
>>>> ensure the integrity of the operation being performed on the expected object.
On Wed, Aug 10, 2022 at 7:26 AM Benjamin Tissoires
<[email protected]> wrote:
>
> On Wed, Aug 10, 2022 at 3:44 PM Roberto Sassu <[email protected]> wrote:
> >
> > > From: Jarkko Sakkinen [mailto:[email protected]]
> > > Sent: Tuesday, August 9, 2022 6:55 PM
> > > On Tue, Aug 09, 2022 at 03:45:54PM +0200, Roberto Sassu wrote:
> > > > From: Benjamin Tissoires <[email protected]>
> > > >
> > > > From: Benjamin Tissoires <[email protected]>
> > > >
> > > > This allows to declare a kfunc as sleepable and prevents its use in
> > > > a non sleepable program.
> > >
> > > Nit: "Declare a kfunc as sleepable and prevent its use in a
> > > non-sleepable program."
> > >
> > > It's missing the part *how* the patch accomplishes its goals.
> >
> > I will add:
> >
> > If an eBPF program is going to call a kfunc declared as sleepable,
> > eBPF will look at the eBPF program flags. If BPF_F_SLEEPABLE is
> > not set, execution of that program is denied.
>
> All those changes are looking good to me.
>
> Thanks a lot for keeping pushing on this patch :)
Multiple series other than the HID one started resending your patch
once you dropped it, it's obviously needed :) Thanks for sending it in
the first place :)
>
> Cheers,
> Benjamin
>
> >
> > Roberto
> >
> > > > Signed-off-by: Benjamin Tissoires <[email protected]>
> > > > Co-developed-by: Yosry Ahmed <[email protected]>
> > > > Signed-off-by: Yosry Ahmed <[email protected]>
> > > > Signed-off-by: Hao Luo <[email protected]>
> > > > ---
> > > > Documentation/bpf/kfuncs.rst | 6 ++++++
> > > > include/linux/btf.h | 1 +
> > > > kernel/bpf/btf.c | 9 +++++++++
> > > > 3 files changed, 16 insertions(+)
> > > >
> > > > diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
> > > > index c0b7dae6dbf5..c8b21de1c772 100644
> > > > --- a/Documentation/bpf/kfuncs.rst
> > > > +++ b/Documentation/bpf/kfuncs.rst
> > > > @@ -146,6 +146,12 @@ that operate (change some property, perform some
> > > operation) on an object that
> > > > was obtained using an acquire kfunc. Such kfuncs need an unchanged pointer
> > > to
> > > > ensure the integrity of the operation being performed on the expected object.
> > > >
> > > > +2.4.6 KF_SLEEPABLE flag
> > > > +-----------------------
> > > > +
> > > > +The KF_SLEEPABLE flag is used for kfuncs that may sleep. Such kfuncs can
> > > only
> > > > +be called by sleepable BPF programs (BPF_F_SLEEPABLE).
> > > > +
> > > > 2.5 Registering the kfuncs
> > > > --------------------------
> > > >
> > > > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > > > index cdb376d53238..976cbdd2981f 100644
> > > > --- a/include/linux/btf.h
> > > > +++ b/include/linux/btf.h
> > > > @@ -49,6 +49,7 @@
> > > > * for this case.
> > > > */
> > > > #define KF_TRUSTED_ARGS (1 << 4) /* kfunc only takes trusted pointer
> > > arguments */
> > > > +#define KF_SLEEPABLE (1 << 5) /* kfunc may sleep */
> > > >
> > > > struct btf;
> > > > struct btf_member;
> > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > > index 7e64447659f3..d3e4c86b8fcd 100644
> > > > --- a/kernel/bpf/btf.c
> > > > +++ b/kernel/bpf/btf.c
> > > > @@ -6175,6 +6175,7 @@ static int btf_check_func_arg_match(struct
> > > bpf_verifier_env *env,
> > > > {
> > > > enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
> > > > bool rel = false, kptr_get = false, trusted_arg = false;
> > > > + bool sleepable = false;
> > > > struct bpf_verifier_log *log = &env->log;
> > > > u32 i, nargs, ref_id, ref_obj_id = 0;
> > > > bool is_kfunc = btf_is_kernel(btf);
> > > > @@ -6212,6 +6213,7 @@ static int btf_check_func_arg_match(struct
> > > bpf_verifier_env *env,
> > > > rel = kfunc_flags & KF_RELEASE;
> > > > kptr_get = kfunc_flags & KF_KPTR_GET;
> > > > trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
> > > > + sleepable = kfunc_flags & KF_SLEEPABLE;
> > > > }
> > > >
> > > > /* check that BTF function arguments match actual types that the
> > > > @@ -6419,6 +6421,13 @@ static int btf_check_func_arg_match(struct
> > > bpf_verifier_env *env,
> > > > func_name);
> > > > return -EINVAL;
> > > > }
> > > > +
> > > > + if (sleepable && !env->prog->aux->sleepable) {
> > > > + bpf_log(log, "kernel function %s is sleepable but the program is
> > > not\n",
> > > > + func_name);
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > /* returns argument register number > 0 in case of reference release
> > > kfunc */
> > > > return rel ? ref_regno : 0;
> > > > }
> > > > --
> > > > 2.25.1
> > > >
> > >
> > > BR, Jarkko
> >
>