diff mbox series

[RFC,bpf-next,v4,3/7] error-inject: add new type that carries if the function is non sleepable

Message ID 20220421140740.459558-4-benjamin.tissoires@redhat.com
State New
Headers show
Series Introduce eBPF support for HID devices (new attempt) | expand

Commit Message

Benjamin Tissoires April 21, 2022, 2:07 p.m. UTC
When using error-injection function through bpf to change the return
code, we need to know if the function is sleepable or not.

Currently the code assumes that all error-inject functions are sleepable,
except for a few selected of them, hardcoded in kernel/bpf/verifier.c

Add a new flag to error-inject so we can code that information where the
function is declared.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

---

new in v4:
- another approach would be to define a new kfunc_set, and register
  it with btf. But in that case, what program type would we use?
  BPF_PROG_TYPE_UNSPEC?
- also note that maybe we should consider all of the functions
  non-sleepable and only mark some as sleepable. IMO it makes more
  sense to be more restrictive by default.
---
 include/asm-generic/error-injection.h |  1 +
 kernel/bpf/verifier.c                 | 10 ++++++++--
 lib/error-inject.c                    |  2 ++
 3 files changed, 11 insertions(+), 2 deletions(-)

Comments

Alexei Starovoitov April 26, 2022, 4:11 a.m. UTC | #1
On Thu, Apr 21, 2022 at 04:07:36PM +0200, Benjamin Tissoires wrote:
> When using error-injection function through bpf to change the return
> code, we need to know if the function is sleepable or not.
> 
> Currently the code assumes that all error-inject functions are sleepable,
> except for a few selected of them, hardcoded in kernel/bpf/verifier.c
> 
> Add a new flag to error-inject so we can code that information where the
> function is declared.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> ---
> 
> new in v4:
> - another approach would be to define a new kfunc_set, and register
>   it with btf. But in that case, what program type would we use?
>   BPF_PROG_TYPE_UNSPEC?
> - also note that maybe we should consider all of the functions
>   non-sleepable and only mark some as sleepable. IMO it makes more
>   sense to be more restrictive by default.

I think the approach in this patch is fine.
We didn't have issues with check_non_sleepable_error_inject() so far,
so I wouldn't start refactoring it.

