On Wed, Jul 31, 2024 at 4:38 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 7/27/24 8:01 PM, Amery Hung wrote:
> > @@ -8399,7 +8408,12 @@ static const struct bpf_reg_types func_ptr_types = { .types = { PTR_TO_FUNC } };
> > static const struct bpf_reg_types stack_ptr_types = { .types = { PTR_TO_STACK } };
> > static const struct bpf_reg_types const_str_ptr_types = { .types = { PTR_TO_MAP_VALUE } };
> > static const struct bpf_reg_types timer_types = { .types = { PTR_TO_MAP_VALUE } };
> > -static const struct bpf_reg_types kptr_xchg_dest_types = { .types = { PTR_TO_MAP_VALUE } };
> > +static const struct bpf_reg_types kptr_xchg_dest_types = {
> > + .types = {
> > + PTR_TO_MAP_VALUE,
> > + PTR_TO_BTF_ID | MEM_ALLOC
> > + }
> > +};
> > static const struct bpf_reg_types dynptr_types = {
> > .types = {
> > PTR_TO_STACK,
> > @@ -8470,7 +8484,8 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> > if (base_type(arg_type) == ARG_PTR_TO_MEM)
> > type &= ~DYNPTR_TYPE_FLAG_MASK;
> >
> > - if (meta->func_id == BPF_FUNC_kptr_xchg && type_is_alloc(type)) {
> > + /* local kptr types are allowed as the source argument of bpf_kptr_xchg */
> > + if (meta->func_id == BPF_FUNC_kptr_xchg && type_is_alloc(type) && regno == BPF_REG_2) {
> > type &= ~MEM_ALLOC;
> > type &= ~MEM_PERCPU;
> > }
> > @@ -8563,7 +8578,7 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> > verbose(env, "verifier internal error: unimplemented handling of MEM_ALLOC\n");
> > return -EFAULT;
> > }
> > - if (meta->func_id == BPF_FUNC_kptr_xchg) {
> > + if (meta->func_id == BPF_FUNC_kptr_xchg && regno == BPF_REG_2) {
>
> I think this BPF_REG_2 check is because the dst (BPF_REG_1) can be MEM_ALLOC
> now. Just want to ensure I understand it correctly.
>
Right. I can add a comment for this too if that helps:
/* Check if local kptr in src arg matches kptr in dst arg */
> One nit. Please also update the document for bpf_kptr_xchg in uapi/linux/bpf.h:
>
> ==== >8888 ====
> * void *bpf_kptr_xchg(void *map_value, void *ptr)
> * Description
> * Exchange kptr at pointer *map_value* with *ptr*, and return the
> ==== 8888< ====
>
Got it. I will change the first argument from "map_value" to "dst" and
the description to reflect what this series does.
Thanks,
Amery
