Date:   Wed, 15 Mar 2023 14:17:52 -0700
From:   John Fastabend <john.fastabend@gmail.com>
To:     starmiku1207184332@gmail.com, ast@kernel.org, daniel@iogearbox.net,
        andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
        yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org,
        sdf@google.com, haoluo@google.com, jolsa@kernel.org
Cc:     bpf@vger.kernel.org, linux-kernel@vger.kernel.org,
        baijiaju1990@outlook.com, Teng Qi <starmiku1207184332@gmail.com>
Message-ID: <641236007b4ca_63dce208c7@john.notmuch>
In-Reply-To: <20230315041721.1034794-1-starmiku1207184332@gmail.com>
References: <20230315041721.1034794-1-starmiku1207184332@gmail.com>
Subject: RE: [PATCH] kernel: bpf: stackmap: fix a possible sleep-in-atomic bug
 in bpf_mmap_unlock_get_irq_work()
Mime-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

starmiku1207184332@ wrote:
> From: Teng Qi <starmiku1207184332@gmail.com>
> 
> 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 <starmiku1207184332@gmail.com>

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.