From: KP Singh <[email protected]>
Usage of spin locks was not allowed for tracing programs due to
insufficient preemption checks. The verifier does not currently prevent
LSM programs from using spin locks, but the helpers are not exposed
via bpf_lsm_func_proto.
Based on the discussion in [1], non-sleepable LSM programs should be
able to use bpf_spin_{lock, unlock}.
Sleepable LSM programs can be preempted which means that allowng spin
locks will need more work (disabling preemption and the verifier
ensuring that no sleepable helpers are called when a spin lock is held).
[1]: https://lore.kernel.org/bpf/[email protected]/T/#md601a053229287659071600d3483523f752cd2fb
Signed-off-by: KP Singh <[email protected]>
---
kernel/bpf/bpf_lsm.c | 4 ++++
kernel/bpf/verifier.c | 17 +++++++++++++++++
2 files changed, 21 insertions(+)
diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index 61f8cc52fd5b..93383df2140b 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -63,6 +63,10 @@ bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_task_storage_get_proto;
case BPF_FUNC_task_storage_delete:
return &bpf_task_storage_delete_proto;
+ case BPF_FUNC_spin_lock:
+ return &bpf_spin_lock_proto;
+ case BPF_FUNC_spin_unlock:
+ return &bpf_spin_unlock_proto;
default:
return tracing_prog_func_proto(func_id, prog);
}
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 314018e8fc12..7c6c246077cf 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9739,6 +9739,23 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
return -EINVAL;
}
+ if (map_value_has_spin_lock(map)) {
+ if (prog_type == BPF_PROG_TYPE_SOCKET_FILTER) {
+ verbose(env, "socket filter progs cannot use bpf_spin_lock yet\n");
+ return -EINVAL;
+ }
+
+ if (is_tracing_prog_type(prog_type)) {
+ verbose(env, "tracing progs cannot use bpf_spin_lock yet\n");
+ return -EINVAL;
+ }
+
+ if (prog->aux->sleepable) {
+ verbose(env, "sleepable progs cannot use bpf_spin_lock yet\n");
+ return -EINVAL;
+ }
+ }
+
if ((bpf_prog_is_dev_bound(prog->aux) || bpf_map_is_dev_bound(map)) &&
!bpf_offload_prog_map_match(prog, map)) {
verbose(env, "offload device mismatch between prog and map\n");
--
2.29.1.341.ge80a0c044ae-goog
On Wed, Nov 04, 2020 at 05:44:49PM +0100, KP Singh wrote:
> From: KP Singh <[email protected]>
>
> Usage of spin locks was not allowed for tracing programs due to
> insufficient preemption checks. The verifier does not currently prevent
> LSM programs from using spin locks, but the helpers are not exposed
> via bpf_lsm_func_proto.
This could be the first patch but don't feel strongly about it.
>
> Based on the discussion in [1], non-sleepable LSM programs should be
> able to use bpf_spin_{lock, unlock}.
>
> Sleepable LSM programs can be preempted which means that allowng spin
> locks will need more work (disabling preemption and the verifier
> ensuring that no sleepable helpers are called when a spin lock is held).
>
> [1]: https://lore.kernel.org/bpf/[email protected]/T/#md601a053229287659071600d3483523f752cd2fb
>
> Signed-off-by: KP Singh <[email protected]>
> ---
> kernel/bpf/bpf_lsm.c | 4 ++++
> kernel/bpf/verifier.c | 17 +++++++++++++++++
> 2 files changed, 21 insertions(+)
>
> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index 61f8cc52fd5b..93383df2140b 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
> @@ -63,6 +63,10 @@ bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> return &bpf_task_storage_get_proto;
> case BPF_FUNC_task_storage_delete:
> return &bpf_task_storage_delete_proto;
> + case BPF_FUNC_spin_lock:
> + return &bpf_spin_lock_proto;
> + case BPF_FUNC_spin_unlock:
> + return &bpf_spin_unlock_proto;
> default:
> return tracing_prog_func_proto(func_id, prog);
> }
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 314018e8fc12..7c6c246077cf 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -9739,6 +9739,23 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
> return -EINVAL;
> }
>
> + if (map_value_has_spin_lock(map)) {
> + if (prog_type == BPF_PROG_TYPE_SOCKET_FILTER) {
> + verbose(env, "socket filter progs cannot use bpf_spin_lock yet\n");
> + return -EINVAL;
> + }
> +
> + if (is_tracing_prog_type(prog_type)) {
> + verbose(env, "tracing progs cannot use bpf_spin_lock yet\n");
> + return -EINVAL;
> + }
It is good to have a more specific verifier log. However,
these are duplicated checks (a few lines above in the same function).
They should at least be removed.
> +
> + if (prog->aux->sleepable) {
> + verbose(env, "sleepable progs cannot use bpf_spin_lock yet\n");
> + return -EINVAL;
> + }
> + }
> +
> if ((bpf_prog_is_dev_bound(prog->aux) || bpf_map_is_dev_bound(map)) &&
> !bpf_offload_prog_map_match(prog, map)) {
> verbose(env, "offload device mismatch between prog and map\n");
> --
> 2.29.1.341.ge80a0c044ae-goog
>
On Wed, Nov 4, 2020 at 11:35 PM Martin KaFai Lau <[email protected]> wrote:
>
> On Wed, Nov 04, 2020 at 05:44:49PM +0100, KP Singh wrote:
> > From: KP Singh <[email protected]>
> >
> > Usage of spin locks was not allowed for tracing programs due to
> > insufficient preemption checks. The verifier does not currently prevent
> > LSM programs from using spin locks, but the helpers are not exposed
> > via bpf_lsm_func_proto.
> This could be the first patch but don't feel strongly about it.
>
> >
> > Based on the discussion in [1], non-sleepable LSM programs should be
> > able to use bpf_spin_{lock, unlock}.
> >
> > Sleepable LSM programs can be preempted which means that allowng spin
> > locks will need more work (disabling preemption and the verifier
> > ensuring that no sleepable helpers are called when a spin lock is held).
> >
> > [1]: https://lore.kernel.org/bpf/[email protected]/T/#md601a053229287659071600d3483523f752cd2fb
> >
> > Signed-off-by: KP Singh <[email protected]>
> > ---
> > kernel/bpf/bpf_lsm.c | 4 ++++
> > kernel/bpf/verifier.c | 17 +++++++++++++++++
> > 2 files changed, 21 insertions(+)
> >
> > diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > index 61f8cc52fd5b..93383df2140b 100644
> > --- a/kernel/bpf/bpf_lsm.c
> > +++ b/kernel/bpf/bpf_lsm.c
> > @@ -63,6 +63,10 @@ bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> > return &bpf_task_storage_get_proto;
> > case BPF_FUNC_task_storage_delete:
> > return &bpf_task_storage_delete_proto;
> > + case BPF_FUNC_spin_lock:
> > + return &bpf_spin_lock_proto;
> > + case BPF_FUNC_spin_unlock:
> > + return &bpf_spin_unlock_proto;
> > default:
> > return tracing_prog_func_proto(func_id, prog);
> > }
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 314018e8fc12..7c6c246077cf 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -9739,6 +9739,23 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
> > return -EINVAL;
> > }
> >
> > + if (map_value_has_spin_lock(map)) {
> > + if (prog_type == BPF_PROG_TYPE_SOCKET_FILTER) {
> > + verbose(env, "socket filter progs cannot use bpf_spin_lock yet\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (is_tracing_prog_type(prog_type)) {
> > + verbose(env, "tracing progs cannot use bpf_spin_lock yet\n");
> > + return -EINVAL;
> > + }
> It is good to have a more specific verifier log. However,
> these are duplicated checks (a few lines above in the same function).
> They should at least be removed.
>
Thanks, I fixed this up and will move this as the first patch.
> > +
> > + if (prog->aux->sleepable) {
> > + verbose(env, "sleepable progs cannot use bpf_spin_lock yet\n");
> > + return -EINVAL;
> > + }
> > + }
> > +
> > if ((bpf_prog_is_dev_bound(prog->aux) || bpf_map_is_dev_bound(map)) &&
> > !bpf_offload_prog_map_match(prog, map)) {
> > verbose(env, "offload device mismatch between prog and map\n");
> > --
> > 2.29.1.341.ge80a0c044ae-goog
> >