netdev - Re: [PATCH v2 bpf-next 6/9] bpf: introduce BPF_F

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:   Sat, 19 Jan 2019 22:16:29 +0000
From:   Alexei Starovoitov <ast@...com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Alexei Starovoitov <ast@...nel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 6/9] bpf: introduce BPF_F_LOCK flag

On 1/19/19 10:56 AM, Jakub Kicinski wrote:
> On Thu, 17 Jan 2019 22:50:53 -0800, Alexei Starovoitov wrote:
>> Introduce BPF_F_LOCK flag for map_lookup and map_update syscall commands
>> and for map_update() helper function.
>> In all these cases take a lock of existing element (which was provided
>> in BTF description) before copying (in or out) the rest of map value.
>>
>> Implementation details that are part of uapi:
>>
>> Array:
>> The array map takes the element lock for lookup/update.
>>
>> Hash:
>> hash map also takes the lock for lookup/update and tries to avoid the bucket lock.
>> If old element exists it takes the element lock and updates the element in place.
>> If element doesn't exist it allocates new one and inserts into hash table
>> while holding the bucket lock.
>> In rare case the hashmap has to take both the bucket lock and the element lock
>> to update old value in place.
>>
>> Cgroup local storage:
>> It is similar to array. update in place and lookup are done with lock taken.
>>
>> Signed-off-by: Alexei Starovoitov <ast@...nel.org>
> 
>> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
>> index 48a41bf65e1b..6397b12c130e 100644
>> --- a/kernel/bpf/hashtab.c
>> +++ b/kernel/bpf/hashtab.c
>> @@ -809,11 +809,11 @@ static struct htab_elem *alloc_htab_elem(struct bpf_htab *htab, void *key,
>>   static int check_flags(struct bpf_htab *htab, struct htab_elem *l_old,
>>   		       u64 map_flags)
>>   {
>> -	if (l_old && map_flags == BPF_NOEXIST)
>> +	if (l_old && (map_flags & ~BPF_F_LOCK) == BPF_NOEXIST)
>>   		/* elem already exists */
>>   		return -EEXIST;
>>   
>> -	if (!l_old && map_flags == BPF_EXIST)
>> +	if (!l_old && (map_flags & ~BPF_F_LOCK) == BPF_EXIST)
>>   		/* elem doesn't exist, cannot update it */
>>   		return -ENOENT;
>>   
>> @@ -832,7 +832,7 @@ static int htab_map_update_elem(struct bpf_map *map, void *key, void *value,
>>   	u32 key_size, hash;
>>   	int ret;
>>   
>> -	if (unlikely(map_flags > BPF_EXIST))
>> +	if (unlikely(map_flags & ~(BPF_F_LOCK | BPF_EXIST | BPF_NOEXIST)))
>>   		/* unknown flags */
>>   		return -EINVAL;
> 
> Perhaps we should check for:
> 
> 	hweight(map_flags & (BPF_EXIST | BPF_NOEXIST)) < 2

hweight in critical path is imo too slow.

> right now this > BPF_EXIST is a little hacky, as it depends on the fact,
> that BPF_EXIST is 2..  If I read the code correctly  BPF_EXIST |
> BPF_NOEXIST will be treated as BPF_NOEXIST.  Not sure that matters.

#define BPF_ANY         0 /* create new element or update existing */
#define BPF_NOEXIST     1 /* create new element if it didn't exist */
#define BPF_EXIST       2 /* update existing element */
#define BPF_F_LOCK      4 /* spin_lock-ed map_lookup/map_update */

for all other flags we just 'or' them together and do
flag & ~(FLAG1 | FLAG2 | ..)
so it's easy to add new flags later.
Here I'm only skipping BPF_ANY from this practice because it's zero.

>> diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
>> index 0295427f06e2..6b572e2de7fb 100644
>> --- a/kernel/bpf/local_storage.c
>> +++ b/kernel/bpf/local_storage.c
>> @@ -131,7 +131,14 @@ static int cgroup_storage_update_elem(struct bpf_map *map, void *_key,
>>   	struct bpf_cgroup_storage *storage;
>>   	struct bpf_storage_buffer *new;
>>   
>> -	if (flags != BPF_ANY && flags != BPF_EXIST)
>> +	if (unlikely(flags & ~(BPF_F_LOCK | BPF_EXIST | BPF_NOEXIST)))
>> +		return -EINVAL;
>> +
>> +	if (unlikely(flags & BPF_NOEXIST))
>> +		return -EINVAL;
> 
> nit: for arrays we have a different return code with BPF_NOEXIST, but
>       here its both EINVAL, so perhaps just drop the BPF_NOEXIST from
>       the mask?

I very much prefer compiler to do such optimizations.
First line is doing:
flag & ~(FLAG1 | FLAG2 | ..)
like in all other places.
And second check if (unlikely(flags & BPF_NOEXIST)) is specific
to this helper.

>> @@ -818,6 +836,12 @@ static int map_update_elem(union bpf_attr *attr)
>>   		goto err_put;
>>   	}
>>   
>> +	if ((attr->flags & BPF_F_LOCK) &&
>> +	    !map_value_has_spin_lock(map)) {
> 
> nit: do we need this check in syscall.c for update?  Just wondering.

yes. otherwise the flag will be nop for other map types (percpu, lru, etc)