> ---
>  include/asm-generic/error-injection.h |  1 +
>  kernel/bpf/verifier.c                 | 10 ++++++++--
>  lib/error-inject.c                    |  2 ++
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/include/asm-generic/error-injection.h b/include/asm-generic/error-injection.h
> index fbca56bd9cbc..5974942353a6 100644
> --- a/include/asm-generic/error-injection.h
> +++ b/include/asm-generic/error-injection.h
> @@ -9,6 +9,7 @@ enum {
>  	EI_ETYPE_ERRNO,		/* Return -ERRNO if failure */
>  	EI_ETYPE_ERRNO_NULL,	/* Return -ERRNO or NULL if failure */
>  	EI_ETYPE_TRUE,		/* Return true if failure */
> +	EI_ETYPE_NS_ERRNO,	/* Return -ERRNO if failure and tag the function as non-sleepable */

>  };
>  
>  struct error_injection_entry {
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0f339f9058f3..45c8feea6478 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -14085,6 +14085,11 @@ static int check_non_sleepable_error_inject(u32 btf_id)
>  	return btf_id_set_contains(&btf_non_sleepable_error_inject, btf_id);
>  }
>  
> +static int is_non_sleepable_error_inject(unsigned long addr)
> +{
> +	return get_injectable_error_type(addr) == EI_ETYPE_NS_ERRNO;

It's a linear search. Probably ok. But would be good to double check
that we're not calling it a lot.

> +}
> +
>  int bpf_check_attach_target(struct bpf_verifier_log *log,
>  			    const struct bpf_prog *prog,
>  			    const struct bpf_prog *tgt_prog,
> @@ -14281,8 +14286,9 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>  				/* fentry/fexit/fmod_ret progs can be sleepable only if they are
>  				 * attached to ALLOW_ERROR_INJECTION and are not in denylist.
>  				 */
> -				if (!check_non_sleepable_error_inject(btf_id) &&
> -				    within_error_injection_list(addr))
> +				if (within_error_injection_list(addr) &&
> +				    !check_non_sleepable_error_inject(btf_id) &&
> +				    !is_non_sleepable_error_inject(addr))
>  					ret = 0;
>  				break;
>  			case BPF_PROG_TYPE_LSM:
> diff --git a/lib/error-inject.c b/lib/error-inject.c
> index 2ff5ef689d72..560c3b18f439 100644
> --- a/lib/error-inject.c
> +++ b/lib/error-inject.c
> @@ -183,6 +183,8 @@ static const char *error_type_string(int etype)
>  		return "ERRNO_NULL";
>  	case EI_ETYPE_TRUE:
>  		return "TRUE";
> +	case EI_ETYPE_NS_ERRNO:
> +		return "NS_ERRNO";
>  	default:
>  		return "(unknown)";
>  	}
> -- 
> 2.35.1
>
Benjamin Tissoires April 26, 2022, 7:52 a.m. UTC | #2
On Tue, Apr 26, 2022 at 6:11 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Apr 21, 2022 at 04:07:36PM +0200, Benjamin Tissoires wrote:
> > When using error-injection function through bpf to change the return
> > code, we need to know if the function is sleepable or not.
> >
> > Currently the code assumes that all error-inject functions are sleepable,
> > except for a few selected of them, hardcoded in kernel/bpf/verifier.c
> >
> > Add a new flag to error-inject so we can code that information where the
> > function is declared.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >
> > ---
> >
> > new in v4:
> > - another approach would be to define a new kfunc_set, and register
> >   it with btf. But in that case, what program type would we use?
> >   BPF_PROG_TYPE_UNSPEC?
> > - also note that maybe we should consider all of the functions
> >   non-sleepable and only mark some as sleepable. IMO it makes more
> >   sense to be more restrictive by default.
>
> I think the approach in this patch is fine.
> We didn't have issues with check_non_sleepable_error_inject() so far,
> so I wouldn't start refactoring it.

OK... though I can't help but thinking that adding a new
error-inject.h enum value is going to be bad, because it's an API
change, and users might not expect NS_ERRNO.

OTOH, if we had a new kfunc_set, we keep the existing error-inject API
in place with all the variants and we just teach the verifier that the
function is non sleepable.

