[PATCH v2 bpf-next] libbpf: Fix signed overflow in ringbuf_process_ring

[PATCH v2 bpf-next] libbpf: Fix signed overflow in ringbuf_process_ring

[Brendan Jackman]

One of our benchmarks running in (Google-internal) CI pushes data
through the ringbuf faster htan than userspace is able to consume
it. In this case it seems we're actually able to get >INT_MAX entries
in a single ringbuf_buffer__consume call. ASAN detected that cnt
overflows in this case.

Fix by using 64-bit counter internally and then capping the result to
INT_MAX before converting to the int return type.

Fixes: bf99c936f947 (libbpf: Add BPF ring buffer support)
Signed-off-by: Brendan Jackman <jackmanb@google.com>
---

diff v1->v2: Now we don't break the loop at INT_MAX, we just cap the reported
entry count.

Note: I feel a bit guilty about the fact that this makes the reader
think about implicit conversions. Nobody likes thinking about that.

But explicit casts don't really help with clarity:

  return (int)min(cnt, (int64_t)INT_MAX); // ugh

shrug..

 tools/lib/bpf/ringbuf.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c
index e7a8d847161f..2e114c2d0047 100644
--- a/tools/lib/bpf/ringbuf.c
+++ b/tools/lib/bpf/ringbuf.c
@@ -204,7 +204,9 @@ static inline int roundup_len(__u32 len)

 static int ringbuf_process_ring(struct ring* r)
 {
-	int *len_ptr, len, err, cnt = 0;
+	int *len_ptr, len, err;
+	/* 64-bit to avoid overflow in case of extreme application behavior */
+	int64_t cnt = 0;
 	unsigned long cons_pos, prod_pos;
 	bool got_new_data;
 	void *sample;
@@ -240,7 +242,7 @@ static int ringbuf_process_ring(struct ring* r)
 		}
 	} while (got_new_data);
 done:
-	return cnt;
+	return min(cnt, INT_MAX);
 }

 /* Consume available ring buffer(s) data without event polling.
@@ -263,8 +265,8 @@ int ring_buffer__consume(struct ring_buffer *rb)
 }

 /* Poll for available data and consume records, if any are available.
- * Returns number of records consumed, or negative number, if any of the
- * registered callbacks returned error.
+ * Returns number of records consumed (or INT_MAX, whichever is less), or
+ * negative number, if any of the registered callbacks returned error.
  */
 int ring_buffer__poll(struct ring_buffer *rb, int timeout_ms)
 {
--
2.31.1.498.g6c1eba8ee3d-goog

Re: [PATCH v2 bpf-next] libbpf: Fix signed overflow in ringbuf_process_ring

[Andrii Nakryiko]

On Thu, Apr 29, 2021 at 6:05 AM Brendan Jackman <jackmanb@google.com> wrote:
>
> One of our benchmarks running in (Google-internal) CI pushes data
> through the ringbuf faster htan than userspace is able to consume
> it. In this case it seems we're actually able to get >INT_MAX entries
> in a single ringbuf_buffer__consume call. ASAN detected that cnt
> overflows in this case.
>
> Fix by using 64-bit counter internally and then capping the result to
> INT_MAX before converting to the int return type.
>
> Fixes: bf99c936f947 (libbpf: Add BPF ring buffer support)
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---
>
> diff v1->v2: Now we don't break the loop at INT_MAX, we just cap the reported
> entry count.
>
> Note: I feel a bit guilty about the fact that this makes the reader
> think about implicit conversions. Nobody likes thinking about that.
>
> But explicit casts don't really help with clarity:
>
>   return (int)min(cnt, (int64_t)INT_MAX); // ugh
>

I'd go with

if (cnt > INT_MAX)
    return INT_MAX;

return cnt;

If you don't mind, I can patch it up while applying?

> shrug..
>
>  tools/lib/bpf/ringbuf.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c
> index e7a8d847161f..2e114c2d0047 100644
> --- a/tools/lib/bpf/ringbuf.c
> +++ b/tools/lib/bpf/ringbuf.c
> @@ -204,7 +204,9 @@ static inline int roundup_len(__u32 len)
>
>  static int ringbuf_process_ring(struct ring* r)
>  {
> -       int *len_ptr, len, err, cnt = 0;
> +       int *len_ptr, len, err;
> +       /* 64-bit to avoid overflow in case of extreme application behavior */
> +       int64_t cnt = 0;
>         unsigned long cons_pos, prod_pos;
>         bool got_new_data;
>         void *sample;
> @@ -240,7 +242,7 @@ static int ringbuf_process_ring(struct ring* r)
>                 }
>         } while (got_new_data);
>  done:
> -       return cnt;
> +       return min(cnt, INT_MAX);
>  }
>
>  /* Consume available ring buffer(s) data without event polling.
> @@ -263,8 +265,8 @@ int ring_buffer__consume(struct ring_buffer *rb)
>  }
>
>  /* Poll for available data and consume records, if any are available.
> - * Returns number of records consumed, or negative number, if any of the
> - * registered callbacks returned error.
> + * Returns number of records consumed (or INT_MAX, whichever is less), or
> + * negative number, if any of the registered callbacks returned error.
>   */
>  int ring_buffer__poll(struct ring_buffer *rb, int timeout_ms)
>  {
> --
> 2.31.1.498.g6c1eba8ee3d-goog
>

Re: [PATCH v2 bpf-next] libbpf: Fix signed overflow in ringbuf_process_ring

[Brendan Jackman]

On Fri, 30 Apr 2021 at 18:31, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Apr 29, 2021 at 6:05 AM Brendan Jackman <jackmanb@google.com> wrote:

> > Note: I feel a bit guilty about the fact that this makes the reader
> > think about implicit conversions. Nobody likes thinking about that.
> >
> > But explicit casts don't really help with clarity:
> >
> >   return (int)min(cnt, (int64_t)INT_MAX); // ugh
> >
>
> I'd go with
>
> if (cnt > INT_MAX)
>     return INT_MAX;
>
> return cnt;

Sure, it has all the same implicit casts/promotions but I guess it's
clearer anyway.

> If you don't mind, I can patch it up while applying?

Yes please do, thanks!

Re: [PATCH v2 bpf-next] libbpf: Fix signed overflow in ringbuf_process_ring

[Andrii Nakryiko]

On Mon, May 3, 2021 at 5:01 AM Brendan Jackman <jackmanb@google.com> wrote:
>
> On Fri, 30 Apr 2021 at 18:31, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Apr 29, 2021 at 6:05 AM Brendan Jackman <jackmanb@google.com> wrote:
>
> > > Note: I feel a bit guilty about the fact that this makes the reader
> > > think about implicit conversions. Nobody likes thinking about that.
> > >
> > > But explicit casts don't really help with clarity:
> > >
> > >   return (int)min(cnt, (int64_t)INT_MAX); // ugh
> > >
> >
> > I'd go with
> >
> > if (cnt > INT_MAX)
> >     return INT_MAX;
> >
> > return cnt;
>
> Sure, it has all the same implicit casts/promotions but I guess it's
> clearer anyway.

I might be wrong, but given INT_MAX is defined as

#  define INT_MAX      2147483647

(notice no suffix specifying which type it is), this constant will be
interpreted by compiler as desired type in the given context. So in

if (cnt > INT_MAX)

INT_MAX should be a uint64_t constant. But even if not, it is
up-converted to int64_t with no loss anyway.

>
> > If you don't mind, I can patch it up while applying?
>
> Yes please do, thanks!

So while doing that I noticed that you didn't fix ring_buffer__poll(),
so I had to fix it up a bit more extensively. Please check the end
result in bpf tree and let me know if there are any problems with it:

2a30f9440640 ("libbpf: Fix signed overflow in ringbuf_process_ring")

Re: [PATCH v2 bpf-next] libbpf: Fix signed overflow in ringbuf_process_ring

[Brendan Jackman]

On Mon, 3 May 2021 at 19:46, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Mon, May 3, 2021 at 5:01 AM Brendan Jackman <jackmanb@google.com> wrote:
> >
> > On Fri, 30 Apr 2021 at 18:31, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> So while doing that I noticed that you didn't fix ring_buffer__poll(),
> so I had to fix it up a bit more extensively. Please check the end
> result in bpf tree and let me know if there are any problems with it:
>
> 2a30f9440640 ("libbpf: Fix signed overflow in ringbuf_process_ring")

Ah, thanks for that. Yep, the additional fix looks good to me.

I think it actually fixes another very niche issue:

 int ring_buffer__poll(struct ring_buffer *rb, int timeout_ms)
 {
-       int i, cnt, err, res = 0;
+       int i, cnt;
+       int64_t err, res = 0;

        cnt = epoll_wait(rb->epoll_fd, rb->events, rb->ring_cnt, timeout_ms);
+       if (cnt < 0)
+               return -errno;
+
        for (i = 0; i < cnt; i++) {
                __u32 ring_id = rb->events[i].data.fd;
                struct ring *ring = &rb->rings[ring_id];
@@ -280,7 +290,9 @@ int ring_buffer__poll(struct ring_buffer *rb, int
timeout_ms)
                        return err;
                res += err;
        }
-       return cnt < 0 ? -errno : res;

If the callback returns an error but errno is 0 this fails to report the error.

errno(3) says "the value of errno is never set to zero by any system
call or library function" but then describes a scenario where an
application might usefully set it to zero itself. Maybe it can also be
0 in new threads, depending on your metaphysical interpretation of "by
a system call or library function".

+       if (res > INT_MAX)
+               return INT_MAX;
+       return res;

Re: [PATCH v2 bpf-next] libbpf: Fix signed overflow in ringbuf_process_ring

[Andrii Nakryiko]

On Tue, May 4, 2021 at 2:01 AM Brendan Jackman <jackmanb@google.com> wrote:
>
> On Mon, 3 May 2021 at 19:46, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, May 3, 2021 at 5:01 AM Brendan Jackman <jackmanb@google.com> wrote:
> > >
> > > On Fri, 30 Apr 2021 at 18:31, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> > So while doing that I noticed that you didn't fix ring_buffer__poll(),
> > so I had to fix it up a bit more extensively. Please check the end
> > result in bpf tree and let me know if there are any problems with it:
> >
> > 2a30f9440640 ("libbpf: Fix signed overflow in ringbuf_process_ring")
>
> Ah, thanks for that. Yep, the additional fix looks good to me.
>
> I think it actually fixes another very niche issue:
>
>  int ring_buffer__poll(struct ring_buffer *rb, int timeout_ms)
>  {
> -       int i, cnt, err, res = 0;
> +       int i, cnt;
> +       int64_t err, res = 0;
>
>         cnt = epoll_wait(rb->epoll_fd, rb->events, rb->ring_cnt, timeout_ms);
> +       if (cnt < 0)
> +               return -errno;
> +
>         for (i = 0; i < cnt; i++) {
>                 __u32 ring_id = rb->events[i].data.fd;
>                 struct ring *ring = &rb->rings[ring_id];
> @@ -280,7 +290,9 @@ int ring_buffer__poll(struct ring_buffer *rb, int
> timeout_ms)
>                         return err;
>                 res += err;
>         }
> -       return cnt < 0 ? -errno : res;
>
> If the callback returns an error but errno is 0 this fails to report the error.

Yeah, there was no need to be clever about that. Explicit if (cnt < 0)
check is obvious and correct.

>
> errno(3) says "the value of errno is never set to zero by any system
> call or library function" but then describes a scenario where an
> application might usefully set it to zero itself. Maybe it can also be
> 0 in new threads, depending on your metaphysical interpretation of "by
> a system call or library function".
>
> +       if (res > INT_MAX)
> +               return INT_MAX;
> +       return res;