| Message ID | 20230717114307.46124-2-aspsk@isovalent.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | fix setting return values for htab batch ops and docs | expand |
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.
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.
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.
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;
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(-)