linux-kernel - Re: [PATCH v2 bpf-next 2/9] bpf: add generic support for lookup and lookup_and

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]

Message-ID: <e9cb66e4-16fd-8850-4755-3034f75788c5@fb.com>
Date:   Fri, 22 Nov 2019 00:34:15 +0000
From:   Yonghong Song <yhs@...com>
To:     Brian Vazquez <brianvv@...gle.com>
CC:     Brian Vazquez <brianvv.kernel@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        "David S . Miller" <davem@...emloft.net>,
        Stanislav Fomichev <sdf@...gle.com>,
        Petar Penkov <ppenkov@...gle.com>,
        Willem de Bruijn <willemb@...gle.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>
Subject: Re: [PATCH v2 bpf-next 2/9] bpf: add generic support for lookup and
 lookup_and_delete batch ops



On 11/21/19 1:36 PM, Brian Vazquez wrote:
> Hi Yonghong,
> thanks for reviewing the patch, I will fix all the direct returns and
> small fixes in next version.
> 
> On Thu, Nov 21, 2019 at 9:36 AM Yonghong Song <yhs@...com> wrote:
>>
>>
>>
>> On 11/19/19 11:30 AM, Brian Vazquez wrote:
>>> This commit introduces generic support for the bpf_map_lookup_batch and
>>> bpf_map_lookup_and_delete_batch ops. This implementation can be used by
>>> almost all the bpf maps since its core implementation is relying on the
>>> existing map_get_next_key, map_lookup_elem and map_delete_elem
>>> functions. The bpf syscall subcommands introduced are:
>>>
[...]
>>> +     for (cp = 0; cp < max_count; cp++) {
>>> +             if (cp || first_key) {
>>> +                     rcu_read_lock();
>>> +                     err = map->ops->map_get_next_key(map, prev_key, key);
>>> +                     rcu_read_unlock();
>>> +                     if (err)
>>> +                             break;
>>> +             }
>>> +             err = bpf_map_copy_value(map, key, value,
>>> +                                      attr->batch.elem_flags, do_delete);
>>> +
>>> +             if (err == -ENOENT) {
>>> +                     if (retry) {
>>> +                             retry--;
>>
>> What is the 'retry' semantics here? After 'continue', cp++ is executed.
> 
> Good catch, I'll move cp++ to a proper place. retry is used to prevent
> the cases where the map is doing many concurrent additions and
> deletions, this could result in map_get_next_key succeeding but
> bpf_map_copy_value failing, in which case I think it'd be better to
> try and find a next elem, but we don't want to do this for more than 3
> times.
> 
>>
>>> +                             continue;
>>> +                     }
>>> +                     err = -EINTR;
>>
>> Why returning -EINTR?
> 
> I thought that this is the err more appropriate for the behaviour I
> describe above. Should I handle that case? WDYT?

I see. We do not want to use -ENOENT since get_next_key may return 
-ENOENT. I think -EINTR is okay here to indicate we have key, but 
key/value entry is gone right before the attempted access.