* [RFC PATCH] bpf: Fix potential call bpf_link_free() in atomic context
@ 2020-09-17 7:44 Muchun Song
2020-09-17 22:37 ` Song Liu
2020-09-21 17:29 ` Andrii Nakryiko
0 siblings, 2 replies; 6+ messages in thread
From: Muchun Song @ 2020-09-17 7:44 UTC (permalink / raw)
To: ast, daniel, kafai, songliubraving, yhs, andriin, john.fastabend,
kpsingh
Cc: netdev, bpf, Muchun Song
The in_atomic macro cannot always detect atomic context. In particular,
it cannot know about held spinlocks in non-preemptible kernels. Although,
there is no user call bpf_link_put() with holding spinlock now. Be the
safe side, we can avoid this in the feature.
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
kernel/bpf/syscall.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 178c147350f5..6347be0a5c82 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2345,12 +2345,8 @@ 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);
}
static int bpf_link_release(struct inode *inode, struct file *filp)
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [RFC PATCH] bpf: Fix potential call bpf_link_free() in atomic context
2020-09-17 7:44 [RFC PATCH] bpf: Fix potential call bpf_link_free() in atomic context Muchun Song
@ 2020-09-17 22:37 ` Song Liu
2020-09-19 5:26 ` [External] " Muchun Song
2020-09-21 17:29 ` Andrii Nakryiko
1 sibling, 1 reply; 6+ messages in thread
From: Song Liu @ 2020-09-17 22:37 UTC (permalink / raw)
To: Muchun Song
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
Networking, bpf
On Thu, Sep 17, 2020 at 12:46 AM Muchun Song <songmuchun@bytedance.com> wrote:
>
> The in_atomic macro cannot always detect atomic context. In particular,
> it cannot know about held spinlocks in non-preemptible kernels. Although,
> there is no user call bpf_link_put() with holding spinlock now. Be the
> safe side, we can avoid this in the feature.
>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
Acked-by: Song Liu <songliubraving@fb.com>
This is a little weird, but I guess that is OK, as bpf_link_put() is
not in the critical
path. Is the plan to eliminate in_atomic() (as much as possible)?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [External] Re: [RFC PATCH] bpf: Fix potential call bpf_link_free() in atomic context
2020-09-17 22:37 ` Song Liu
@ 2020-09-19 5:26 ` Muchun Song
0 siblings, 0 replies; 6+ messages in thread
From: Muchun Song @ 2020-09-19 5:26 UTC (permalink / raw)
To: Song Liu
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
Networking, bpf
On Fri, Sep 18, 2020 at 6:37 AM Song Liu <song@kernel.org> wrote:
>
> On Thu, Sep 17, 2020 at 12:46 AM Muchun Song <songmuchun@bytedance.com> wrote:
> >
> > The in_atomic macro cannot always detect atomic context. In particular,
> > it cannot know about held spinlocks in non-preemptible kernels. Although,
> > there is no user call bpf_link_put() with holding spinlock now. Be the
> > safe side, we can avoid this in the feature.
> >
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>
> Acked-by: Song Liu <songliubraving@fb.com>
>
> This is a little weird, but I guess that is OK, as bpf_link_put() is
> not in the critical
Yeah, bpf_link_put() is OK now because there is no user call it
with a holding spinlock.
> path. Is the plan to eliminate in_atomic() (as much as possible)?
Most other users of in_atomic() just for WARN_ON. It seems
there is no problem :).
--
Yours,
Muchun
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] bpf: Fix potential call bpf_link_free() in atomic context
2020-09-17 7:44 [RFC PATCH] bpf: Fix potential call bpf_link_free() in atomic context Muchun Song
2020-09-17 22:37 ` Song Liu
@ 2020-09-21 17:29 ` Andrii Nakryiko
2020-09-21 18:31 ` Andrii Nakryiko
1 sibling, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2020-09-21 17:29 UTC (permalink / raw)
To: Muchun Song
Cc: Alexei Starovoitov, Daniel Borkmann, Martin Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, john fastabend, KP Singh,
Networking, bpf
On Thu, Sep 17, 2020 at 12:46 AM Muchun Song <songmuchun@bytedance.com> wrote:
>
> The in_atomic macro cannot always detect atomic context. In particular,
> it cannot know about held spinlocks in non-preemptible kernels. Although,
> there is no user call bpf_link_put() with holding spinlock now. Be the
> safe side, we can avoid this in the feature.
>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
This change seems unnecessary (or at least premature), as if we ever
get a use case that does bpf_link_put() from under held spinlock, we
should see a warning about that (and in that case I bet code can be
rewritten to not hold spinlock during bpf_link_put()). But on the
other hand it makes bpf_link_put() to follow the pattern of
bpf_map_put(), which always defers the work, so I'm ok with this. As
Song mentioned, this is not called from a performance-critical hot
path, so doesn't matter all that much.
Acked-by: Andrii Nakryiko <andriin@fb.com>
> kernel/bpf/syscall.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 178c147350f5..6347be0a5c82 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2345,12 +2345,8 @@ 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);
> }
>
> static int bpf_link_release(struct inode *inode, struct file *filp)
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC PATCH] bpf: Fix potential call bpf_link_free() in atomic context
2020-09-21 17:29 ` Andrii Nakryiko
@ 2020-09-21 18:31 ` Andrii Nakryiko
2020-09-21 19:27 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2020-09-21 18:31 UTC (permalink / raw)
To: Muchun Song
Cc: Alexei Starovoitov, Daniel Borkmann, Martin Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, john fastabend, KP Singh,
Networking, bpf
On Mon, Sep 21, 2020 at 10:29 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Sep 17, 2020 at 12:46 AM Muchun Song <songmuchun@bytedance.com> wrote:
> >
> > The in_atomic macro cannot always detect atomic context. In particular,
> > it cannot know about held spinlocks in non-preemptible kernels. Although,
> > there is no user call bpf_link_put() with holding spinlock now. Be the
> > safe side, we can avoid this in the feature.
> >
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > ---
>
> This change seems unnecessary (or at least premature), as if we ever
> get a use case that does bpf_link_put() from under held spinlock, we
> should see a warning about that (and in that case I bet code can be
> rewritten to not hold spinlock during bpf_link_put()). But on the
> other hand it makes bpf_link_put() to follow the pattern of
> bpf_map_put(), which always defers the work, so I'm ok with this. As
> Song mentioned, this is not called from a performance-critical hot
> path, so doesn't matter all that much.
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
>
btw, you probably need to resubmit this patch as a non-RFC one for it
to be applied?..
> > kernel/bpf/syscall.c | 8 ++------
> > 1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 178c147350f5..6347be0a5c82 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -2345,12 +2345,8 @@ 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);
> > }
> >
> > static int bpf_link_release(struct inode *inode, struct file *filp)
> > --
> > 2.20.1
> >
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC PATCH] bpf: Fix potential call bpf_link_free() in atomic context
2020-09-21 18:31 ` Andrii Nakryiko
@ 2020-09-21 19:27 ` Daniel Borkmann
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2020-09-21 19:27 UTC (permalink / raw)
To: Andrii Nakryiko, Muchun Song
Cc: Alexei Starovoitov, Martin Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, john fastabend, KP Singh, Networking, bpf
On 9/21/20 8:31 PM, Andrii Nakryiko wrote:
> On Mon, Sep 21, 2020 at 10:29 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> On Thu, Sep 17, 2020 at 12:46 AM Muchun Song <songmuchun@bytedance.com> wrote:
>>>
>>> The in_atomic macro cannot always detect atomic context. In particular,
>>> it cannot know about held spinlocks in non-preemptible kernels. Although,
>>> there is no user call bpf_link_put() with holding spinlock now. Be the
>>> safe side, we can avoid this in the feature.
>>>
>>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>>> ---
>>
>> This change seems unnecessary (or at least premature), as if we ever
>> get a use case that does bpf_link_put() from under held spinlock, we
>> should see a warning about that (and in that case I bet code can be
>> rewritten to not hold spinlock during bpf_link_put()). But on the
>> other hand it makes bpf_link_put() to follow the pattern of
>> bpf_map_put(), which always defers the work, so I'm ok with this. As
>> Song mentioned, this is not called from a performance-critical hot
>> path, so doesn't matter all that much.
>>
>> Acked-by: Andrii Nakryiko <andriin@fb.com>
Agree, SGTM.
> btw, you probably need to resubmit this patch as a non-RFC one for it
> to be applied?..
Given first time BPF contributor & it has already several ACKs, I took it
into bpf-next, thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-21 19:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-17 7:44 [RFC PATCH] bpf: Fix potential call bpf_link_free() in atomic context Muchun Song
2020-09-17 22:37 ` Song Liu
2020-09-19 5:26 ` [External] " Muchun Song
2020-09-21 17:29 ` Andrii Nakryiko
2020-09-21 18:31 ` Andrii Nakryiko
2020-09-21 19:27 ` Daniel Borkmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.