>
> > ---
> >  include/asm-generic/error-injection.h |  1 +
> >  kernel/bpf/verifier.c                 | 10 ++++++++--
> >  lib/error-inject.c                    |  2 ++
> >  3 files changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/asm-generic/error-injection.h b/include/asm-generic/error-injection.h
> > index fbca56bd9cbc..5974942353a6 100644
> > --- a/include/asm-generic/error-injection.h
> > +++ b/include/asm-generic/error-injection.h
> > @@ -9,6 +9,7 @@ enum {
> >       EI_ETYPE_ERRNO,         /* Return -ERRNO if failure */
> >       EI_ETYPE_ERRNO_NULL,    /* Return -ERRNO or NULL if failure */
> >       EI_ETYPE_TRUE,          /* Return true if failure */
> > +     EI_ETYPE_NS_ERRNO,      /* Return -ERRNO if failure and tag the function as non-sleepable */
>
> >  };
> >
> >  struct error_injection_entry {
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 0f339f9058f3..45c8feea6478 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -14085,6 +14085,11 @@ static int check_non_sleepable_error_inject(u32 btf_id)
> >       return btf_id_set_contains(&btf_non_sleepable_error_inject, btf_id);
> >  }
> >
> > +static int is_non_sleepable_error_inject(unsigned long addr)
> > +{
> > +     return get_injectable_error_type(addr) == EI_ETYPE_NS_ERRNO;
>
> It's a linear search. Probably ok. But would be good to double check
> that we're not calling it a lot.

IIUC, the kfunc_set approach would solve that, no?

Cheers,
Benjamin

>
> > +}
> > +
> >  int bpf_check_attach_target(struct bpf_verifier_log *log,
> >                           const struct bpf_prog *prog,
> >                           const struct bpf_prog *tgt_prog,
> > @@ -14281,8 +14286,9 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> >                               /* fentry/fexit/fmod_ret progs can be sleepable only if they are
> >                                * attached to ALLOW_ERROR_INJECTION and are not in denylist.
> >                                */
> > -                             if (!check_non_sleepable_error_inject(btf_id) &&
> > -                                 within_error_injection_list(addr))
> > +                             if (within_error_injection_list(addr) &&
> > +                                 !check_non_sleepable_error_inject(btf_id) &&
> > +                                 !is_non_sleepable_error_inject(addr))
> >                                       ret = 0;
> >                               break;
> >                       case BPF_PROG_TYPE_LSM:
> > diff --git a/lib/error-inject.c b/lib/error-inject.c
> > index 2ff5ef689d72..560c3b18f439 100644
> > --- a/lib/error-inject.c
> > +++ b/lib/error-inject.c
> > @@ -183,6 +183,8 @@ static const char *error_type_string(int etype)
> >               return "ERRNO_NULL";
> >       case EI_ETYPE_TRUE:
> >               return "TRUE";
> > +     case EI_ETYPE_NS_ERRNO:
> > +             return "NS_ERRNO";
> >       default:
> >               return "(unknown)";
> >       }
> > --
> > 2.35.1
> >
>
Alexei Starovoitov April 30, 2022, 3:29 a.m. UTC | #3
On Tue, Apr 26, 2022 at 12:52 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Tue, Apr 26, 2022 at 6:11 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Thu, Apr 21, 2022 at 04:07:36PM +0200, Benjamin Tissoires wrote:
> > > When using error-injection function through bpf to change the return
> > > code, we need to know if the function is sleepable or not.
> > >
> > > Currently the code assumes that all error-inject functions are sleepable,
> > > except for a few selected of them, hardcoded in kernel/bpf/verifier.c
> > >
> > > Add a new flag to error-inject so we can code that information where the
> > > function is declared.
> > >
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > >
> > > ---
> > >
> > > new in v4:
> > > - another approach would be to define a new kfunc_set, and register
> > >   it with btf. But in that case, what program type would we use?
> > >   BPF_PROG_TYPE_UNSPEC?
> > > - also note that maybe we should consider all of the functions
> > >   non-sleepable and only mark some as sleepable. IMO it makes more
> > >   sense to be more restrictive by default.
> >
> > I think the approach in this patch is fine.
> > We didn't have issues with check_non_sleepable_error_inject() so far,
> > so I wouldn't start refactoring it.
>
> OK... though I can't help but thinking that adding a new
> error-inject.h enum value is going to be bad, because it's an API
> change, and users might not expect NS_ERRNO.

Not sure about api concern. This is the kernel internal tag.
bpf progs are not aware of them. The functions can change
from sleepable to non-sleepable too.
allow_error_inject can be removed. And so on.

> OTOH, if we had a new kfunc_set, we keep the existing error-inject API
> in place with all the variants and we just teach the verifier that the
> function is non sleepable.
...
> IIUC, the kfunc_set approach would solve that, no?

