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:   Mon, 15 Jan 2018 00:58:13 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
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 01/15/2018 12:52 AM, Jakub Kicinski wrote:
> 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.

Yeah, agree, it's redundant, but not a bug. I'm fine if you roll this into
your follow-ups, since netdevsim and test cases are still to come anyway.