Re: [PATCH v3 bpf-next 00/15] bpf: net: Remove duplicated code from bpf_setsockopt()

Re: [PATCH v3 bpf-next 00/15] bpf: net: Remove duplicated code from bpf_setsockopt()

[sdf]

On 08/10, Martin KaFai Lau wrote:
> The code in bpf_setsockopt() is mostly a copy-and-paste from
> the sock_setsockopt(), do_tcp_setsockopt(), do_ipv6_setsockopt(),
> and do_ip_setsockopt().  As the allowed optnames in bpf_setsockopt()
> grows, so are the duplicated code.  The code between the copies
> also slowly drifted.

> This set is an effort to clean this up and reuse the existing
> {sock,do_tcp,do_ipv6,do_ip}_setsockopt() as much as possible.

> After the clean up, this set also adds a few allowed optnames
> that we need to the bpf_setsockopt().

> The initial attempt was to clean up both bpf_setsockopt() and
> bpf_getsockopt() together.  However, the patch set was getting
> too long.  It is beneficial to leave the bpf_getsockopt()
> out for another patch set.  Thus, this set is focusing
> on the bpf_setsockopt().

> v3:
> - s/in_bpf/has_current_bpf_ctx/ (Andrii)
> - Add comments to has_current_bpf_ctx() and sockopt_lock_sock()
>    (Stanislav)
> - Use vmlinux.h in selftest and add defines to bpf_tracing_net.h
>    (Stanislav)
> - Use bpf_getsockopt(SO_MARK) in selftest (Stanislav)
> - Use BPF_CORE_READ_BITFIELD in selftest (Yonghong)

Reviewed-by: Stanislav Fomichev <sdf@google.com>

(I didn't go super deep on the selftest)

Re: [PATCH v3 bpf-next 00/15] bpf: net: Remove duplicated code from bpf_setsockopt()

[Daniel Borkmann]

On 8/11/22 7:04 PM, sdf@google.com wrote:
> On 08/10, Martin KaFai Lau wrote:
>> The code in bpf_setsockopt() is mostly a copy-and-paste from
>> the sock_setsockopt(), do_tcp_setsockopt(), do_ipv6_setsockopt(),
>> and do_ip_setsockopt().  As the allowed optnames in bpf_setsockopt()
>> grows, so are the duplicated code.  The code between the copies
>> also slowly drifted.
> 
>> This set is an effort to clean this up and reuse the existing
>> {sock,do_tcp,do_ipv6,do_ip}_setsockopt() as much as possible.
> 
>> After the clean up, this set also adds a few allowed optnames
>> that we need to the bpf_setsockopt().
> 
>> The initial attempt was to clean up both bpf_setsockopt() and
>> bpf_getsockopt() together.  However, the patch set was getting
>> too long.  It is beneficial to leave the bpf_getsockopt()
>> out for another patch set.  Thus, this set is focusing
>> on the bpf_setsockopt().
> 
>> v3:
>> - s/in_bpf/has_current_bpf_ctx/ (Andrii)
>> - Add comments to has_current_bpf_ctx() and sockopt_lock_sock()
>>    (Stanislav)
>> - Use vmlinux.h in selftest and add defines to bpf_tracing_net.h
>>    (Stanislav)
>> - Use bpf_getsockopt(SO_MARK) in selftest (Stanislav)
>> - Use BPF_CORE_READ_BITFIELD in selftest (Yonghong)
> 
> Reviewed-by: Stanislav Fomichev <sdf@google.com>
> 
> (I didn't go super deep on the selftest)

Looks like that one throws a build error, fwiw:

https://github.com/kernel-patches/bpf/runs/7844497492?check_suite_focus=true

   [...]
     CLNG-BPF [test_maps] kfunc_call_test_subprog.o
     CLNG-BPF [test_maps] bpf_iter_test_kern6.o
   progs/setget_sockopt.c:39:33: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion]
           { .opt = SO_REUSEADDR, .flip = 1, },
                                          ^
   progs/setget_sockopt.c:42:33: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion]
           { .opt = SO_KEEPALIVE, .flip = 1, },
                                          ^
   progs/setget_sockopt.c:44:33: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion]
           { .opt = SO_REUSEPORT, .flip = 1, },
                                          ^
     CLNG-BPF [test_maps] btf__core_reloc_type_id.o
   progs/setget_sockopt.c:48:32: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion]
           { .opt = SO_TXREHASH, .flip = 1, },
                                         ^
   progs/setget_sockopt.c:53:32: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion]
           { .opt = TCP_NODELAY, .flip = 1, },
                                         ^
   progs/setget_sockopt.c:61:45: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion]
           { .opt = TCP_THIN_LINEAR_TIMEOUTS, .flip = 1, },
                                                      ^
   progs/setget_sockopt.c:75:39: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion]
           { .opt = IPV6_AUTOFLOWLABEL, .flip = 1, },
                                                ^
   7 errors generated.
   make: *** [Makefile:521: /tmp/runner/work/bpf/bpf/tools/testing/selftests/bpf/setget_sockopt.o] Error 1
   make: *** Waiting for unfinished jobs....
   make: Leaving directory '/tmp/runner/work/bpf/bpf/tools/testing/selftests/bpf'
   Error: Process completed with exit code 2.