Makes sense. Let's figure out an extensible kfunc_set approach
that is not centralized in verifier.c
Benjamin Tissoires April 30, 2022, 7:24 a.m. UTC | #4
On Sat, Apr 30, 2022 at 5:30 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Apr 26, 2022 at 12:52 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > On Tue, Apr 26, 2022 at 6:11 AM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Thu, Apr 21, 2022 at 04:07:36PM +0200, Benjamin Tissoires wrote:
> > > > When using error-injection function through bpf to change the return
> > > > code, we need to know if the function is sleepable or not.
> > > >
> > > > Currently the code assumes that all error-inject functions are sleepable,
> > > > except for a few selected of them, hardcoded in kernel/bpf/verifier.c
> > > >
> > > > Add a new flag to error-inject so we can code that information where the
> > > > function is declared.
> > > >
> > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > >
> > > > ---
> > > >
> > > > new in v4:
> > > > - another approach would be to define a new kfunc_set, and register
> > > >   it with btf. But in that case, what program type would we use?
> > > >   BPF_PROG_TYPE_UNSPEC?
> > > > - also note that maybe we should consider all of the functions
> > > >   non-sleepable and only mark some as sleepable. IMO it makes more
> > > >   sense to be more restrictive by default.
> > >
> > > I think the approach in this patch is fine.
> > > We didn't have issues with check_non_sleepable_error_inject() so far,
> > > so I wouldn't start refactoring it.
> >
> > OK... though I can't help but thinking that adding a new
> > error-inject.h enum value is going to be bad, because it's an API
> > change, and users might not expect NS_ERRNO.
>
> Not sure about api concern. This is the kernel internal tag.
> bpf progs are not aware of them. The functions can change
> from sleepable to non-sleepable too.
> allow_error_inject can be removed. And so on.
>
> > OTOH, if we had a new kfunc_set, we keep the existing error-inject API
> > in place with all the variants and we just teach the verifier that the
> > function is non sleepable.
> ...
> > IIUC, the kfunc_set approach would solve that, no?
>
> Makes sense. Let's figure out an extensible kfunc_set approach
> that is not centralized in verifier.c
>

OK, I'll work on this in v5.

But I need to rethink the whole sleepable/non-sleepable definitions
for my use case, because I have now a clear separation between not
sleepable context (in fentry/fexit/fmod_ret) and sleepable context (in
SEC("syscall")), so maybe the whole thing is not really required.

Cheers,
Benjamin
diff mbox series

Patch

diff --git a/include/asm-generic/error-injection.h b/include/asm-generic/error-injection.h
index fbca56bd9cbc..5974942353a6 100644
--- a/include/asm-generic/error-injection.h
+++ b/include/asm-generic/error-injection.h
@@ -9,6 +9,7 @@  enum {
 	EI_ETYPE_ERRNO,		/* Return -ERRNO if failure */
 	EI_ETYPE_ERRNO_NULL,	/* Return -ERRNO or NULL if failure */
 	EI_ETYPE_TRUE,		/* Return true if failure */
+	EI_ETYPE_NS_ERRNO,	/* Return -ERRNO if failure and tag the function as non-sleepable */
 };
 
 struct error_injection_entry {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0f339f9058f3..45c8feea6478 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -14085,6 +14085,11 @@  static int check_non_sleepable_error_inject(u32 btf_id)
 	return btf_id_set_contains(&btf_non_sleepable_error_inject, btf_id);
 }
 
+static int is_non_sleepable_error_inject(unsigned long addr)
+{
+	return get_injectable_error_type(addr) == EI_ETYPE_NS_ERRNO;
+}
+
 int bpf_check_attach_target(struct bpf_verifier_log *log,
 			    const struct bpf_prog *prog,
 			    const struct bpf_prog *tgt_prog,
@@ -14281,8 +14286,9 @@  int bpf_check_attach_target(struct bpf_verifier_log *log,
 				/* fentry/fexit/fmod_ret progs can be sleepable only if they are
 				 * attached to ALLOW_ERROR_INJECTION and are not in denylist.
 				 */
-				if (!check_non_sleepable_error_inject(btf_id) &&
-				    within_error_injection_list(addr))
+				if (within_error_injection_list(addr) &&
+				    !check_non_sleepable_error_inject(btf_id) &&
+				    !is_non_sleepable_error_inject(addr))
 					ret = 0;
 				break;
 			case BPF_PROG_TYPE_LSM:
diff --git a/lib/error-inject.c b/lib/error-inject.c
index 2ff5ef689d72..560c3b18f439 100644
--- a/lib/error-inject.c
+++ b/lib/error-inject.c
@@ -183,6 +183,8 @@  static const char *error_type_string(int etype)
 		return "ERRNO_NULL";
 	case EI_ETYPE_TRUE:
 		return "TRUE";
+	case EI_ETYPE_NS_ERRNO:
+		return "NS_ERRNO";
 	default:
 		return "(unknown)";
 	}