[PATCH bpf-next 0/2] fix setting return values for htab batch ops and docs

[PATCH bpf-next 0/2] fix setting return values for htab batch ops and docs

[Anton Protopopov]

This is a small follow up to the conversation with Hou in the following
thread:

  https://lore.kernel.org/bpf/20230705160139.19967-1-aspsk@isovalent.com/T/#u

Namely, the conversation was about that comments in <linux/bpf.h>
describing the return values from the batch operations are not 100%
obvious. I tried to make comments more clear. While doing this I also
found that this is better to patch how __htab_map_lookup_and_delete_batch
sets return values: the output parameter count could be set to non-zero in
case of error, which may confuse some userspace apps (as errno && non-zero
counter is considered a partially successful operation for batch ops).

Anton Protopopov (2):
  bpf: fix setting return values for htab batch ops
  bpf: update uapi/linux/bpf.h docs on the batch map ops

 include/uapi/linux/bpf.h | 22 ++++++++++++----------
 kernel/bpf/hashtab.c     | 14 +++++++-------
 2 files changed, 19 insertions(+), 17 deletions(-)

-- 
2.34.1

[PATCH bpf-next 1/2] bpf: fix setting return values for htab batch ops

[Anton Protopopov]

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 <aspsk@isovalent.com>
---
 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

Re: [PATCH bpf-next 1/2] bpf: fix setting return values for htab batch ops

[Alexei Starovoitov]

On Mon, Jul 17, 2023 at 4:42 AM Anton Protopopov <aspsk@isovalent.com> 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 <aspsk@isovalent.com>
> ---
>  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.

Re: [PATCH bpf-next 1/2] bpf: fix setting return values for htab batch ops

[Anton Protopopov]

On Tue, Jul 18, 2023 at 05:52:38PM -0700, Alexei Starovoitov wrote:
> On Mon, Jul 17, 2023 at 4:42 AM Anton Protopopov <aspsk@isovalent.com> 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 <aspsk@isovalent.com>
> > ---
> >  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.

Re: [PATCH bpf-next 1/2] bpf: fix setting return values for htab batch ops

[Alexei Starovoitov]

On Wed, Jul 19, 2023 at 12:07 AM Anton Protopopov <aspsk@isovalent.com> 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 <aspsk@isovalent.com> 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 <aspsk@isovalent.com>
> > > ---
> > >  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.

[PATCH bpf-next 2/2] bpf: update uapi/linux/bpf.h docs on the batch map ops

[Anton Protopopov]

