| Message ID | 20220421140740.459558-4-benjamin.tissoires@redhat.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | BPF |
| Headers | show |
| Series | Introduce eBPF support for HID devices (new attempt) | expand |
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 >
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 > > >
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
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 --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)"; }
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(-)