netdev - Re: [PATCH bpf-next] bpf: abstract away entire bpf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2d6ae192-fe22-0239-54c7-142ec21b7794@iogearbox.net>
Date:   Fri, 13 Mar 2020 00:22:58 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Andrii Nakryiko <andriin@...com>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, ast@...com
Cc:     andrii.nakryiko@...il.com, kernel-team@...com
Subject: Re: [PATCH bpf-next] bpf: abstract away entire bpf_link clean up
 procedure

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@...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;
>   	}
>   
>