Re: [PATCH v3 bpf-next 02/15] bpf: net: Avoid sk_setsockopt() taking sk lock when called from bpf

[Andrii Nakryiko]

On Wed, Aug 10, 2022 at 12:10 PM Martin KaFai Lau <kafai@fb.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@fb.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 */

[...]

Re: [PATCH v3 bpf-next 07/15] bpf: Initialize the bpf_run_ctx in bpf_iter_run_prog()

[Andrii Nakryiko]

On Wed, Aug 10, 2022 at 12:11 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> The bpf-iter-prog for tcp and unix sk can do bpf_setsockopt()
> which needs has_current_bpf_ctx() to decide if it is called by a
> bpf prog.  This patch initializes the bpf_run_ctx in
> bpf_iter_run_prog() for the has_current_bpf_ctx() to use.
>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
>  include/linux/bpf.h   | 2 +-
>  kernel/bpf/bpf_iter.c | 5 +++++
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 0a600b2013cc..15ab980e9525 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1967,7 +1967,7 @@ static inline bool unprivileged_ebpf_enabled(void)
>  }
>
>  /* Not all bpf prog type has the bpf_ctx.
> - * Only trampoline and cgroup-bpf have it.
> + * Only trampoline, cgroup-bpf, and iter have it.

Apart from this part which I'd drop, lgtm:

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>   * 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.
> diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
> index 4b112aa8bba3..6476b2c03527 100644
> --- a/kernel/bpf/bpf_iter.c
> +++ b/kernel/bpf/bpf_iter.c
> @@ -685,19 +685,24 @@ struct bpf_prog *bpf_iter_get_info(struct bpf_iter_meta *meta, bool in_stop)
>
>  int bpf_iter_run_prog(struct bpf_prog *prog, void *ctx)
>  {
> +       struct bpf_run_ctx run_ctx, *old_run_ctx;
>         int ret;
>
>         if (prog->aux->sleepable) {
>                 rcu_read_lock_trace();
>                 migrate_disable();
>                 might_fault();
> +               old_run_ctx = bpf_set_run_ctx(&run_ctx);
>                 ret = bpf_prog_run(prog, ctx);
> +               bpf_reset_run_ctx(old_run_ctx);
>                 migrate_enable();
>                 rcu_read_unlock_trace();
>         } else {
>                 rcu_read_lock();
>                 migrate_disable();
> +               old_run_ctx = bpf_set_run_ctx(&run_ctx);
>                 ret = bpf_prog_run(prog, ctx);
> +               bpf_reset_run_ctx(old_run_ctx);
>                 migrate_enable();
>                 rcu_read_unlock();
>         }
> --
> 2.30.2
>

Re: [PATCH v3 bpf-next 00/15] bpf: net: Remove duplicated code from bpf_setsockopt()

[Martin KaFai Lau]

On Tue, Aug 16, 2022 at 12:04:52AM +0200, Daniel Borkmann wrote:
> On 8/11/22 7:04 PM, sdf@google.com wrote:
> > On 08/10, Martin KaFai Lau wrote:
> > > The code in bpf_setsockopt() is mostly a copy-and-paste from
> > > the sock_setsockopt(), do_tcp_setsockopt(), do_ipv6_setsockopt(),
> > > and do_ip_setsockopt().  As the allowed optnames in bpf_setsockopt()
> > > grows, so are the duplicated code.  The code between the copies
> > > also slowly drifted.
> > 
> > > This set is an effort to clean this up and reuse the existing
> > > {sock,do_tcp,do_ipv6,do_ip}_setsockopt() as much as possible.
> > 
> > > After the clean up, this set also adds a few allowed optnames
> > > that we need to the bpf_setsockopt().
> > 
> > > The initial attempt was to clean up both bpf_setsockopt() and
> > > bpf_getsockopt() together.  However, the patch set was getting
> > > too long.  It is beneficial to leave the bpf_getsockopt()
> > > out for another patch set.  Thus, this set is focusing
> > > on the bpf_setsockopt().
> > 
> > > v3:
> > > - s/in_bpf/has_current_bpf_ctx/ (Andrii)
> > > - Add comments to has_current_bpf_ctx() and sockopt_lock_sock()
> > >    (Stanislav)
> > > - Use vmlinux.h in selftest and add defines to bpf_tracing_net.h
> > >    (Stanislav)
> > > - Use bpf_getsockopt(SO_MARK) in selftest (Stanislav)
> > > - Use BPF_CORE_READ_BITFIELD in selftest (Yonghong)
> > 
> > Reviewed-by: Stanislav Fomichev <sdf@google.com>
> > 
> > (I didn't go super deep on the selftest)
> 
> Looks like that one throws a build error, fwiw:
> 
> https://github.com/kernel-patches/bpf/runs/7844497492?check_suite_focus=true
> 
>   [...]
>     CLNG-BPF [test_maps] kfunc_call_test_subprog.o
>     CLNG-BPF [test_maps] bpf_iter_test_kern6.o
>   progs/setget_sockopt.c:39:33: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion]
>           { .opt = SO_REUSEADDR, .flip = 1, },
>                                          ^
>   progs/setget_sockopt.c:42:33: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion]
>           { .opt = SO_KEEPALIVE, .flip = 1, },
>                                          ^
>   progs/setget_sockopt.c:44:33: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion]
>           { .opt = SO_REUSEPORT, .flip = 1, },
>                                          ^
>     CLNG-BPF [test_maps] btf__core_reloc_type_id.o
>   progs/setget_sockopt.c:48:32: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion]
>           { .opt = SO_TXREHASH, .flip = 1, },
>                                         ^
>   progs/setget_sockopt.c:53:32: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion]
>           { .opt = TCP_NODELAY, .flip = 1, },
>                                         ^
>   progs/setget_sockopt.c:61:45: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion]
>           { .opt = TCP_THIN_LINEAR_TIMEOUTS, .flip = 1, },
>                                                      ^
>   progs/setget_sockopt.c:75:39: error: implicit truncation from 'int' to bit-field changes value from 1 to -1 [-Werror,-Wbitfield-constant-conversion]
>           { .opt = IPV6_AUTOFLOWLABEL, .flip = 1, },
>                                                ^
>   7 errors generated.
>   make: *** [Makefile:521: /tmp/runner/work/bpf/bpf/tools/testing/selftests/bpf/setget_sockopt.o] Error 1
>   make: *** Waiting for unfinished jobs....
>   make: Leaving directory '/tmp/runner/work/bpf/bpf/tools/testing/selftests/bpf'
>   Error: Process completed with exit code 2.
Thanks for the report.  I also see it after moving from clang 15 to 16.
I will address it in v4.