Hi,
On 2/26/2025 10:19 AM, Hou Tao wrote:
> Hi,
>
> On 2/14/2025 12:19 AM, Leon Hwang wrote:
>> This patch introduces global percpu data, inspired by commit
>> 6316f78306c1 ("Merge branch 'support-global-data'"). It enables the
>> definition of global percpu variables in BPF, similar to the
>> DEFINE_PER_CPU() macro in the kernel[0].
>>
>> For example, in BPF, it is able to define a global percpu variable like:
>>
>> int data SEC(".percpu");
>>
>> With this patch, tools like retsnoop[1] and bpflbr[2] can simplify their
>> BPF code for handling LBRs. The code can be updated from
>>
>> static struct perf_branch_entry lbrs[1][MAX_LBR_ENTRIES] SEC(".data.lbrs");
>>
>> to
>>
>> static struct perf_branch_entry lbrs[MAX_LBR_ENTRIES] SEC(".percpu.lbrs");
>>
>> This eliminates the need to retrieve the CPU ID using the
>> bpf_get_smp_processor_id() helper.
>>
>> Additionally, by reusing global percpu data map, sharing information
>> between tail callers and callees or freplace callers and callees becomes
>> simpler compared to reusing percpu_array maps.
>>
>> Links:
>> [0] https://github.com/torvalds/linux/blob/fbfd64d25c7af3b8695201ebc85efe90be28c5a3/include/linux/percpu-defs.h#L114
>> [1] https://github.com/anakryiko/retsnoop
>> [2] https://github.com/Asphaltt/bpflbr
>>
>> Signed-off-by: Leon Hwang <leon.hwang@xxxxxxxxx>
>> ---
> SNIP
>> @@ -815,6 +850,8 @@ const struct bpf_map_ops percpu_array_map_ops = {
>> .map_get_next_key = array_map_get_next_key,
>> .map_lookup_elem = percpu_array_map_lookup_elem,
>> .map_gen_lookup = percpu_array_map_gen_lookup,
>> + .map_direct_value_addr = percpu_array_map_direct_value_addr,
>> + .map_direct_value_meta = percpu_array_map_direct_value_meta,
>> .map_update_elem = array_map_update_elem,
>> .map_delete_elem = array_map_delete_elem,
>> .map_lookup_percpu_elem = percpu_array_map_lookup_percpu_elem,
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 9971c03adfd5d..5682546b1193e 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -6810,6 +6810,8 @@ static 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_ARRAY)
>> + return -EINVAL;
> Is the check still necessary ? Because its caller has already added the
> check of map_type.
>> err = map->ops->map_direct_value_addr(map, &addr, off);
>> if (err)
>> return err;
>> @@ -7322,6 +7324,7 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
>> /* if map is read-only, track its contents as scalars */
>> if (tnum_is_const(reg->var_off) &&
>> bpf_map_is_rdonly(map) &&
>> + map->map_type == BPF_MAP_TYPE_ARRAY &&
>> map->ops->map_direct_value_addr) {
>> int map_off = off + reg->var_off.value;
>> u64 val = 0;
> Do we also need to check in check_ld_imm() to ensure the dst_reg of
> bpf_ld_imm64 on a per-cpu array will not be treated as a map-value-ptr ?
Just find out that if the check in check_ld_imm() is added, these
map_type checking added in multiple functions will be unnecessary,
because all of these functions needs the register to be a map-value-ptr.
>> @@ -9128,6 +9131,11 @@ static int check_reg_const_str(struct bpf_verifier_env *env,
>> return -EACCES;
>> }
>>
>> + if (map->map_type != BPF_MAP_TYPE_ARRAY) {
>> + verbose(env, "only array map supports direct string value access\n");
>> + return -EINVAL;
>> + }
>> +
>> err = check_map_access(env, regno, reg->off,
>> map->value_size - reg->off, false,
>> ACCESS_HELPER);
>> @@ -10802,6 +10810,11 @@ static int check_bpf_snprintf_call(struct bpf_verifier_env *env,
>> return -EINVAL;
>> num_args = data_len_reg->var_off.value / 8;
>>
>> + if (fmt_map->map_type != BPF_MAP_TYPE_ARRAY) {
>> + verbose(env, "only array map supports snprintf\n");
>> + return -EINVAL;
>> + }
>> +
>>
>
> .
