* [PATCH bpf-next] bpf: abstract away entire bpf_link clean up procedure
@ 2020-03-12 20:39 Andrii Nakryiko
2020-03-12 23:22 ` Daniel Borkmann
2020-03-12 23:38 ` Martin KaFai Lau
0 siblings, 2 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2020-03-12 20:39 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
Instead of requiring users to do three steps for cleaning up bpf_link, its
anon_inode file, and unused fd, abstract that away into bpf_link_cleanup()
helper. bpf_link_defunct() is removed, as it shouldn't be needed as an
individual operation anymore.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
include/linux/bpf.h | 3 ++-
kernel/bpf/syscall.c | 18 +++++++++++-------
2 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 4fd91b7c95ea..358f3eb07c01 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1075,7 +1075,8 @@ struct bpf_link_ops {
void bpf_link_init(struct bpf_link *link, const struct bpf_link_ops *ops,
struct bpf_prog *prog);
-void bpf_link_defunct(struct bpf_link *link);
+void bpf_link_cleanup(struct bpf_link *link, struct file *link_file,
+ int link_fd);
void bpf_link_inc(struct bpf_link *link);
void bpf_link_put(struct bpf_link *link);
int bpf_link_new_fd(struct bpf_link *link);
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index b2f73ecacced..d2f49ae225b0 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2188,9 +2188,17 @@ void bpf_link_init(struct bpf_link *link, const struct bpf_link_ops *ops,
link->prog = prog;
}
-void bpf_link_defunct(struct bpf_link *link)
+/* Clean up bpf_link and corresponding anon_inode file and FD. After
+ * anon_inode is created, bpf_link can't be just kfree()'d due to deferred
+ * anon_inode's release() call. This helper manages marking bpf_link as
+ * defunct, releases anon_inode file and puts reserved FD.
+ */
+void bpf_link_cleanup(struct bpf_link *link, struct file *link_file,
+ int link_fd)
{
link->prog = NULL;
+ fput(link_file);
+ put_unused_fd(link_fd);
}
void bpf_link_inc(struct bpf_link *link)
@@ -2383,9 +2391,7 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
err = bpf_trampoline_link_prog(prog);
if (err) {
- bpf_link_defunct(&link->link);
- fput(link_file);
- put_unused_fd(link_fd);
+ bpf_link_cleanup(&link->link, link_file, link_fd);
goto out_put_prog;
}
@@ -2498,9 +2504,7 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
err = bpf_probe_register(link->btp, prog);
if (err) {
- bpf_link_defunct(&link->link);
- fput(link_file);
- put_unused_fd(link_fd);
+ bpf_link_cleanup(&link->link, link_file, link_fd);
goto out_put_btp;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf: abstract away entire bpf_link clean up procedure
2020-03-12 20:39 [PATCH bpf-next] bpf: abstract away entire bpf_link clean up procedure Andrii Nakryiko
@ 2020-03-12 23:22 ` Daniel Borkmann
2020-03-12 23:27 ` Andrii Nakryiko
2020-03-12 23:38 ` Martin KaFai Lau
1 sibling, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2020-03-12 23:22 UTC (permalink / raw)
To: Andrii Nakryiko, bpf, netdev, ast; +Cc: andrii.nakryiko, kernel-team
On 3/12/20 9:39 PM, Andrii Nakryiko wrote:
> Instead of requiring users to do three steps for cleaning up bpf_link, its
> anon_inode file, and unused fd, abstract that away into bpf_link_cleanup()
> helper. bpf_link_defunct() is removed, as it shouldn't be needed as an
> individual operation anymore.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> include/linux/bpf.h | 3 ++-
> kernel/bpf/syscall.c | 18 +++++++++++-------
> 2 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 4fd91b7c95ea..358f3eb07c01 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1075,7 +1075,8 @@ struct bpf_link_ops {
>
> void bpf_link_init(struct bpf_link *link, const struct bpf_link_ops *ops,
> struct bpf_prog *prog);
> -void bpf_link_defunct(struct bpf_link *link);
> +void bpf_link_cleanup(struct bpf_link *link, struct file *link_file,
> + int link_fd);
> void bpf_link_inc(struct bpf_link *link);
> void bpf_link_put(struct bpf_link *link);
> int bpf_link_new_fd(struct bpf_link *link);
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index b2f73ecacced..d2f49ae225b0 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2188,9 +2188,17 @@ void bpf_link_init(struct bpf_link *link, const struct bpf_link_ops *ops,
> link->prog = prog;
> }
>
> -void bpf_link_defunct(struct bpf_link *link)
> +/* Clean up bpf_link and corresponding anon_inode file and FD. After
> + * anon_inode is created, bpf_link can't be just kfree()'d due to deferred
> + * anon_inode's release() call. This helper manages marking bpf_link as
> + * defunct, releases anon_inode file and puts reserved FD.
> + */
> +void bpf_link_cleanup(struct bpf_link *link, struct file *link_file,
> + int link_fd)
Looks good, but given it is only used here this should be static instead.
> {
> link->prog = NULL;
> + fput(link_file);
> + put_unused_fd(link_fd);
> }
>
> void bpf_link_inc(struct bpf_link *link)
> @@ -2383,9 +2391,7 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
>
> err = bpf_trampoline_link_prog(prog);
> if (err) {
> - bpf_link_defunct(&link->link);
> - fput(link_file);
> - put_unused_fd(link_fd);
> + bpf_link_cleanup(&link->link, link_file, link_fd);
> goto out_put_prog;
> }
>
> @@ -2498,9 +2504,7 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
>
> err = bpf_probe_register(link->btp, prog);
> if (err) {
> - bpf_link_defunct(&link->link);
> - fput(link_file);
> - put_unused_fd(link_fd);
> + bpf_link_cleanup(&link->link, link_file, link_fd);
> goto out_put_btp;
> }
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf: abstract away entire bpf_link clean up procedure
2020-03-12 23:22 ` Daniel Borkmann
@ 2020-03-12 23:27 ` Andrii Nakryiko
2020-03-12 23:41 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2020-03-12 23:27 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov, Kernel Team
On Thu, Mar 12, 2020 at 4:23 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 3/12/20 9:39 PM, Andrii Nakryiko wrote:
> > Instead of requiring users to do three steps for cleaning up bpf_link, its
> > anon_inode file, and unused fd, abstract that away into bpf_link_cleanup()
> > helper. bpf_link_defunct() is removed, as it shouldn't be needed as an
> > individual operation anymore.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > include/linux/bpf.h | 3 ++-
> > kernel/bpf/syscall.c | 18 +++++++++++-------
> > 2 files changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 4fd91b7c95ea..358f3eb07c01 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -1075,7 +1075,8 @@ struct bpf_link_ops {
> >
> > void bpf_link_init(struct bpf_link *link, const struct bpf_link_ops *ops,
> > struct bpf_prog *prog);
> > -void bpf_link_defunct(struct bpf_link *link);
> > +void bpf_link_cleanup(struct bpf_link *link, struct file *link_file,
> > + int link_fd);
> > void bpf_link_inc(struct bpf_link *link);
> > void bpf_link_put(struct bpf_link *link);
> > int bpf_link_new_fd(struct bpf_link *link);
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index b2f73ecacced..d2f49ae225b0 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -2188,9 +2188,17 @@ void bpf_link_init(struct bpf_link *link, const struct bpf_link_ops *ops,
> > link->prog = prog;
> > }
> >
> > -void bpf_link_defunct(struct bpf_link *link)
> > +/* Clean up bpf_link and corresponding anon_inode file and FD. After
> > + * anon_inode is created, bpf_link can't be just kfree()'d due to deferred
> > + * anon_inode's release() call. This helper manages marking bpf_link as
> > + * defunct, releases anon_inode file and puts reserved FD.
> > + */
> > +void bpf_link_cleanup(struct bpf_link *link, struct file *link_file,
> > + int link_fd)
>
> Looks good, but given it is only used here this should be static instead.
>
This is part of bpf_link internal API. I have patches locally for
cgroup bpf_link that use this for clean up as well already, other
bpf_link types will also use this.
> > {
> > link->prog = NULL;
> > + fput(link_file);
> > + put_unused_fd(link_fd);
> > }
> >
> > void bpf_link_inc(struct bpf_link *link)
> > @@ -2383,9 +2391,7 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
> >
> > err = bpf_trampoline_link_prog(prog);
> > if (err) {
> > - bpf_link_defunct(&link->link);
> > - fput(link_file);
> > - put_unused_fd(link_fd);
> > + bpf_link_cleanup(&link->link, link_file, link_fd);
> > goto out_put_prog;
> > }
> >
> > @@ -2498,9 +2504,7 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
> >
> > err = bpf_probe_register(link->btp, prog);
> > if (err) {
> > - bpf_link_defunct(&link->link);
> > - fput(link_file);
> > - put_unused_fd(link_fd);
> > + bpf_link_cleanup(&link->link, link_file, link_fd);
> > goto out_put_btp;
> > }
> >
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf: abstract away entire bpf_link clean up procedure
2020-03-12 23:27 ` Andrii Nakryiko
@ 2020-03-12 23:41 ` Daniel Borkmann
2020-03-12 23:47 ` Andrii Nakryiko
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2020-03-12 23:41 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov, Kernel Team
On 3/13/20 12:27 AM, Andrii Nakryiko wrote:
> On Thu, Mar 12, 2020 at 4:23 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 3/12/20 9:39 PM, Andrii Nakryiko wrote:
>>> Instead of requiring users to do three steps for cleaning up bpf_link, its
>>> anon_inode file, and unused fd, abstract that away into bpf_link_cleanup()
>>> helper. bpf_link_defunct() is removed, as it shouldn't be needed as an
>>> individual operation anymore.
>>>
>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>>> ---
>>> include/linux/bpf.h | 3 ++-
>>> kernel/bpf/syscall.c | 18 +++++++++++-------
>>> 2 files changed, 13 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>> index 4fd91b7c95ea..358f3eb07c01 100644
>>> --- a/include/linux/bpf.h
>>> +++ b/include/linux/bpf.h
>>> @@ -1075,7 +1075,8 @@ struct bpf_link_ops {
>>>
>>> void bpf_link_init(struct bpf_link *link, const struct bpf_link_ops *ops,
>>> struct bpf_prog *prog);
>>> -void bpf_link_defunct(struct bpf_link *link);
>>> +void bpf_link_cleanup(struct bpf_link *link, struct file *link_file,
>>> + int link_fd);
>>> void bpf_link_inc(struct bpf_link *link);
>>> void bpf_link_put(struct bpf_link *link);
>>> int bpf_link_new_fd(struct bpf_link *link);
>>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>>> index b2f73ecacced..d2f49ae225b0 100644
>>> --- a/kernel/bpf/syscall.c
>>> +++ b/kernel/bpf/syscall.c
>>> @@ -2188,9 +2188,17 @@ void bpf_link_init(struct bpf_link *link, const struct bpf_link_ops *ops,
>>> link->prog = prog;
>>> }
>>>
>>> -void bpf_link_defunct(struct bpf_link *link)
>>> +/* Clean up bpf_link and corresponding anon_inode file and FD. After
>>> + * anon_inode is created, bpf_link can't be just kfree()'d due to deferred
>>> + * anon_inode's release() call. This helper manages marking bpf_link as
>>> + * defunct, releases anon_inode file and puts reserved FD.
>>> + */
>>> +void bpf_link_cleanup(struct bpf_link *link, struct file *link_file,
>>> + int link_fd)
>>
>> Looks good, but given it is only used here this should be static instead.
>
> This is part of bpf_link internal API. I have patches locally for
> cgroup bpf_link that use this for clean up as well already, other
> bpf_link types will also use this.
Meaning it's a logical part of your future series. When you added the bpf_link_*
stuff only the symbols should have been in bpf.h that are actually used in the
tree outside of syscall.c, and when you extend the series in future /then/ we
can export more as needed, so everything is kept self-contained. This is common
practice.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf: abstract away entire bpf_link clean up procedure
2020-03-12 23:41 ` Daniel Borkmann
@ 2020-03-12 23:47 ` Andrii Nakryiko
0 siblings, 0 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2020-03-12 23:47 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov, Kernel Team
On Thu, Mar 12, 2020 at 4:41 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 3/13/20 12:27 AM, Andrii Nakryiko wrote:
> > On Thu, Mar 12, 2020 at 4:23 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >> On 3/12/20 9:39 PM, Andrii Nakryiko wrote:
> >>> Instead of requiring users to do three steps for cleaning up bpf_link, its
> >>> anon_inode file, and unused fd, abstract that away into bpf_link_cleanup()
> >>> helper. bpf_link_defunct() is removed, as it shouldn't be needed as an
> >>> individual operation anymore.
> >>>
> >>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> >>> ---
> >>> include/linux/bpf.h | 3 ++-
> >>> kernel/bpf/syscall.c | 18 +++++++++++-------
> >>> 2 files changed, 13 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> >>> index 4fd91b7c95ea..358f3eb07c01 100644
> >>> --- a/include/linux/bpf.h
> >>> +++ b/include/linux/bpf.h
> >>> @@ -1075,7 +1075,8 @@ struct bpf_link_ops {
> >>>
> >>> void bpf_link_init(struct bpf_link *link, const struct bpf_link_ops *ops,
> >>> struct bpf_prog *prog);
> >>> -void bpf_link_defunct(struct bpf_link *link);
> >>> +void bpf_link_cleanup(struct bpf_link *link, struct file *link_file,
> >>> + int link_fd);
> >>> void bpf_link_inc(struct bpf_link *link);
> >>> void bpf_link_put(struct bpf_link *link);
> >>> int bpf_link_new_fd(struct bpf_link *link);
> >>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> >>> index b2f73ecacced..d2f49ae225b0 100644
> >>> --- a/kernel/bpf/syscall.c
> >>> +++ b/kernel/bpf/syscall.c
> >>> @@ -2188,9 +2188,17 @@ void bpf_link_init(struct bpf_link *link, const struct bpf_link_ops *ops,
> >>> link->prog = prog;
> >>> }
> >>>
> >>> -void bpf_link_defunct(struct bpf_link *link)
> >>> +/* Clean up bpf_link and corresponding anon_inode file and FD. After
> >>> + * anon_inode is created, bpf_link can't be just kfree()'d due to deferred
> >>> + * anon_inode's release() call. This helper manages marking bpf_link as
> >>> + * defunct, releases anon_inode file and puts reserved FD.
> >>> + */
> >>> +void bpf_link_cleanup(struct bpf_link *link, struct file *link_file,
> >>> + int link_fd)
> >>
> >> Looks good, but given it is only used here this should be static instead.
> >
> > This is part of bpf_link internal API. I have patches locally for
> > cgroup bpf_link that use this for clean up as well already, other
> > bpf_link types will also use this.
>
> Meaning it's a logical part of your future series. When you added the bpf_link_*
> stuff only the symbols should have been in bpf.h that are actually used in the
> tree outside of syscall.c, and when you extend the series in future /then/ we
> can export more as needed, so everything is kept self-contained. This is common
> practice.
Alright, will remove in v2.
>
> Thanks,
> Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf: abstract away entire bpf_link clean up procedure
2020-03-12 20:39 [PATCH bpf-next] bpf: abstract away entire bpf_link clean up procedure Andrii Nakryiko
2020-03-12 23:22 ` Daniel Borkmann
@ 2020-03-12 23:38 ` Martin KaFai Lau
1 sibling, 0 replies; 6+ messages in thread
From: Martin KaFai Lau @ 2020-03-12 23:38 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, netdev, ast, daniel, andrii.nakryiko, kernel-team
On Thu, Mar 12, 2020 at 01:39:14PM -0700, Andrii Nakryiko wrote:
> Instead of requiring users to do three steps for cleaning up bpf_link, its
> anon_inode file, and unused fd, abstract that away into bpf_link_cleanup()
> helper. bpf_link_defunct() is removed, as it shouldn't be needed as an
> individual operation anymore.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> include/linux/bpf.h | 3 ++-
> kernel/bpf/syscall.c | 18 +++++++++++-------
Changes are only in syscall.c. Should bpf_link_cleanup() be static?
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-12 23:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-12 20:39 [PATCH bpf-next] bpf: abstract away entire bpf_link clean up procedure Andrii Nakryiko
2020-03-12 23:22 ` Daniel Borkmann
2020-03-12 23:27 ` Andrii Nakryiko
2020-03-12 23:41 ` Daniel Borkmann
2020-03-12 23:47 ` Andrii Nakryiko
2020-03-12 23:38 ` Martin KaFai Lau
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).