| Message ID | 20230614083430.oENawF8f@linutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ab5d47bd41b1db82c295b0e751e2b822b43a4b5a |
| Delegated to: | BPF |
| Headers | show |
| Series | [v4] bpf: Remove in_atomic() from bpf_link_put(). | expand |
On Wed, Jun 14, 2023 at 10:34:30AM +0200, Sebastian Andrzej Siewior wrote: > 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(). Just to make sure that I understand, you are proposing that the RCU callbacks continue to run with BH disabled, but that BH-disabled regions are preemptible in kernels built with CONFIG_PREEMPT_RT=y? Or did I miss a turn in there somewhere? Thanx, Paul > It was pointed out that the bpf_link_put() invocation should not be > delayed if originated from close(). It was also pointed out that other > invocations from within a syscall should also avoid the workqueue. > Everyone else should use workqueue by default to remain safe in the > future (while auditing the code, every caller was preemptible except for > the RCU case). > > Let bpf_link_put() use the worker unconditionally. Add > bpf_link_put_direct() which will directly free the resources and is used > by close() and from within __sys_bpf(). > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > v3…v4: > - Revert back to bpf_link_put_direct() to the direct free and let > bpf_link_put() use the worker. Let close() and all invocations from > within the syscall use bpf_link_put_direct() which are all instances > within syscall.c here. > > v2…v3: > - Drop bpf_link_put_direct(). Let bpf_link_put() do the direct free > and add bpf_link_put_from_atomic() to do the delayed free via the > worker. > > v1…v2: > - Add bpf_link_put_direct() to be used from bpf_link_release() as > suggested. > > kernel/bpf/syscall.c | 29 ++++++++++++++++------------- > 1 file changed, 16 insertions(+), 13 deletions(-) > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 14f39c1e573ee..8f09aef5949d4 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -2777,28 +2777,31 @@ static void bpf_link_put_deferred(struct work_struct *work) > bpf_link_free(link); > } > > -/* bpf_link_put can be called from atomic context, but ensures that resources > - * are freed from process context > +/* bpf_link_put might be called from atomic context. It needs to be called > + * from sleepable context in order to acquire sleeping locks during the process. > */ > 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; > } > > @@ -4764,7 +4767,7 @@ static int link_update(union bpf_attr *attr) > if (ret) > bpf_prog_put(new_prog); > out_put_link: > - bpf_link_put(link); > + bpf_link_put_direct(link); > return ret; > } > > @@ -4787,7 +4790,7 @@ static int link_detach(union bpf_attr *attr) > else > ret = -EOPNOTSUPP; > > - bpf_link_put(link); > + bpf_link_put_direct(link); > return ret; > } > > @@ -4857,7 +4860,7 @@ static int bpf_link_get_fd_by_id(const union bpf_attr *attr) > > fd = bpf_link_new_fd(link); > if (fd < 0) > - bpf_link_put(link); > + bpf_link_put_direct(link); > > return fd; > } > @@ -4934,7 +4937,7 @@ static int bpf_iter_create(union bpf_attr *attr) > return PTR_ERR(link); > > err = bpf_iter_new_fd(link); > - bpf_link_put(link); > + bpf_link_put_direct(link); > > return err; > } > -- > 2.40.1 >
On 2023-06-15 09:43:11 [-0700], Paul E. McKenney wrote: > On Wed, Jun 14, 2023 at 10:34:30AM +0200, Sebastian Andrzej Siewior wrote: > > 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(). > > Just to make sure that I understand, you are proposing that the RCU > callbacks continue to run with BH disabled, but that BH-disabled regions > are preemptible in kernels built with CONFIG_PREEMPT_RT=y? > > Or did I miss a turn in there somewhere? I'm not proposing anything, just stating what we have. On PREEMPT_RT you are preemptible within the RCU callback but must not invoke schedule(). Similar to the RCU read section on CONFIG_PREEMPT where you are preemptible but must not invoke schedule(). > > Thanx, Paul Sebastian
On Thu, Jun 15, 2023 at 09:13:41PM +0200, Sebastian Andrzej Siewior wrote: > On 2023-06-15 09:43:11 [-0700], Paul E. McKenney wrote: > > On Wed, Jun 14, 2023 at 10:34:30AM +0200, Sebastian Andrzej Siewior wrote: > > > 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(). > > > > Just to make sure that I understand, you are proposing that the RCU > > callbacks continue to run with BH disabled, but that BH-disabled regions > > are preemptible in kernels built with CONFIG_PREEMPT_RT=y? > > > > Or did I miss a turn in there somewhere? > > I'm not proposing anything, just stating what we have. On PREEMPT_RT > you are preemptible within the RCU callback but must not invoke > schedule(). Similar to the RCU read section on CONFIG_PREEMPT where you > are preemptible but must not invoke schedule(). Thank you for the clarification! The main risk of preemptible RCU callbacks is callback flooding, but RCU priority boosting should take care of that. Thanx, Paul
Hello: This patch was applied to bpf/bpf-next.git (master) by Andrii Nakryiko <andrii@kernel.org>: On Wed, 14 Jun 2023 10:34:30 +0200 you wrote: > 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(). > > [...] Here is the summary with links: - [v4] bpf: Remove in_atomic() from bpf_link_put(). https://git.kernel.org/bpf/bpf-next/c/ab5d47bd41b1 You are awesome, thank you!
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 14f39c1e573ee..8f09aef5949d4 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -2777,28 +2777,31 @@ static void bpf_link_put_deferred(struct work_struct *work) bpf_link_free(link); } -/* bpf_link_put can be called from atomic context, but ensures that resources - * are freed from process context +/* bpf_link_put might be called from atomic context. It needs to be called + * from sleepable context in order to acquire sleeping locks during the process. */ 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; } @@ -4764,7 +4767,7 @@ static int link_update(union bpf_attr *attr) if (ret) bpf_prog_put(new_prog); out_put_link: - bpf_link_put(link); + bpf_link_put_direct(link); return ret; } @@ -4787,7 +4790,7 @@ static int link_detach(union bpf_attr *attr) else ret = -EOPNOTSUPP; - bpf_link_put(link); + bpf_link_put_direct(link); return ret; } @@ -4857,7 +4860,7 @@ static int bpf_link_get_fd_by_id(const union bpf_attr *attr) fd = bpf_link_new_fd(link); if (fd < 0) - bpf_link_put(link); + bpf_link_put_direct(link); return fd; } @@ -4934,7 +4937,7 @@ static int bpf_iter_create(union bpf_attr *attr) return PTR_ERR(link); err = bpf_iter_new_fd(link); - bpf_link_put(link); + bpf_link_put_direct(link); return err; }
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(). It was also pointed out that other invocations from within a syscall should also avoid the workqueue. Everyone else should use workqueue by default to remain safe in the future (while auditing the code, every caller was preemptible except for the RCU case). Let bpf_link_put() use the worker unconditionally. Add bpf_link_put_direct() which will directly free the resources and is used by close() and from within __sys_bpf(). Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- v3…v4: - Revert back to bpf_link_put_direct() to the direct free and let bpf_link_put() use the worker. Let close() and all invocations from within the syscall use bpf_link_put_direct() which are all instances within syscall.c here. v2…v3: - Drop bpf_link_put_direct(). Let bpf_link_put() do the direct free and add bpf_link_put_from_atomic() to do the delayed free via the worker. v1…v2: - Add bpf_link_put_direct() to be used from bpf_link_release() as suggested. kernel/bpf/syscall.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-)