The map_lookup{,_and_delete}_batch operations are expected to set the
output parameter, counter, to the number of elements successfully copied
to the user space. This is also expected to be true if an error is
returned and the errno is set to a value other than EFAULT. The current
implementation can return -EINVAL without setting the counter to zero, so
some userspace programs may confuse this with a [partially] successful
operation. Move code which sets the counter to zero to the top of the
function so that we always return a correct value.
Fixes: 057996380a42 ("bpf: Add batch ops to all htab bpf map")
Signed-off-by: Anton Protopopov <[email protected]>
---
kernel/bpf/hashtab.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index a8c7e1c5abfa..fa8e3f1e1724 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -1692,6 +1692,13 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map,
struct bucket *b;
int ret = 0;
+ max_count = attr->batch.count;
+ if (!max_count)
+ return 0;
+
+ if (put_user(0, &uattr->batch.count))
+ return -EFAULT;
+
elem_map_flags = attr->batch.elem_flags;
if ((elem_map_flags & ~BPF_F_LOCK) ||
((elem_map_flags & BPF_F_LOCK) && !btf_record_has_field(map->record, BPF_SPIN_LOCK)))
@@ -1701,13 +1708,6 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map,
if (map_flags)
return -EINVAL;
- max_count = attr->batch.count;
- if (!max_count)
- return 0;
-
- if (put_user(0, &uattr->batch.count))
- return -EFAULT;
-
batch = 0;
if (ubatch && copy_from_user(&batch, ubatch, sizeof(batch)))
return -EFAULT;
--
2.34.1
On Mon, Jul 17, 2023 at 4:42 AM Anton Protopopov <[email protected]> wrote:
>
> The map_lookup{,_and_delete}_batch operations are expected to set the
> output parameter, counter, to the number of elements successfully copied
> to the user space. This is also expected to be true if an error is
> returned and the errno is set to a value other than EFAULT. The current
> implementation can return -EINVAL without setting the counter to zero, so
> some userspace programs may confuse this with a [partially] successful
> operation. Move code which sets the counter to zero to the top of the
> function so that we always return a correct value.
>
> Fixes: 057996380a42 ("bpf: Add batch ops to all htab bpf map")
> Signed-off-by: Anton Protopopov <[email protected]>
> ---
> kernel/bpf/hashtab.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index a8c7e1c5abfa..fa8e3f1e1724 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -1692,6 +1692,13 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map,
> struct bucket *b;
> int ret = 0;
>
> + max_count = attr->batch.count;
> + if (!max_count)
> + return 0;
> +
> + if (put_user(0, &uattr->batch.count))
> + return -EFAULT;
> +
> elem_map_flags = attr->batch.elem_flags;
> if ((elem_map_flags & ~BPF_F_LOCK) ||
> ((elem_map_flags & BPF_F_LOCK) && !btf_record_has_field(map->record, BPF_SPIN_LOCK)))
> @@ -1701,13 +1708,6 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map,
> if (map_flags)
> return -EINVAL;
>
> - max_count = attr->batch.count;
> - if (!max_count)
> - return 0;
> -
> - if (put_user(0, &uattr->batch.count))
> - return -EFAULT;
> -
I hear your concern, but I don't think it's a good idea
to return 0 when flags were incorrect.
That will cause more suprises to user space.
I think the code is fine as-is.
On Tue, Jul 18, 2023 at 05:52:38PM -0700, Alexei Starovoitov wrote:
> On Mon, Jul 17, 2023 at 4:42 AM Anton Protopopov <[email protected]> wrote:
> >
> > The map_lookup{,_and_delete}_batch operations are expected to set the
> > output parameter, counter, to the number of elements successfully copied
> > to the user space. This is also expected to be true if an error is
> > returned and the errno is set to a value other than EFAULT. The current
> > implementation can return -EINVAL without setting the counter to zero, so
> > some userspace programs may confuse this with a [partially] successful
> > operation. Move code which sets the counter to zero to the top of the
> > function so that we always return a correct value.
> >
> > Fixes: 057996380a42 ("bpf: Add batch ops to all htab bpf map")
> > Signed-off-by: Anton Protopopov <[email protected]>
> > ---
> > kernel/bpf/hashtab.c | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> > index a8c7e1c5abfa..fa8e3f1e1724 100644
> > --- a/kernel/bpf/hashtab.c
> > +++ b/kernel/bpf/hashtab.c
> > @@ -1692,6 +1692,13 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map,
> > struct bucket *b;
> > int ret = 0;
> >
> > + max_count = attr->batch.count;
> > + if (!max_count)
> > + return 0;
> > +
> > + if (put_user(0, &uattr->batch.count))
> > + return -EFAULT;
> > +
> > elem_map_flags = attr->batch.elem_flags;
> > if ((elem_map_flags & ~BPF_F_LOCK) ||
> > ((elem_map_flags & BPF_F_LOCK) && !btf_record_has_field(map->record, BPF_SPIN_LOCK)))
> > @@ -1701,13 +1708,6 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map,
> > if (map_flags)
> > return -EINVAL;
> >
> > - max_count = attr->batch.count;
> > - if (!max_count)
> > - return 0;
> > -
> > - if (put_user(0, &uattr->batch.count))
> > - return -EFAULT;
> > -
>
> I hear your concern, but I don't think it's a good idea
> to return 0 when flags were incorrect.
> That will cause more suprises to user space.
> I think the code is fine as-is.
Yes, thanks, this makes sense. And actually we can do both:
max_count = attr->batch.count;
put_user(0, &uattr->batch.count);
/* check flags */
if (!max_count)
return 0;
This way we always set the userspace counter to a correct value
and also check flags in the right place.
On Wed, Jul 19, 2023 at 12:07 AM Anton Protopopov <[email protected]> wrote:
>
> On Tue, Jul 18, 2023 at 05:52:38PM -0700, Alexei Starovoitov wrote:
> > On Mon, Jul 17, 2023 at 4:42 AM Anton Protopopov <[email protected]> wrote:
> > >
> > > The map_lookup{,_and_delete}_batch operations are expected to set the
> > > output parameter, counter, to the number of elements successfully copied
> > > to the user space. This is also expected to be true if an error is
> > > returned and the errno is set to a value other than EFAULT. The current
> > > implementation can return -EINVAL without setting the counter to zero, so
> > > some userspace programs may confuse this with a [partially] successful
> > > operation. Move code which sets the counter to zero to the top of the
> > > function so that we always return a correct value.
> > >
> > > Fixes: 057996380a42 ("bpf: Add batch ops to all htab bpf map")
> > > Signed-off-by: Anton Protopopov <[email protected]>
> > > ---
> > > kernel/bpf/hashtab.c | 14 +++++++-------
> > > 1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> > > index a8c7e1c5abfa..fa8e3f1e1724 100644
> > > --- a/kernel/bpf/hashtab.c
> > > +++ b/kernel/bpf/hashtab.c
> > > @@ -1692,6 +1692,13 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map,
> > > struct bucket *b;
> > > int ret = 0;
> > >
> > > + max_count = attr->batch.count;
> > > + if (!max_count)
> > > + return 0;
> > > +
> > > + if (put_user(0, &uattr->batch.count))
> > > + return -EFAULT;
> > > +
> > > elem_map_flags = attr->batch.elem_flags;
> > > if ((elem_map_flags & ~BPF_F_LOCK) ||
> > > ((elem_map_flags & BPF_F_LOCK) && !btf_record_has_field(map->record, BPF_SPIN_LOCK)))
> > > @@ -1701,13 +1708,6 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map,
> > > if (map_flags)
> > > return -EINVAL;
> > >
> > > - max_count = attr->batch.count;
> > > - if (!max_count)
> > > - return 0;
> > > -
> > > - if (put_user(0, &uattr->batch.count))
> > > - return -EFAULT;
> > > -
> >
> > I hear your concern, but I don't think it's a good idea
> > to return 0 when flags were incorrect.
> > That will cause more suprises to user space.
> > I think the code is fine as-is.
>
> Yes, thanks, this makes sense. And actually we can do both:
>
> max_count = attr->batch.count;
> put_user(0, &uattr->batch.count);
> /* check flags */
> if (!max_count)
> return 0;
>
> This way we always set the userspace counter to a correct value
> and also check flags in the right place.
Looks too convoluted to me.
I think concerns over user space always assuming batch.count
is updated with zero even when it calls api incorrectly are overblown.