Re: [PATCH v2 bpf-next 3/9] bpf: add generic support for update and delete batch ops

[Yonghong Song]

On 11/19/19 11:30 AM, Brian Vazquez wrote:
> This commit adds generic support for update and delete batch ops that
> can be used for almost all the bpf maps. These commands share the same
> UAPI attr that lookup and lookup_and_delete batch ops use and the
> syscall commands are:
> 
>    BPF_MAP_UPDATE_BATCH
>    BPF_MAP_DELETE_BATCH
> 
> The main difference between update/delete and lookup/lookup_and_delete
> batch ops is that for update/delete keys/values must be specified for
> userspace and because of that, neither in_batch nor out_batch are used.
> 
> Suggested-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> Signed-off-by: Brian Vazquez <brianvv@xxxxxxxxxx>
> Signed-off-by: Yonghong Song <yhs@xxxxxx>
> ---
>   include/linux/bpf.h      |  10 ++++
>   include/uapi/linux/bpf.h |   2 +
>   kernel/bpf/syscall.c     | 126 ++++++++++++++++++++++++++++++++++++++-
>   3 files changed, 137 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 767a823dbac74..96a19e1fd2b5b 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -46,6 +46,10 @@ struct bpf_map_ops {
>   	int (*map_lookup_and_delete_batch)(struct bpf_map *map,
>   					   const union bpf_attr *attr,
>   					   union bpf_attr __user *uattr);
> +	int (*map_update_batch)(struct bpf_map *map, const union bpf_attr *attr,
> +				union bpf_attr __user *uattr);
> +	int (*map_delete_batch)(struct bpf_map *map, const union bpf_attr *attr,
> +				union bpf_attr __user *uattr);
>   
>   	/* funcs callable from userspace and from eBPF programs */
>   	void *(*map_lookup_elem)(struct bpf_map *map, void *key);
> @@ -808,6 +812,12 @@ int  generic_map_lookup_batch(struct bpf_map *map,
>   int  generic_map_lookup_and_delete_batch(struct bpf_map *map,
>   					 const union bpf_attr *attr,
>   					 union bpf_attr __user *uattr);
> +int  generic_map_update_batch(struct bpf_map *map,
> +			      const union bpf_attr *attr,
> +			      union bpf_attr __user *uattr);
> +int  generic_map_delete_batch(struct bpf_map *map,
> +			      const union bpf_attr *attr,
> +			      union bpf_attr __user *uattr);
>   
>   extern int sysctl_unprivileged_bpf_disabled;
>   
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index e60b7b7cda61a..0f6ff0c4d79dd 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -109,6 +109,8 @@ enum bpf_cmd {
>   	BPF_BTF_GET_NEXT_ID,
>   	BPF_MAP_LOOKUP_BATCH,
>   	BPF_MAP_LOOKUP_AND_DELETE_BATCH,
> +	BPF_MAP_UPDATE_BATCH,
> +	BPF_MAP_DELETE_BATCH,
>   };
>   
>   enum bpf_map_type {
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index d0d3d0e0eaca4..06e1bcf40fb8d 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1127,6 +1127,120 @@ static int map_get_next_key(union bpf_attr *attr)
>   	return err;
>   }
>   
> +int generic_map_delete_batch(struct bpf_map *map,
> +			     const union bpf_attr *attr,
> +			     union bpf_attr __user *uattr)
> +{
> +	void __user *keys = u64_to_user_ptr(attr->batch.keys);
> +	int ufd = attr->map_fd;
> +	u32 cp, max_count;
> +	struct fd f;
> +	void *key;
> +	int err;
> +
> +	f = fdget(ufd);
> +	if (attr->batch.elem_flags & ~BPF_F_LOCK)
> +		return -EINVAL;
> +
> +	if ((attr->batch.elem_flags & BPF_F_LOCK) &&
> +	    !map_value_has_spin_lock(map)) {
> +		err = -EINVAL;
> +		goto err_put;

Just return -EINVAL?

> +	}
> +
> +	max_count = attr->batch.count;
> +	if (!max_count)
> +		return 0;
> +
> +	err = -ENOMEM;

Why initialize err to -ENOMEM? Maybe just err = 0.

> +	for (cp = 0; cp < max_count; cp++) {
> +		key = __bpf_copy_key(keys + cp * map->key_size, map->key_size);
> +		if (IS_ERR(key)) {
> +			err = PTR_ERR(key);
> +			break;
> +		}
> +
> +		if (err)
> +			break;

The above is incorrect, esp. if you assign err initial value to -ENOMEM.
The above ` if (err) break; ` is not really needed. If there is error,
you already break in the above.
If map->key_size is not 0, the return value 'key' cannot be NULL pointer.

> +		if (bpf_map_is_dev_bound(map)) {
> +			err = bpf_map_offload_delete_elem(map, key);
> +			break;
> +		}
> +
> +		preempt_disable();
> +		__this_cpu_inc(bpf_prog_active);
> +		rcu_read_lock();
> +		err = map->ops->map_delete_elem(map, key);
> +		rcu_read_unlock();
> +		__this_cpu_dec(bpf_prog_active);
> +		preempt_enable();
> +		maybe_wait_bpf_programs(map);
> +		if (err)
> +			break;
> +	}
> +	if (copy_to_user(&uattr->batch.count, &cp, sizeof(cp)))
> +		err = -EFAULT;

If previous err = -EFAULT, even if copy_to_user() succeeded,
return value will be -EFAULT, so uattr->batch.count cannot be
trusted. So may be do
    if (err != -EFAULT && copy_to_user(...))
       err = -EFAULT
?
There are several other places like this.

> +err_put:

You don't need err_put label in the above.

> +	return err;
> +}
> +int generic_map_update_batch(struct bpf_map *map,
> +			     const union bpf_attr *attr,
> +			     union bpf_attr __user *uattr)
> +{
> +	void __user *values = u64_to_user_ptr(attr->batch.values);
> +	void __user *keys = u64_to_user_ptr(attr->batch.keys);
> +	u32 value_size, cp, max_count;
> +	int ufd = attr->map_fd;
> +	void *key, *value;
> +	struct fd f;
> +	int err;
> +
> +	f = fdget(ufd);
> +	if (attr->batch.elem_flags & ~BPF_F_LOCK)
> +		return -EINVAL;
> +
> +	if ((attr->batch.elem_flags & BPF_F_LOCK) &&
> +	    !map_value_has_spin_lock(map)) {
> +		err = -EINVAL;
> +		goto err_put;

Directly return -EINVAL?

> +	}
> +
> +	value_size = bpf_map_value_size(map);
> +
> +	max_count = attr->batch.count;
> +	if (!max_count)
> +		return 0;
> +
> +	err = -ENOMEM;
> +	value = kmalloc(value_size, GFP_USER | __GFP_NOWARN);
> +	if (!value)
> +		goto err_put;

Directly return -ENOMEM?

> +
> +	for (cp = 0; cp < max_count; cp++) {
> +		key = __bpf_copy_key(keys + cp * map->key_size, map->key_size);

Do you need to free 'key' after its use?

> +		if (IS_ERR(key)) {
> +			err = PTR_ERR(key);
> +			break;
> +		}
> +		err = -EFAULT;
> +		if (copy_from_user(value, values + cp * value_size, value_size))
> +			break;
> +
> +		err = bpf_map_update_value(map, f, key, value,
> +					   attr->batch.elem_flags);
> +
> +		if (err)
> +			break;
> +	}
> +
> +	if (copy_to_user(&uattr->batch.count, &cp, sizeof(cp)))
> +		err = -EFAULT;

Similar to the above comment, if err already -EFAULT, no need
to do copy_to_user().

> +
> +	kfree(value);
> +err_put:

err_put label is not needed.

> +	return err;
> +}
> +
>   static int __generic_map_lookup_batch(struct bpf_map *map,
>   				      const union bpf_attr *attr,
>   				      union bpf_attr __user *uattr,
> @@ -3117,8 +3231,12 @@ static int bpf_map_do_batch(const union bpf_attr *attr,
>   
>   	if (cmd == BPF_MAP_LOOKUP_BATCH)
>   		BPF_DO_BATCH(map->ops->map_lookup_batch);
> -	else
> +	else if (cmd == BPF_MAP_LOOKUP_AND_DELETE_BATCH)
>   		BPF_DO_BATCH(map->ops->map_lookup_and_delete_batch);
> +	else if (cmd == BPF_MAP_UPDATE_BATCH)
> +		BPF_DO_BATCH(map->ops->map_update_batch);
> +	else
> +		BPF_DO_BATCH(map->ops->map_delete_batch);

Also need to check map_get_sys_perms() permissions for these two new 
commands. Both delete and update needs FMODE_CAN_WRITE permission.

>   
>   err_put:
>   	fdput(f);
> @@ -3229,6 +3347,12 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
>   		err = bpf_map_do_batch(&attr, uattr,
>   				       BPF_MAP_LOOKUP_AND_DELETE_BATCH);
>   		break;
> +	case BPF_MAP_UPDATE_BATCH:
> +		err = bpf_map_do_batch(&attr, uattr, BPF_MAP_UPDATE_BATCH);
> +		break;
> +	case BPF_MAP_DELETE_BATCH:
> +		err = bpf_map_do_batch(&attr, uattr, BPF_MAP_DELETE_BATCH);
> +		break;
>   	default:
>   		err = -EINVAL;
>   		break;
>