netdev - Re: [PATCH bpf-next v2 07/15] bpf: offload: add map offload infrastructure

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] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 14 Jan 2018 15:52:55 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        David Miller <davem@...emloft.net>,
        Linux Netdev List <netdev@...r.kernel.org>,
        oss-drivers@...ronome.com, tehnerd@...com
Subject: Re: [PATCH bpf-next v2 07/15] bpf: offload: add map offload infrastructure

On Sun, Jan 14, 2018 at 3:37 PM, Daniel Borkmann <daniel@...earbox.net> wrote:
> Hi Jakub,
>
> Series looks fine, just stumbled over one small thing here below.
>
> On 01/12/2018 05:29 AM, Jakub Kicinski wrote:
> [...]
>> +bool bpf_offload_dev_match(struct bpf_prog *prog, struct bpf_map *map)
>> +{
>> +     struct bpf_offloaded_map *offmap;
>> +     struct bpf_prog_offload *offload;
>> +     bool ret;
>> +
>> +     if (!!bpf_prog_is_dev_bound(prog->aux) != !!bpf_map_is_dev_bound(map))
>> +             return false;
>> +     if (!bpf_prog_is_dev_bound(prog->aux))
>> +             return true;
>
> Should this not say 'false' if the prog has no offload_requested ...
>
>> +     down_read(&bpf_devs_lock);
>> +     offload = prog->aux->offload;
>> +     offmap = map_to_offmap(map);
>> +
>> +     ret = offload && offload->netdev == offmap->netdev;
>
> ... meaning we return true from bpf_offload_dev_match() only in the
> case when netdevs match?

IOW return false when both program and map are not offloaded?  I was
going for "are those two compatible" kind of logic.

But I'll change, the only user of this function is the verifier
compatibility check and that already handles the "neither is
offloaded" case.

>> +     up_read(&bpf_devs_lock);
>> +
>> +     return ret;
>> +}
> Thanks,
> Daniel