netdev - Re: [PATCH v3 bpf-next 02/15] bpf: net: Avoid sk_setsockopt() taking sk lock when called from 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>] [day] [month] [year] [list]

Date:   Mon, 15 Aug 2022 20:32:00 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        David Miller <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>, kernel-team@...com,
        Paolo Abeni <pabeni@...hat.com>,
        Stanislav Fomichev <sdf@...gle.com>
Subject: Re: [PATCH v3 bpf-next 02/15] bpf: net: Avoid sk_setsockopt() taking
 sk lock when called from bpf

On Wed, Aug 10, 2022 at 12:10 PM Martin KaFai Lau <kafai@...com> wrote:
>
> Most of the code in bpf_setsockopt(SOL_SOCKET) are duplicated from
> the sk_setsockopt().  The number of supported optnames are
> increasing ever and so as the duplicated code.
>
> One issue in reusing sk_setsockopt() is that the bpf prog
> has already acquired the sk lock.  This patch adds a
> has_current_bpf_ctx() to tell if the sk_setsockopt() is called from
> a bpf prog.  The bpf prog calling bpf_setsockopt() is either running
> in_task() or in_serving_softirq().  Both cases have the current->bpf_ctx
> initialized.  Thus, the has_current_bpf_ctx() only needs to
> test !!current->bpf_ctx.
>
> This patch also adds sockopt_{lock,release}_sock() helpers
> for sk_setsockopt() to use.  These helpers will test
> has_current_bpf_ctx() before acquiring/releasing the lock.  They are
> in EXPORT_SYMBOL for the ipv6 module to use in a latter patch.
>
> Note on the change in sock_setbindtodevice().  sockopt_lock_sock()
> is done in sock_setbindtodevice() instead of doing the lock_sock
> in sock_bindtoindex(..., lock_sk = true).
>
> Signed-off-by: Martin KaFai Lau <kafai@...com>
> ---
>  include/linux/bpf.h | 14 ++++++++++++++
>  include/net/sock.h  |  3 +++
>  net/core/sock.c     | 30 +++++++++++++++++++++++++++---
>  3 files changed, 44 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index a627a02cf8ab..0a600b2013cc 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1966,6 +1966,16 @@ static inline bool unprivileged_ebpf_enabled(void)
>         return !sysctl_unprivileged_bpf_disabled;
>  }
>
> +/* Not all bpf prog type has the bpf_ctx.
> + * Only trampoline and cgroup-bpf have it.

this is not true already (perf_event and kprobe/uprobe/tp progs have
bpf_ctx as well) and can easily get out of sync in the future, so I'd
drop the list of types that support bpf_ctx.

> + * For the bpf prog type that has initialized the bpf_ctx,
> + * this function can be used to decide if a kernel function
> + * is called by a bpf program.
> + */
> +static inline bool has_current_bpf_ctx(void)
> +{
> +       return !!current->bpf_ctx;
> +}
>  #else /* !CONFIG_BPF_SYSCALL */

[...]