bpf: fix excessively checking for elem_flags in batch update mode

[PATCH] bpf: fix excessively checking for elem_flags in batch update mode

Posted by Lin Feng 1 year, 5 months ago

Currently generic_map_update_batch will reject all valid command flags for
BPF_MAP_UPDATE_ELEM other than BPF_F_LOCK, which is overkill, map updating
semantic does allow specify BPF_NOEXIST or BPF_EXIST even for batching
update.

Signed-off-by: Lin Feng <linf@wangsu.com>
---
 kernel/bpf/syscall.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 869265852d51..d85361f9a9b8 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1852,7 +1852,7 @@ int generic_map_update_batch(struct bpf_map *map, struct file *map_file,
 	void *key, *value;
 	int err = 0;
 
-	if (attr->batch.elem_flags & ~BPF_F_LOCK)
+	if ((attr->batch.elem_flags & ~BPF_F_LOCK) > BPF_EXIST)
 		return -EINVAL;
 
 	if ((attr->batch.elem_flags & BPF_F_LOCK) &&
-- 
2.42.0

Re: [PATCH] bpf: fix excessively checking for elem_flags in batch update mode

Posted by Daniel Borkmann 1 year, 5 months ago

On 7/17/24 1:15 PM, Lin Feng wrote:
> Currently generic_map_update_batch will reject all valid command flags for
> BPF_MAP_UPDATE_ELEM other than BPF_F_LOCK, which is overkill, map updating
> semantic does allow specify BPF_NOEXIST or BPF_EXIST even for batching
> update.
> 
> Signed-off-by: Lin Feng <linf@wangsu.com>

[ +Hou/Brian ]

Please also add a BPF selftest along with this extension which exercises the
batch update and validates the behavior for the various flags which are now enabled.

Also, please discuss the semantics in the commit msg.. errors due to BPF_EXIST and
BPF_NOEXIST will cause bpf_map_update_value() to fail and then break the loop. It's
probably fine given batch.count (cp) will be propagated back to user space to tell
how many elements could actually get updated.

> ---
>   kernel/bpf/syscall.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 869265852d51..d85361f9a9b8 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1852,7 +1852,7 @@ int generic_map_update_batch(struct bpf_map *map, struct file *map_file,
>   	void *key, *value;
>   	int err = 0;
>   
> -	if (attr->batch.elem_flags & ~BPF_F_LOCK)
> +	if ((attr->batch.elem_flags & ~BPF_F_LOCK) > BPF_EXIST)
>   		return -EINVAL;
>   
>   	if ((attr->batch.elem_flags & BPF_F_LOCK) &&
>

Re: [PATCH] bpf: fix excessively checking for elem_flags in batch update mode

Posted by Hou Tao 1 year, 4 months ago

Hi,

On 7/20/2024 12:22 AM, Daniel Borkmann wrote:
> On 7/17/24 1:15 PM, Lin Feng wrote:
>> Currently generic_map_update_batch will reject all valid command
>> flags for
>> BPF_MAP_UPDATE_ELEM other than BPF_F_LOCK, which is overkill, map
>> updating
>> semantic does allow specify BPF_NOEXIST or BPF_EXIST even for batching
>> update.
>>
>> Signed-off-by: Lin Feng <linf@wangsu.com>
>
> [ +Hou/Brian ]
>
> Please also add a BPF selftest along with this extension which
> exercises the
> batch update and validates the behavior for the various flags which
> are now enabled.

Agreed. There are already some batched map operation tests in
tools/testing/selftests/bpf/map_tests/htab_map_batch_ops.c, I think
extending the test cases in the file will be fine.
> Also, please discuss the semantics in the commit msg.. errors due to
> BPF_EXIST and
> BPF_NOEXIST will cause bpf_map_update_value() to fail and then break
> the loop. It's
> probably fine given batch.count (cp) will be propagated back to user
> space to tell
> how many elements could actually get updated.

It seems that the initial commit aa2e93b8e58e ("bpf: Add generic support
for update and delete batch ops") only enabled BPF_F_LOCK for
BPF_MAP_UPDATE_BATCH, but the document commit 0cb804547927 ("bpf:
Document BPF_MAP_*_BATCH syscall commands for BPF_MAP_UPDATE_BATCH
considered both BPF_NOEXIST and BPF_EXIST are valid. The
bpf_map_update_batch() API in libbpf also considered both BPF_NOEXIST
and BPF_EXIST are valid, but we just never test it before.
>
>> ---
>>   kernel/bpf/syscall.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 869265852d51..d85361f9a9b8 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -1852,7 +1852,7 @@ int generic_map_update_batch(struct bpf_map
>> *map, struct file *map_file,
>>       void *key, *value;
>>       int err = 0;
>>   -    if (attr->batch.elem_flags & ~BPF_F_LOCK)
>> +    if ((attr->batch.elem_flags & ~BPF_F_LOCK) > BPF_EXIST)
>>           return -EINVAL;
>>         if ((attr->batch.elem_flags & BPF_F_LOCK) &&
>>
>
> .

Re: [PATCH] bpf: fix excessively checking for elem_flags in batch update mode

Posted by Lin Feng 1 year, 4 months ago

Hi Tao,

See below please,

On 7/23/24 09:21, Hou Tao wrote:
> Hi,
> 
> On 7/20/2024 12:22 AM, Daniel Borkmann wrote:
>> On 7/17/24 1:15 PM, Lin Feng wrote:
>>> Currently generic_map_update_batch will reject all valid command
>>> flags for
>>> BPF_MAP_UPDATE_ELEM other than BPF_F_LOCK, which is overkill, map
>>> updating
>>> semantic does allow specify BPF_NOEXIST or BPF_EXIST even for batching
>>> update.
>>>
>>> Signed-off-by: Lin Feng <linf@wangsu.com>
>>
>> [ +Hou/Brian ]
>>
>> Please also add a BPF selftest along with this extension which
>> exercises the
>> batch update and validates the behavior for the various flags which
>> are now enabled.
> 
> Agreed. There are already some batched map operation tests in
> tools/testing/selftests/bpf/map_tests/htab_map_batch_ops.c, I think
> extending the test cases in the file will be fine.
>> Also, please discuss the semantics in the commit msg.. errors due to
>> BPF_EXIST and
>> BPF_NOEXIST will cause bpf_map_update_value() to fail and then break
>> the loop. It's
>> probably fine given batch.count (cp) will be propagated back to user
>> space to tell
>> how many elements could actually get updated.
> 
> It seems that the initial commit aa2e93b8e58e ("bpf: Add generic support
> for update and delete batch ops") only enabled BPF_F_LOCK for
> BPF_MAP_UPDATE_BATCH, but the document commit 0cb804547927 ("bpf:
> Document BPF_MAP_*_BATCH syscall commands for BPF_MAP_UPDATE_BATCH
> considered both BPF_NOEXIST and BPF_EXIST are valid. The
> bpf_map_update_batch() API in libbpf also considered both BPF_NOEXIST
> and BPF_EXIST are valid, but we just never test it before.

I did notice the conflict between those two commits, besides the already
supported update flags in single-update mode, the latter patch says "both
BPF_NOEXIST and BPF_EXIST are valid", so here came this patch.

And thank you again for your detailed analysis, so I need to extend the 
testsuits and confirm this one wouldn't break any exsiting ones, I will
resend them batch in next version.

Have a nice day,
linfeng

>>
>>> ---
>>>   kernel/bpf/syscall.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>>> index 869265852d51..d85361f9a9b8 100644
>>> --- a/kernel/bpf/syscall.c
>>> +++ b/kernel/bpf/syscall.c
>>> @@ -1852,7 +1852,7 @@ int generic_map_update_batch(struct bpf_map
>>> *map, struct file *map_file,
>>>       void *key, *value;
>>>       int err = 0;
>>>   -    if (attr->batch.elem_flags & ~BPF_F_LOCK)
>>> +    if ((attr->batch.elem_flags & ~BPF_F_LOCK) > BPF_EXIST)
>>>           return -EINVAL;
>>>         if ((attr->batch.elem_flags & BPF_F_LOCK) &&
>>>
>>
>> .
>

Re: [PATCH] bpf: fix excessively checking for elem_flags in batch update mode

Posted by Lin Feng 1 year, 4 months ago

Hi Daniel,

Thanks for your reply! Without basic knowledge of rules of thumb for patch in
bpf, I didn't expect a single line change need that many more considerations,
and will do some more work on it following your sugguestion!

Thanks,
linfeng

On 7/20/24 00:22, Daniel Borkmann wrote:
> On 7/17/24 1:15 PM, Lin Feng wrote:
>> Currently generic_map_update_batch will reject all valid command flags for
>> BPF_MAP_UPDATE_ELEM other than BPF_F_LOCK, which is overkill, map updating
>> semantic does allow specify BPF_NOEXIST or BPF_EXIST even for batching
>> update.
>>
>> Signed-off-by: Lin Feng <linf@wangsu.com>
> 
> [ +Hou/Brian ]
> 
> Please also add a BPF selftest along with this extension which exercises the
> batch update and validates the behavior for the various flags which are now enabled.
> 
> Also, please discuss the semantics in the commit msg.. errors due to BPF_EXIST and
> BPF_NOEXIST will cause bpf_map_update_value() to fail and then break the loop. It's
> probably fine given batch.count (cp) will be propagated back to user space to tell
> how many elements could actually get updated.
> 
>> ---
>>   kernel/bpf/syscall.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 869265852d51..d85361f9a9b8 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -1852,7 +1852,7 @@ int generic_map_update_batch(struct bpf_map *map, struct file *map_file,
>>   	void *key, *value;
>>   	int err = 0;
>>   
>> -	if (attr->batch.elem_flags & ~BPF_F_LOCK)
>> +	if ((attr->batch.elem_flags & ~BPF_F_LOCK) > BPF_EXIST)
>>   		return -EINVAL;
>>   
>>   	if ((attr->batch.elem_flags & BPF_F_LOCK) &&
>>
>