From: Teng Qi <[email protected]>
bpf_mmap_unlock_get_irq_work() and bpf_mmap_unlock_mm() cooperate to safely
acquire mm->mmap_lock safely. The code in bpf_mmap_unlock_get_irq_work():
struct mmap_unlock_irq_work *work = NULL;
bool irq_work_busy = false;
if (irqs_disabled()) {
if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
work = this_cpu_ptr(&mmap_unlock_work);
if (irq_work_is_busy(&work->irq_work)) {
irq_work_busy = true;
}
} else {
irq_work_busy = true;
}
}
*work_ptr = work;
shows that the pointer of struct mmap_unlock_irq_work "work" is not NULL if
irqs_disabled() == true and IS_ENABLED(CONFIG_PREEMPT_RT) == false or NULL in
other cases. The "work" will be passed to bpf_mmap_unlock_mm() as the argument.
The code in bpf_mmap_unlock_mm():
if (!work) {
mmap_read_unlock(mm);
} else {
work->mm = mm;
rwsem_release(&mm->mmap_lock.dep_map, _RET_IP_);
irq_work_queue(&work->irq_work);
}
shows that mm->mmap_lock is released directly if "work" is NULL. Otherwise,
irq_work_queue is called to avoid calling mmap_read_unlock() in an irq disabled
context because of its possible sleep operation. However, mmap_read_unlock()
is unsafely called in a preempt disabled context when spin_lock() or
rcu_read_lock() has been called.
We found that some ebpf helpers that call these two functions may be invoked in
preempt disabled contexts through various hooks. We can give an example:
SEC("kprobe/kmem_cache_free")
int bpf_prog1(struct pt_regs *ctx)
{
char buff[50];
bpf_get_stack(ctx, buff, sizeof(struct bpf_stack_build_id),
BPF_F_USER_BUILD_ID | BPF_F_USER_STACK);
return 0;
}
The hook "kprobe/kmem_cache_free" is often called in preempt disabled contexts
by many modules. To fix this possible bug, we add in_atomic() in
bpf_mmap_unlock_get_irq_work().
Signed-off-by: Teng Qi <[email protected]>
---
kernel/bpf/mmap_unlock_work.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/mmap_unlock_work.h b/kernel/bpf/mmap_unlock_work.h
index 5d18d7d85bef..3d472d24d88f 100644
--- a/kernel/bpf/mmap_unlock_work.h
+++ b/kernel/bpf/mmap_unlock_work.h
@@ -26,7 +26,7 @@ static inline bool bpf_mmap_unlock_get_irq_work(struct mmap_unlock_irq_work **wo
struct mmap_unlock_irq_work *work = NULL;
bool irq_work_busy = false;
- if (irqs_disabled()) {
+ if (in_atomic() || irqs_disabled()) {
if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
work = this_cpu_ptr(&mmap_unlock_work);
if (irq_work_is_busy(&work->irq_work)) {
--
2.25.1
starmiku1207184332@ wrote:
> From: Teng Qi <[email protected]>
>
> bpf_mmap_unlock_get_irq_work() and bpf_mmap_unlock_mm() cooperate to safely
> acquire mm->mmap_lock safely. The code in bpf_mmap_unlock_get_irq_work():
> struct mmap_unlock_irq_work *work = NULL;
> bool irq_work_busy = false;
> if (irqs_disabled()) {
> if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
> work = this_cpu_ptr(&mmap_unlock_work);
> if (irq_work_is_busy(&work->irq_work)) {
> irq_work_busy = true;
> }
> } else {
> irq_work_busy = true;
> }
> }
> *work_ptr = work;
>
> shows that the pointer of struct mmap_unlock_irq_work "work" is not NULL if
> irqs_disabled() == true and IS_ENABLED(CONFIG_PREEMPT_RT) == false or NULL in
> other cases. The "work" will be passed to bpf_mmap_unlock_mm() as the argument.
> The code in bpf_mmap_unlock_mm():
> if (!work) {
> mmap_read_unlock(mm);
> } else {
> work->mm = mm;
> rwsem_release(&mm->mmap_lock.dep_map, _RET_IP_);
> irq_work_queue(&work->irq_work);
> }
>
> shows that mm->mmap_lock is released directly if "work" is NULL. Otherwise,
> irq_work_queue is called to avoid calling mmap_read_unlock() in an irq disabled
> context because of its possible sleep operation. However, mmap_read_unlock()
> is unsafely called in a preempt disabled context when spin_lock() or
> rcu_read_lock() has been called.
>
> We found that some ebpf helpers that call these two functions may be invoked in
> preempt disabled contexts through various hooks. We can give an example:
Did you get a stack trace we could add to the commit message would be nice
to have.
> SEC("kprobe/kmem_cache_free")
> int bpf_prog1(struct pt_regs *ctx)
> {
> char buff[50];
> bpf_get_stack(ctx, buff, sizeof(struct bpf_stack_build_id),
> BPF_F_USER_BUILD_ID | BPF_F_USER_STACK);
> return 0;
> }
How about add this as a selftest so we can ensure we keep this working.
>
> The hook "kprobe/kmem_cache_free" is often called in preempt disabled contexts
> by many modules. To fix this possible bug, we add in_atomic() in
> bpf_mmap_unlock_get_irq_work().
>
>
> Signed-off-by: Teng Qi <[email protected]>
And a fixes tag so we get backports correct.
> ---
> kernel/bpf/mmap_unlock_work.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/mmap_unlock_work.h b/kernel/bpf/mmap_unlock_work.h
> index 5d18d7d85bef..3d472d24d88f 100644
> --- a/kernel/bpf/mmap_unlock_work.h
> +++ b/kernel/bpf/mmap_unlock_work.h
> @@ -26,7 +26,7 @@ static inline bool bpf_mmap_unlock_get_irq_work(struct mmap_unlock_irq_work **wo
> struct mmap_unlock_irq_work *work = NULL;
> bool irq_work_busy = false;
>
> - if (irqs_disabled()) {
> + if (in_atomic() || irqs_disabled()) {
> if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
> work = this_cpu_ptr(&mmap_unlock_work);
> if (irq_work_is_busy(&work->irq_work)) {
Thanks.