[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+XBgLWBDJbCgToWPW2FkRO_AkkaE8+DMnAk55vyT+KyLZv4Xg@mail.gmail.com>
Date: Mon, 21 Sep 2020 13:12:30 +0200
From: Luka Oreskovic <luka.oreskovic@...tura.hr>
To: Song Liu <song@...nel.org>
Cc: Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
Juraj Vijtiuk <juraj.vijtiuk@...tura.hr>,
Luka Perkov <luka.perkov@...tura.hr>
Subject: Re: [PATCH bpf-next] bpf: add support for other map types to bpf_map_lookup_and_delete_elem
On Fri, Sep 18, 2020 at 1:21 AM Song Liu <song@...nel.org> wrote:
>
> On Thu, Sep 17, 2020 at 7:16 AM Luka Oreskovic
> <luka.oreskovic@...tura.hr> wrote:
> >
> [...]
>
> > +++ b/kernel/bpf/syscall.c
> > @@ -1475,6 +1475,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;
> > +
>
> Please explain (in comments for commit log) the use of BPF_F_LOCK in
> the commit log,
> as it is new for BPF_MAP_LOOKUP_AND_DELETE_ELEM.
>
> > f = fdget(ufd);
> > map = __bpf_map_get(f);
> > if (IS_ERR(map))
> > @@ -1485,13 +1488,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);
> > @@ -1502,7 +1511,24 @@ static int map_lookup_and_delete_elem(union bpf_attr *attr)
> > map->map_type == BPF_MAP_TYPE_STACK) {
> > err = map->ops->map_pop_elem(map, value);
> > } else {
> > - err = -ENOTSUPP;
> > + err = bpf_map_copy_value(map, key, value, attr->flags);
> > + if (err)
> > + goto free_value;
>
> IIUC, we cannot guarantee the value returned is the same as the value we
> deleted. If this is true, I guess this may confuse the user with some
> concurrency
> bug.
>
> Thanks,
> Song
>
> [...]
Thank you very much for your review. This is my first time contributing
to the linux community, so I am very grateful for any input.
For the first point, you are correct, the commit message should
have been more detailed. As for the second point, I see the problem,
but I'm not sure how to resolve it. Maybe moving the
bpf_disable_instrumentation call could work, but I'm not sure if
that could create different problems. I'll try to find and acceptable
solution and resubmit the patch.
Best wishes,
Luka Oreskovic
Powered by blists - more mailing lists