Re: [PATCH v3 bpf-next] bpf: add lookup_and_delete_elem support to hashtab

[Denis Salopek <denis.salopek@sartura.hr>]

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;
> >   	}