Re: [PATCH bpf-next v3 1/6] bpf: Introduce internal check_map_flags helper function

["Leon Hwang" <leon.hwang@xxxxxxxxx>]

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;

        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
>
> [...]