On Tue, Aug 26, 2025 at 8:25 AM Leon Hwang <leon.hwang@xxxxxxxxx> wrote:
>
> On Sat Aug 23, 2025 at 6:14 AM +08, Andrii Nakryiko wrote:
> > On Thu, Aug 21, 2025 at 9:08 AM Leon Hwang <leon.hwang@xxxxxxxxx> wrote:
> >>
> >> It is to unify map flags checking for lookup, update, lookup_batch and
> >> update_batch.
> >>
> >> Therefore, it will be convenient to check BPF_F_CPU flag in this helper
> >> function for them in next patch.
> >>
> >> Signed-off-by: Leon Hwang <leon.hwang@xxxxxxxxx>
> >> ---
> >> kernel/bpf/syscall.c | 45 ++++++++++++++++++++++----------------------
> >> 1 file changed, 22 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> >> index 0fbfa8532c392..19f7f5de5e7dc 100644
> >> --- a/kernel/bpf/syscall.c
> >> +++ b/kernel/bpf/syscall.c
> >> @@ -1654,6 +1654,17 @@ static void *___bpf_copy_key(bpfptr_t ukey, u64 key_size)
> >> return NULL;
> >> }
> >>
> >> +static int check_map_flags(struct bpf_map *map, u64 flags, bool check_flag)
> >
> > "check_map_flags" is super generically named... (and actually
> > misleading, it's not map flags you are checking), so I think it should
> > be something along the lines of "check_map_op_flag", i.e. map
> > *operation* flag?
> >
> > but also check_flag bool argument name for a function called "check
> > flags" is so confusing... The idea here is whether we should enforce
> > there is no *extra* flags beyond those common for all operations,
> > right? So maybe call it "allow_extra_flags" or alternatively
> > "strict_extra_flags", something suggesting that his is something in
> > addition to common flags
> >
> > alternatively, and perhaps best of all, I'd move that particular check
> > outside and just maintain something like ARRAY_CREATE_FLAG_MASK for
> > each operation, checking it explicitly where appropriate. WDYT?
> >
>
> Ack.
>
> Following this idea, the checking functions will be
>
> static inline bool bpf_map_check_op_flags(struct bpf_map *map, u64 flags, bool strict_extra_flags,
> u64 extra_flags_mask)
> {
> if (strict_extra_flags && ((u32)flags & extra_flags_mask))
> return -EINVAL;
with this implementation strict_extra_flags argument is superficial,
you can just pass extra_flags_mask == 0 to disable this check,
effectively
>
> if ((flags & BPF_F_LOCK) && !btf_record_has_field(map->record, BPF_SPIN_LOCK))
> return -EINVAL;
>
> if (!(flags & BPF_F_CPU) && flags >> 32)
> return -EINVAL;
>
> if ((flags & (BPF_F_CPU | BPF_F_ALL_CPUS)) && !bpf_map_supports_cpu_flags(map->map_type))
> return -EINVAL;
>
> return 0;
> }
>
> #define BPF_MAP_LOOKUP_ELEM_EXTRA_FLAGS_MASK (~(BPF_F_LOCK | BPF_F_CPU | BPF_F_ALL_CPUS))
>
> static inline bool bpf_map_check_update_flags(struct bpf_map *map, u64 flags)
> {
> return bpf_map_check_op_flags(map, flags, false, 0);
> }
>
> static inline bool bpf_map_check_lookup_flags(struct bpf_map *map, u64 flags)
> {
> return bpf_map_check_op_flags(map, flags, true, BPF_MAP_LOOKUP_ELEM_EXTRA_FLAGS_MASK);
> }
>
> static inline bool bpf_map_check_batch_flags(struct bpf_map *map, u64 flags)
> {
> return bpf_map_check_op_flags(map, flags, true, BPF_MAP_LOOKUP_ELEM_EXTRA_FLAGS_MASK);
> }
>
> These functions are better than check_map_flags().
>
> Thanks,
> Leon
>
> > pw-bot: cr
> >
> > [...]
