Received-SPF: pass (google.com: domain of linux-kernel+bounces-156177-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240420-bpf_wq-v2-0-6c986a5a741f@kernel.org> <20240420-bpf_wq-v2-13-6c986a5a741f@kernel.org>
In-Reply-To: <20240420-bpf_wq-v2-13-6c986a5a741f@kernel.org>
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Tue, 23 Apr 2024 19:56:13 -0700
Message-ID: <CAADnVQLbK2LajKjGRCxCUcSVOoPc5rQh=Gvz=AhYCon25CHxUA@mail.gmail.com>
Subject: Re: [PATCH bpf-next v2 13/16] bpf: wq: add bpf_wq_set_callback_impl
To: Benjamin Tissoires <bentiss@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, 
	Andrii Nakryiko <andrii@kernel.org>, Martin KaFai Lau <martin.lau@linux.dev>, 
	Eduard Zingerman <eddyz87@gmail.com>, Song Liu <song@kernel.org>, 
	Yonghong Song <yonghong.song@linux.dev>, John Fastabend <john.fastabend@gmail.com>, 
	KP Singh <kpsingh@kernel.org>, Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>, 
	Jiri Olsa <jolsa@kernel.org>, Mykola Lysenko <mykolal@fb.com>, Shuah Khan <shuah@kernel.org>, 
	bpf <bpf@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>, 
	"open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Apr 20, 2024 at 2:10=E2=80=AFAM Benjamin Tissoires <bentiss@kernel.=
org> wrote:
> @@ -11018,6 +11027,7 @@ enum special_kfunc_type {
>         KF_bpf_percpu_obj_drop_impl,
>         KF_bpf_throw,
>         KF_bpf_iter_css_task_new,
> +       KF_bpf_wq_set_callback_impl,
>  };
>
>  BTF_SET_START(special_kfunc_set)
> @@ -11044,6 +11054,7 @@ BTF_ID(func, bpf_throw)
>  #ifdef CONFIG_CGROUPS
>  BTF_ID(func, bpf_iter_css_task_new)
>  #endif
> +BTF_ID(func, bpf_wq_set_callback_impl)
>  BTF_SET_END(special_kfunc_set)
>
>  BTF_ID_LIST(special_kfunc_list)
> @@ -11074,6 +11085,7 @@ BTF_ID(func, bpf_iter_css_task_new)
>  #else
>  BTF_ID_UNUSED
>  #endif
> +BTF_ID(func, bpf_wq_set_callback_impl)

This is broken on !CONFIG_CGROUPS.
KF_bpf_wq_set_callback_impl in enum special_kfunc_type won't
match special_kfunc_list.
I moved this line up while applying.

>
>  static bool is_kfunc_ret_null(struct bpf_kfunc_call_arg_meta *meta)
>  {
> @@ -11402,12 +11414,28 @@ static bool is_sync_callback_calling_kfunc(u32 =
btf_id)
>         return btf_id =3D=3D special_kfunc_list[KF_bpf_rbtree_add_impl];
>  }
>
> +static bool is_async_callback_calling_kfunc(u32 btf_id)
> +{
> +       return btf_id =3D=3D special_kfunc_list[KF_bpf_wq_set_callback_im=
pl];
> +}
> +
>  static bool is_bpf_throw_kfunc(struct bpf_insn *insn)
>  {
>         return bpf_pseudo_kfunc_call(insn) && insn->off =3D=3D 0 &&
>                insn->imm =3D=3D special_kfunc_list[KF_bpf_throw];
>  }
>
> +static bool is_bpf_wq_set_callback_impl_kfunc(u32 btf_id)
> +{
> +       return btf_id =3D=3D special_kfunc_list[KF_bpf_wq_set_callback_im=
pl];
> +}
> +
> +static bool is_callback_calling_kfunc(u32 btf_id)
> +{
> +       return is_sync_callback_calling_kfunc(btf_id) ||
> +              is_async_callback_calling_kfunc(btf_id);
> +}
> +
>  static bool is_rbtree_lock_required_kfunc(u32 btf_id)
>  {
>         return is_bpf_rbtree_api_kfunc(btf_id);
> @@ -12219,6 +12247,16 @@ static int check_kfunc_call(struct bpf_verifier_=
env *env, struct bpf_insn *insn,
>                 }
>         }
>
> +       if (is_bpf_wq_set_callback_impl_kfunc(meta.func_id)) {
> +               err =3D push_callback_call(env, insn, insn_idx, meta.subp=
rogno,
> +                                        set_timer_callback_state);
> +               if (err) {
> +                       verbose(env, "kfunc %s#%d failed callback verific=
ation\n",
> +                               func_name, meta.func_id);
> +                       return err;
> +               }
> +       }
> +
>         rcu_lock =3D is_kfunc_bpf_rcu_read_lock(&meta);
>         rcu_unlock =3D is_kfunc_bpf_rcu_read_unlock(&meta);
>
> @@ -16982,6 +17020,9 @@ static bool states_equal(struct bpf_verifier_env =
*env,
>         if (old->active_rcu_lock !=3D cur->active_rcu_lock)
>                 return false;
>
> +       if (old->in_sleepable !=3D cur->in_sleepable)
> +               return false;
> +
>         /* for states to be equal callsites have to be the same
>          * and all frame states need to be equivalent
>          */
> @@ -19653,6 +19694,28 @@ static int fixup_kfunc_call(struct bpf_verifier_=
env *env, struct bpf_insn *insn,
>                    desc->func_id =3D=3D special_kfunc_list[KF_bpf_rdonly_=
cast]) {
>                 insn_buf[0] =3D BPF_MOV64_REG(BPF_REG_0, BPF_REG_1);
>                 *cnt =3D 1;
> +       } else if (is_bpf_wq_set_callback_impl_kfunc(desc->func_id)) {
> +               /* The verifier will process callback_fn as many times as=
 necessary
> +                * with different maps and the register states prepared b=
y
> +                * set_timer_callback_state will be accurate.
> +                *
> +                * The following use case is valid:
> +                *   map1 is shared by prog1, prog2, prog3.
> +                *   prog1 calls bpf_timer_init for some map1 elements
> +                *   prog2 calls bpf_timer_set_callback for some map1 ele=
ments.
> +                *     Those that were not bpf_timer_init-ed will return =
-EINVAL.
> +                *   prog3 calls bpf_timer_start for some map1 elements.
> +                *     Those that were not both bpf_timer_init-ed and
> +                *     bpf_timer_set_callback-ed will return -EINVAL.
> +                */

Also removed this comment. It's not correct here.

> +               struct bpf_insn ld_addrs[2] =3D {
> +                       BPF_LD_IMM64(BPF_REG_4, (long)env->prog->aux),
> +               };
> +
> +               insn_buf[0] =3D ld_addrs[0];
> +               insn_buf[1] =3D ld_addrs[1];
> +               insn_buf[2] =3D *insn;
> +               *cnt =3D 3;
>         }
>         return 0;
>  }
>
> --
> 2.44.0
>