netdev - Re: [PATCH bpf-next 1/5] bpf: Handle 8-byte values in DEVMAP and DEVMAP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 27 May 2020 12:38:10 -0600
From:   David Ahern <dsahern@...il.com>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     David Ahern <dsahern@...nel.org>, netdev@...r.kernel.org,
        davem@...emloft.net, kuba@...nel.org, toke@...hat.com,
        daniel@...earbox.net, john.fastabend@...il.com, ast@...nel.org,
        kafai@...com, songliubraving@...com, yhs@...com, andriin@...com
Subject: Re: [PATCH bpf-next 1/5] bpf: Handle 8-byte values in DEVMAP and
 DEVMAP_HASH

On 5/27/20 9:30 AM, Jesper Dangaard Brouer wrote:
> On Wed, 27 May 2020 08:27:36 -0600
> David Ahern <dsahern@...il.com> wrote:
> 
>> On 5/27/20 4:26 AM, Jesper Dangaard Brouer wrote:
>>> IMHO we really need to leverage BTF here, as I'm sure we need to do more
>>> extensions, and this size matching will get more and more unmaintainable.
>>>
>>> With BTF in place, dumping the map via bpftool, will also make the
>>> fields "self-documenting".  
>>
>> furthermore, the kernel is changing the value - an fd is passed in and
>> an id is returned. I do not see how any of this fits into BTF.
> 
> It can, as BTF actually support union's (I just tested that).
> 

You are trying to force an arbitrary kernel API with BTF, and it adds no
value over a static struct definition that grows as new capability is added.

DEVMAPs (and CPUMAP) are not like other maps - the fields in the entries
have meaning to *the kernel*. That means the kernel needs to know the
fields and what they represent - be it a device index, a program fd
converted to an id, a cpuid, or any future extension. If you add 'u32
foo' to this interface, you still need kernel code to understand 'foo'
maps to resource 'bar' via lookup function 'f'. You can not do this
dynamically. BTF does not make sense here.

Furthermore, if the kernel does not understand a field then it better
return an error code - EINVAL so the user knows expected functionality
is not supported by that kernel.

> 
> But a union would also work (also tested via BPF loading and BTF dumpinmg):
> 
>  struct dev_map_ext_val {
>         u32 ifindex;
>         union {
>                 int bpf_prog_fd_write;
>                 u32 bpf_prog_id_read;
>         };
>  };
> 

I prefer the union and without the verb; comments convey the same
meaning. The fd has no meaning beyond the process that created the entry
so it does not need to be kept around bloating the interface.

For v2, I moved the struct to uapi/linux/bpf.h and added comments.