bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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 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
* 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
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).