Re: [PATCH v3 bpf-next 03/11] bpf: add generic support for update and delete batch ops

[Yonghong Song]

On 12/11/19 2:33 PM, 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     | 117 ++++++++++++++++++++++++++++++++++++++-
>   3 files changed, 128 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index a16f209255a59..851fb3ff084b0 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -48,6 +48,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);
> @@ -849,6 +853,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 36d3b885ddedd..dab24a763e4bb 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 708aa89fe2308..8272e76183068 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1206,6 +1206,111 @@ 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);
> +	u32 cp, max_count;
> +	int err = 0;
> +	void *key;
> +
> +	if (attr->batch.elem_flags & ~BPF_F_LOCK)
> +		return -EINVAL;
> +
> +	if ((attr->batch.elem_flags & BPF_F_LOCK) &&
> +	    !map_value_has_spin_lock(map)) {
> +		return -EINVAL;
> +	}
> +
> +	max_count = attr->batch.count;
> +	if (!max_count)
> +		return -EINVAL;

To be consistent with lookup and lookup_and_delete, if max_count = 0,
we can just return 0 instead of -EINVAL.

> +
> +	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 (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;
> +	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 = 0;
> +
> +	f = fdget(ufd);

I did not find the pairing fdput(). Also,
the variable 'f' is used way down in the loop, so
you can do fdget() later.

> +	if (attr->batch.elem_flags & ~BPF_F_LOCK)
> +		return -EINVAL;
> +
> +	if ((attr->batch.elem_flags & BPF_F_LOCK) &&
> +	    !map_value_has_spin_lock(map)) {
> +		return -EINVAL;
> +	}
> +
> +	value_size = bpf_map_value_size(map);
> +
> +	max_count = attr->batch.count;
> +	if (!max_count)
> +		return 0;
> +
> +	value = kmalloc(value_size, GFP_USER | __GFP_NOWARN);
> +	if (!value)
> +		return -ENOMEM;
> +
> +	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;
> +		}
> +		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;
> +
> +	kfree(value);
> +	kfree(key);
> +	return err;
> +}
> +
[...]