Re: [PATCH bpf-next v10 2/9] bpf: Introduce global percpu data

["Emil Tsalapatis" <emil@etsalapatis.com>]

On Mon Jul 20, 2026 at 12:59 AM EDT, Leon Hwang wrote:
> On 17/7/26 14:25, Emil Tsalapatis wrote:
>> On Wed Jul 15, 2026 at 11:32 AM EDT, Leon Hwang wrote:
>
> [...]
>
>>>  
>>> +static int percpu_array_map_direct_value_addr(const struct bpf_map *map, u64 *imm, u32 off)
>>> +{
>>> +	struct bpf_array *array = container_of(map, struct bpf_array, map);
>>> +
>>> +	if (map->max_entries != 1)
>>> +		return -EOPNOTSUPP;
>>> +	if (off >= map->value_size)
>>> +		return -EINVAL;
>>> +	if (!bpf_jit_supports_percpu_insn())
>>> +		return -EOPNOTSUPP;
>> 
>> Nit: This should be first, if the JIT doesn't support the instruction
>> all else is moot. Same below.
>
> Ack.
>
>> 
>>> +
>>> +	*imm = (u64)(__force unsigned long) array->pptrs[0];
>>> +	return 0;
>>> +}
>>> +
>>> +static int percpu_array_map_direct_value_meta(const struct bpf_map *map, u64 imm, u32 *off)
>>> +{
>>> +	struct bpf_array *array = container_of(map, struct bpf_array, map);
>>> +	u64 base = (u64)(__force unsigned long) array->pptrs[0];
>>> +
>>> +	if (map->max_entries != 1)
>>> +		return -EOPNOTSUPP;
>>> +	if (imm < base || imm >= base + array->elem_size)
>>> +		return -ENOENT;
>>> +	if (!bpf_jit_supports_percpu_insn())
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	*off = imm - base;
>>> +	return 0;
>>> +}
>>> +
>
> [...]
>
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index e1244a721194..ae442ea217c4 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -5615,6 +5615,8 @@ int bpf_map_direct_read(struct bpf_map *map, int off, int size, u64 *val,
>>>  	u64 addr;
>>>  	int err;
>>>  
>>> +	if (map->map_type == BPF_MAP_TYPE_INSN_ARRAY || map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY)
>>> +		return -EINVAL;
>>>  	err = map->ops->map_direct_value_addr(map, &addr, off);
>>>  	if (err)
>>>  		return err;
>>> @@ -6174,6 +6176,7 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, struct b
>>>  			if (tnum_is_const(reg->var_off) &&
>>>  			    bpf_map_is_rdonly(map) &&
>>>  			    map->ops->map_direct_value_addr &&
>>> +			    map->map_type != BPF_MAP_TYPE_PERCPU_ARRAY &&
>>>  			    map->map_type != BPF_MAP_TYPE_INSN_ARRAY) {
>> 
>> An issue I have with the patch is that it takes the special-casing we
>> already have for INSN_ARRAY and makes it worse. The instruction arrays
>> implement direct_value_addr, but it's not actually valid to call
>> direct_value_addr from a bunch of places where it's called, so we have
>> to special case check the map type to prevent it. Case in point the
>> special casing in check_arg_const_str in the thunk below.
>> 
>> Imo we should consider making a different map method different from
>> direct_value_addr that INSN_ARRAY and PERCPU_ARRAY implement. That way
>> we remove most of the map type checks in those paths. I'm not exactly
>> sure how that would look like, but I could take a look at it tomorrow.
>
>
> Seems reasonable to me. I don't have good candidates for the new map
> method name.
>
> Instead of making a new map method, how about factoring out a helper for
> the 'if (t == BPF_READ && value_regno >= 0) {}' block? It will simplify
> these map_type checks in the helper.
>

[...]

>> 
>>>  				int map_off = off + reg->var_off.value;
>>>  				u64 val = 0;
>>> @@ -8176,6 +8179,12 @@ static int check_arg_const_str(struct bpf_verifier_env *env,
>>>  		return -EACCES;
>>>  	}
>>>  
>>> +	if (map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY) {
>>> +		verbose(env, "%s points to percpu_array map which cannot be used as const string\n",
>>> +			reg_arg_name(env, argno));
>>> +		return -EACCES;
>>> +	}
>>> +
>>>  	if (!bpf_map_is_rdonly(map)) {
>>>  		verbose(env, "%s does not point to a readonly map'\n", reg_arg_name(env, argno));
>>>  		return -EACCES;
>>> @@ -18279,6 +18288,12 @@ static int check_and_resolve_insns(struct bpf_verifier_env *env)
>>>  					return -EINVAL;
>>>  				}
>>>  
>>> +				if (map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY &&
>>> +				    !env->prog->jit_requested) {
>>> +					verbose(env, "JIT is required to use global percpu data\n");
>>> +					return -EOPNOTSUPP;
>>> +				}
>>> +
>> 
>> Another place where we add map-specific checks where there are none.
>
>
> I think this check is unnecessary, because 'prog->jit_required = true'
> has been set for global percpu data.
>
> Will drop this if.

I think you're right, in which case we can drop it. In turn this means
wrapping the check above in a helper adds enough structure to the logic.
Adding the new method would still work valid but it'd be more borderline
in terms of simplifying the code because we''ll have abstracted most of
the type checks away.

>
> Thanks,
> Leon
>
>> 
>>>  				err = map->ops->map_direct_value_addr(map, &addr, off);
>>>  				if (err) {
>>>  					verbose(env, "invalid access to map value pointer, value_size=%u off=%u\n",
>>