| Message ID | 20210429130510.1621665-1-jackmanb@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | BPF |
| Headers | show |
| Series | [v2,bpf-next] libbpf: Fix signed overflow in ringbuf_process_ring | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 5 maintainers not CCed: netdev@vger.kernel.org yhs@fb.com kafai@fb.com john.fastabend@gmail.com songliubraving@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 28 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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 >
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!
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")
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;
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;
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) {
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> --- -- 2.31.1.498.g6c1eba8ee3d-goog