On Fri, Mar 19, 2021 at 09:35:59PM +0100, Daniel Borkmann wrote:
> On 3/16/21 6:44 PM, Denis Salopek wrote:
> [...]
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index c859bc46d06c..36f65b589b82 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -1463,7 +1463,7 @@ int generic_map_lookup_batch(struct bpf_map *map,
> > return err;
> > }
> > -#define BPF_MAP_LOOKUP_AND_DELETE_ELEM_LAST_FIELD value
> > +#define BPF_MAP_LOOKUP_AND_DELETE_ELEM_LAST_FIELD flags
> > static int map_lookup_and_delete_elem(union bpf_attr *attr)
> > {
> > @@ -1479,6 +1479,9 @@ static int map_lookup_and_delete_elem(union bpf_attr *attr)
> > if (CHECK_ATTR(BPF_MAP_LOOKUP_AND_DELETE_ELEM))
> > return -EINVAL;
> > + if (attr->flags & ~BPF_F_LOCK)
> > + return -EINVAL;
> > +
> > f = fdget(ufd);
> > map = __bpf_map_get(f);
> > if (IS_ERR(map))
> > @@ -1489,13 +1492,19 @@ static int map_lookup_and_delete_elem(union bpf_attr *attr)
> > goto err_put;
> > }
> > + if ((attr->flags & BPF_F_LOCK) &&
> > + !map_value_has_spin_lock(map)) {
> > + err = -EINVAL;
> > + goto err_put;
> > + }
> > +
> > key = __bpf_copy_key(ukey, map->key_size);
> > if (IS_ERR(key)) {
> > err = PTR_ERR(key);
> > goto err_put;
> > }
> > - value_size = map->value_size;
> > + value_size = bpf_map_value_size(map);
> > err = -ENOMEM;
> > value = kmalloc(value_size, GFP_USER | __GFP_NOWARN);
> > @@ -1505,6 +1514,17 @@ static int map_lookup_and_delete_elem(union bpf_attr *attr)
> > if (map->map_type == BPF_MAP_TYPE_QUEUE ||
> > map->map_type == BPF_MAP_TYPE_STACK) {
> > err = map->ops->map_pop_elem(map, value);
> > + } else if (map->map_type == BPF_MAP_TYPE_HASH ||
> > + map->map_type == BPF_MAP_TYPE_PERCPU_HASH ||
> > + map->map_type == BPF_MAP_TYPE_LRU_HASH ||
> > + map->map_type == BPF_MAP_TYPE_LRU_PERCPU_HASH) {
> > + if (!bpf_map_is_dev_bound(map)) {
>
> I think you probably rather meant to fold the above !bpf_map_is_dev_bound(map)
> condition into the higher level 'else if', right? Otherwise for dev bound maps
> you'll always end up with -ENOMEM error rather than -ENOTSUPP.
>
Yes, you're right, thank you. How about setting the err to -ENOTSUPP
before the 'if' and just drop the else altogether?
> > + bpf_disable_instrumentation();
> > + rcu_read_lock();
> > + err = map->ops->map_lookup_and_delete_elem(map, key, value, attr->flags);
> > + rcu_read_unlock();
> > + bpf_enable_instrumentation();
> > + }
> > } else {
> > err = -ENOTSUPP;
> > }