The map_lookup{,_and_delete}_batch operations return same values. Make
this clear in documentation. Also, update the comments so that this is
more clear that -ENOENT is a valid return value in case of success. (In
fact, this is the most common return value, as this is reasonable to do
map_lookup_batch(MAX_ENTRIES), which, in case of success, will always
return -ENOENT.)

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
---
 include/uapi/linux/bpf.h | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 600d0caebbd8..9e6e277bedab 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -632,17 +632,19 @@ union bpf_iter_link_info {
  *			returning the lock. This must be specified if the
  *			elements contain a spinlock.
  *
- *		On success, *count* elements from the map are copied into the
- *		user buffer, with the keys copied into *keys* and the values
- *		copied into the corresponding indices in *values*.
- *
- *		If an error is returned and *errno* is not **EFAULT**, *count*
- *		is set to the number of successfully processed elements.
+ *		On success, up to *count* elements from the map are copied into
+ *		the user buffer, with the keys copied into *keys* and the
+ *		values copied into the corresponding indices in *values*.
  *
  *	Return
  *		Returns zero on success. On error, -1 is returned and *errno*
  *		is set appropriately.
  *
+ *		If an error is returned and *errno* is not **EFAULT**, then
+ *		*count* is set to the number of successfully processed
+ *		elements. In particular, the *errno* may be set to **ENOENT**
+ *		in case of success to indicate that the end of map is reached.
+ *
  *		May set *errno* to **ENOSPC** to indicate that *keys* or
  *		*values* is too small to dump an entire bucket during
  *		iteration of a hash-based map type.
@@ -655,15 +657,15 @@ union bpf_iter_link_info {
  *		**BPF_MAP_LOOKUP_BATCH** with two exceptions:
  *
  *		* Every element that is successfully returned is also deleted
- *		  from the map. This is at least *count* elements. Note that
- *		  *count* is both an input and an output parameter.
+ *		  from the map. The *count* parameter is set to the number of
+ *		  returned elements. This value can be less than the actual
+ *		  number of deleted elements, see the next item.
  *		* Upon returning with *errno* set to **EFAULT**, up to
  *		  *count* elements may be deleted without returning the keys
  *		  and values of the deleted elements.
  *
  *	Return
- *		Returns zero on success. On error, -1 is returned and *errno*
- *		is set appropriately.
+ *		Same as the BPF_MAP_LOOKUP_BATCH return values.
  *
  * BPF_MAP_UPDATE_BATCH
  *	Description
-- 
2.34.1

Re: [PATCH bpf-next 2/2] bpf: update uapi/linux/bpf.h docs on the batch map ops

[Hou Tao]

Hi,

On 7/17/2023 7:43 PM, Anton Protopopov wrote:
> The map_lookup{,_and_delete}_batch operations return same values. Make
> this clear in documentation. Also, update the comments so that this is
> more clear that -ENOENT is a valid return value in case of success. (In
> fact, this is the most common return value, as this is reasonable to do
> map_lookup_batch(MAX_ENTRIES), which, in case of success, will always
> return -ENOENT.)
>
> Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> ---
>  include/uapi/linux/bpf.h | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 600d0caebbd8..9e6e277bedab 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -632,17 +632,19 @@ union bpf_iter_link_info {
>   *			returning the lock. This must be specified if the
>   *			elements contain a spinlock.
>   *
> - *		On success, *count* elements from the map are copied into the
> - *		user buffer, with the keys copied into *keys* and the values
> - *		copied into the corresponding indices in *values*.
> - *
> - *		If an error is returned and *errno* is not **EFAULT**, *count*
> - *		is set to the number of successfully processed elements.
> + *		On success, up to *count* elements from the map are copied into
> + *		the user buffer, with the keys copied into *keys* and the
> + *		values copied into the corresponding indices in *values*.
>   *
>   *	Return
>   *		Returns zero on success. On error, -1 is returned and *errno*
>   *		is set appropriately.
>   *
> + *		If an error is returned and *errno* is not **EFAULT**, then

Maybe we should say "is not EFAULT or EINVAL" ? Otherwise, the update
looks good to me, so

Acked-by: Hou Tao <houtao1@huawei.com>

> + *		*count* is set to the number of successfully processed
> + *		elements. In particular, the *errno* may be set to **ENOENT**
> + *		in case of success to indicate that the end of map is reached.
> + *
>   *		May set *errno* to **ENOSPC** to indicate that *keys* or
>   *		*values* is too small to dump an entire bucket during
>   *		iteration of a hash-based map type.
> @@ -655,15 +657,15 @@ union bpf_iter_link_info {
>   *		**BPF_MAP_LOOKUP_BATCH** with two exceptions:
>   *
>   *		* Every element that is successfully returned is also deleted
> - *		  from the map. This is at least *count* elements. Note that
> - *		  *count* is both an input and an output parameter.
> + *		  from the map. The *count* parameter is set to the number of
> + *		  returned elements. This value can be less than the actual
> + *		  number of deleted elements, see the next item.
>   *		* Upon returning with *errno* set to **EFAULT**, up to
>   *		  *count* elements may be deleted without returning the keys
>   *		  and values of the deleted elements.
>   *
>   *	Return
> - *		Returns zero on success. On error, -1 is returned and *errno*
> - *		is set appropriately.
> + *		Same as the BPF_MAP_LOOKUP_BATCH return values.
>   *
>   * BPF_MAP_UPDATE_BATCH
>   *	Description