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 ?
> @@ -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;
> + }
> +
>
