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;
> +}
> +
[...]
