From patchwork Thu May 25 14:18:13 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Sebastian Andrzej Siewior X-Patchwork-Id: 13255276 X-Patchwork-Delegate: bpf@iogearbox.net Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A8762101E4 for ; Thu, 25 May 2023 14:18:18 +0000 (UTC) Received: from galois.linutronix.de (Galois.linutronix.de [IPv6:2a0a:51c0:0:12e:550::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3C563191 for ; Thu, 25 May 2023 07:18:17 -0700 (PDT) Date: Thu, 25 May 2023 16:18:13 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1685024295; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=IlEzqf8v5LIHFpTK6bS8f7bh0N8Q4MKhEvHKfwTR7Lo=; b=qegWGBFjYZT8xoJu79oaHox4b1lJTsoBfa5uidlT67FSTd5pV25Wrbyadzz9TevEvP9Acw C2j2+Js1a1/72+yZZNZGIDh34rkf5dlt7IWXziBOwY8z0QxktMbfRRHLS+3gZlPg2j1ULb aCZaOP9HrBo6102nloLeXWm0AWdu0XrGgyxZ7Aua92dOsPTrysE/1srBqpprpSAmWoFS3N rFTsR6HlZ3FKqFi0JEt1dCW4eT7bSoOdZTDo9FbsQPlH0C4vJpHpG4CpOmSbCS+bAEiFoG vP4O8pzVTJxgXCS7+1CDRHG00S0FykJcXcYUF/cW/lUTGKVF+KK6tLSl6Xgoyw== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1685024295; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=IlEzqf8v5LIHFpTK6bS8f7bh0N8Q4MKhEvHKfwTR7Lo=; b=dTjZTP92nujluv04YY4Dty5AQxOEelsKuUrwlvzKZbl+/YbCwS5hOm+9wqZuYgLiCw6qB0 Bxli6zoXMSxiGUAw== From: Sebastian Andrzej Siewior To: Andrii Nakryiko Cc: Alexei Starovoitov , bpf , Alexei Starovoitov , Daniel Borkmann , John Fastabend , "Paul E. McKenney" , Peter Zijlstra , Thomas Gleixner Subject: [PATCH v2] bpf: Remove in_atomic() from bpf_link_put(). Message-ID: <20230525141813.TFZLWM4M@linutronix.de> References: <20230509132433.2FSY_6t7@linutronix.de> Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net X-Patchwork-Delegate: bpf@iogearbox.net bpf_free_inode() is invoked as a RCU callback. Usually RCU callbacks are invoked within softirq context. By setting rcutree.use_softirq=0 boot option the RCU callbacks will be invoked in a per-CPU kthread with bottom halves disabled which implies a RCU read section. On PREEMPT_RT the context remains fully preemptible. The RCU read section however does not allow schedule() invocation. The latter happens in mutex_lock() performed by bpf_trampoline_unlink_prog() originated from bpf_link_put(). It was pointed out that the bpf_link_put() invocation should not be delayed if originated from close(). Remove the context checks and use the workqueue unconditionally. For the close() callback add bpf_link_put_direct() which invokes free function directly. Signed-off-by: Sebastian Andrzej Siewior --- v1…v2: - Add bpf_link_put_direct() to be used from bpf_link_release() as suggested. On 2023-05-17 09:26:19 [-0700], Andrii Nakryiko wrote: > Is struct file_operations.release() callback guaranteed to be called > from user context? If yes, then perhaps the most straightforward way > to guarantee synchronous bpf_link cleanup on close(link_fd) is to have > a bpf_link_put() variant that will be only called from user-context > and will just do bpf_link_free(link) directly, without checking > in_atomic(). Yes. __fput() has a might_sleep() and it invokes file_operations::release. kernel/bpf/syscall.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 14f39c1e573ee..85159428e5fee 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -2785,20 +2785,23 @@ void bpf_link_put(struct bpf_link *link) if (!atomic64_dec_and_test(&link->refcnt)) return; - if (in_atomic()) { - INIT_WORK(&link->work, bpf_link_put_deferred); - schedule_work(&link->work); - } else { - bpf_link_free(link); - } + INIT_WORK(&link->work, bpf_link_put_deferred); + schedule_work(&link->work); } EXPORT_SYMBOL(bpf_link_put); +static void bpf_link_put_direct(struct bpf_link *link) +{ + if (!atomic64_dec_and_test(&link->refcnt)) + return; + bpf_link_free(link); +} + static int bpf_link_release(struct inode *inode, struct file *filp) { struct bpf_link *link = filp->private_data; - bpf_link_put(link); + bpf_link_put_direct(link); return 0; }