On Tue, Aug 26, 2025 at 8:35 AM Leon Hwang <leon.hwang@xxxxxxxxx> wrote:
>
> On Sat Aug 23, 2025 at 6:20 AM +08, Andrii Nakryiko wrote:
> > On Thu, Aug 21, 2025 at 9:09 AM Leon Hwang <leon.hwang@xxxxxxxxx> wrote:
> >>
>
> [...]
>
> >> @@ -10630,6 +10630,19 @@ static int validate_map_op(const struct bpf_map *map, size_t key_sz,
> >> int num_cpu = libbpf_num_possible_cpus();
> >> size_t elem_sz = roundup(map->def.value_size, 8);
> >>
> >> + if (flags & (BPF_F_CPU | BPF_F_ALL_CPUS)) {
> >> + if ((flags & BPF_F_CPU) && (flags & BPF_F_ALL_CPUS))
> >> + return -EINVAL;
> >> + if ((flags >> 32) >= num_cpu)
> >> + return -ERANGE;
> >
> > The idea of validate_map_op() is to make it easier for users to
> > understand what's wrong with how they deal with the map, rather than
> > just getting indiscriminate -EINVAL from the kernel.
> >
> > Point being: add human-readable pr_warn() explanations for all the new
> > conditions you are detecting, otherwise it's just meaningless.
> >
>
> Ack.
>
> I'll add these pr_warn() explanations in next revision.
>
> >> + if (value_sz != elem_sz) {
> >> + pr_warn("map '%s': unexpected value size %zu provided for per-CPU map, expected %zu\n",
> >> + map->name, value_sz, elem_sz);
> >> + return -EINVAL;
> >> + }
> >> + break;
> >> + }
> >> +
> >> if (value_sz != num_cpu * elem_sz) {
> >> pr_warn("map '%s': unexpected value size %zu provided for per-CPU map, expected %d * %zu = %zd\n",
> >> map->name, value_sz, num_cpu, elem_sz, num_cpu * elem_sz);
> >> @@ -10654,7 +10667,7 @@ int bpf_map__lookup_elem(const struct bpf_map *map,
> >> {
> >> int err;
> >>
> >> - err = validate_map_op(map, key_sz, value_sz, true);
> >> + err = validate_map_op(map, key_sz, value_sz, true, flags);
> >> if (err)
> >> return libbpf_err(err);
> >>
> >> @@ -10667,7 +10680,7 @@ int bpf_map__update_elem(const struct bpf_map *map,
> >> {
> >> int err;
> >>
> >> - err = validate_map_op(map, key_sz, value_sz, true);
> >> + err = validate_map_op(map, key_sz, value_sz, true, flags);
> >> if (err)
> >> return libbpf_err(err);
> >>
> >> @@ -10679,7 +10692,7 @@ int bpf_map__delete_elem(const struct bpf_map *map,
> >> {
> >> int err;
> >>
> >> - err = validate_map_op(map, key_sz, 0, false /* check_value_sz */);
> >> + err = validate_map_op(map, key_sz, 0, false /* check_value_sz */, 0);
> >
> > hard-coded 0 instead of flags, why?
> >
>
> It should be flags.
>
> However, delete op does not support the introduced cpu flags.
>
> I think it's OK to use 0 here.
>
> >> if (err)
> >> return libbpf_err(err);
> >>
> >> @@ -10692,7 +10705,7 @@ int bpf_map__lookup_and_delete_elem(const struct bpf_map *map,
> >> {
> >> int err;
> >>
> >> - err = validate_map_op(map, key_sz, value_sz, true);
> >> + err = validate_map_op(map, key_sz, value_sz, true, 0);
> >
> > same about flags
> >
>
> Ack.
>
> >> if (err)
> >> return libbpf_err(err);
> >>
> >> @@ -10704,7 +10717,7 @@ int bpf_map__get_next_key(const struct bpf_map *map,
> >> {
> >> int err;
> >>
> >> - err = validate_map_op(map, key_sz, 0, false /* check_value_sz */);
> >> + err = validate_map_op(map, key_sz, 0, false /* check_value_sz */, 0);
> >> if (err)
> >> return libbpf_err(err);
> >>
> >> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> >> index 2e91148d9b44d..6a972a8d060c3 100644
> >> --- a/tools/lib/bpf/libbpf.h
> >> +++ b/tools/lib/bpf/libbpf.h
> >> @@ -1196,12 +1196,13 @@ LIBBPF_API struct bpf_map *bpf_map__inner_map(struct bpf_map *map);
> >> * @param key_sz size in bytes of key data, needs to match BPF map definition's **key_size**
> >> * @param value pointer to memory in which looked up value will be stored
> >> * @param value_sz size in byte of value data memory; it has to match BPF map
> >> - * definition's **value_size**. For per-CPU BPF maps value size has to be
> >> - * a product of BPF map value size and number of possible CPUs in the system
> >> - * (could be fetched with **libbpf_num_possible_cpus()**). Note also that for
> >> - * per-CPU values value size has to be aligned up to closest 8 bytes for
> >> - * alignment reasons, so expected size is: `round_up(value_size, 8)
> >> - * * libbpf_num_possible_cpus()`.
> >> + * definition's **value_size**. For per-CPU BPF maps, value size can be
> >> + * definition's **value_size** if **BPF_F_CPU** or **BPF_F_ALL_CPUS** is
> >> + * specified in **flags**, otherwise a product of BPF map value size and number
> >> + * of possible CPUs in the system (could be fetched with
> >> + * **libbpf_num_possible_cpus()**). Note else that for per-CPU values value
> >> + * size has to be aligned up to closest 8 bytes for alignment reasons, so
> >
> > nit: aligned up for alignment reasons... drop "for alignment reasons", I guess?
> >
>
> It is "for alignment reasons", because percpu maps use bpf_long_memcpy()
> to copy data.
>
my complaint is just wording, if you say "aligned up" it implies that
there is some alignment reason. So I'd just say "size has to be
aligned up to 8 bytes." and be done with it
> static inline void bpf_long_memcpy(void *dst, const void *src, u32 size)
> {
> const long *lsrc = src;
> long *ldst = dst;
>
> size /= sizeof(long);
> while (size--)
> data_race(*ldst++ = *lsrc++);
> }
>
> Thanks,
> Leon
>
> [...]
