lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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