| Message ID | 20230914231123.193901-2-martin.kelly@crowdstrike.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | add libbpf getters for individual ringbuffers | expand |
On Thu, Sep 14, 2023 at 4:12 PM Martin Kelly <martin.kelly@crowdstrike.com> wrote: > > Refactor the cleanup code in ring_buffer__add to use a unified err_out > label. This reduces code duplication, as well as plugging a potential > leak if mmap_sz != (__u64)(size_t)mmap_sz (currently this would miss > unmapping tmp because ringbuf_unmap_ring isn't called). > > Signed-off-by: Martin Kelly <martin.kelly@crowdstrike.com> > --- > tools/lib/bpf/ringbuf.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c > index 02199364db13..f2020807996c 100644 > --- a/tools/lib/bpf/ringbuf.c > +++ b/tools/lib/bpf/ringbuf.c > @@ -118,10 +118,9 @@ int ring_buffer__add(struct ring_buffer *rb, int map_fd, > /* Map writable consumer page */ > tmp = mmap(NULL, rb->page_size, PROT_READ | PROT_WRITE, MAP_SHARED, map_fd, 0); > if (tmp == MAP_FAILED) { > - err = -errno; removing this is wrong, pr_warn() can clobber errno > pr_warn("ringbuf: failed to mmap consumer page for map fd=%d: %d\n", > map_fd, err); > - return libbpf_err(err); > + goto err_out; > } > r->consumer_pos = tmp; > > @@ -131,16 +130,15 @@ int ring_buffer__add(struct ring_buffer *rb, int map_fd, > */ > mmap_sz = rb->page_size + 2 * (__u64)info.max_entries; > if (mmap_sz != (__u64)(size_t)mmap_sz) { > + errno = E2BIG; ditto, stick to err > pr_warn("ringbuf: ring buffer size (%u) is too big\n", info.max_entries); > - return libbpf_err(-E2BIG); > + goto err_out; > } > tmp = mmap(NULL, (size_t)mmap_sz, PROT_READ, MAP_SHARED, map_fd, rb->page_size); > if (tmp == MAP_FAILED) { > - err = -errno; > - ringbuf_unmap_ring(rb, r); > pr_warn("ringbuf: failed to mmap data pages for map fd=%d: %d\n", > map_fd, err); > - return libbpf_err(err); > + goto err_out; > } > r->producer_pos = tmp; > r->data = tmp + rb->page_size; > @@ -151,15 +149,18 @@ int ring_buffer__add(struct ring_buffer *rb, int map_fd, > e->events = EPOLLIN; > e->data.fd = rb->ring_cnt; > if (epoll_ctl(rb->epoll_fd, EPOLL_CTL_ADD, map_fd, e) < 0) { > - err = -errno; > - ringbuf_unmap_ring(rb, r); > pr_warn("ringbuf: failed to epoll add map fd=%d: %d\n", > map_fd, err); > - return libbpf_err(err); > + goto err_out; > } > > rb->ring_cnt++; > return 0; > + > +err_out: > + err = -errno; > + ringbuf_unmap_ring(rb, r); > + return libbpf_err(err); > } > > void ring_buffer__free(struct ring_buffer *rb) > -- > 2.34.1 >
diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c index 02199364db13..f2020807996c 100644 --- a/tools/lib/bpf/ringbuf.c +++ b/tools/lib/bpf/ringbuf.c @@ -118,10 +118,9 @@ int ring_buffer__add(struct ring_buffer *rb, int map_fd, /* Map writable consumer page */ tmp = mmap(NULL, rb->page_size, PROT_READ | PROT_WRITE, MAP_SHARED, map_fd, 0); if (tmp == MAP_FAILED) { - err = -errno; pr_warn("ringbuf: failed to mmap consumer page for map fd=%d: %d\n", map_fd, err); - return libbpf_err(err); + goto err_out; } r->consumer_pos = tmp; @@ -131,16 +130,15 @@ int ring_buffer__add(struct ring_buffer *rb, int map_fd, */ mmap_sz = rb->page_size + 2 * (__u64)info.max_entries; if (mmap_sz != (__u64)(size_t)mmap_sz) { + errno = E2BIG; pr_warn("ringbuf: ring buffer size (%u) is too big\n", info.max_entries); - return libbpf_err(-E2BIG); + goto err_out; } tmp = mmap(NULL, (size_t)mmap_sz, PROT_READ, MAP_SHARED, map_fd, rb->page_size); if (tmp == MAP_FAILED) { - err = -errno; - ringbuf_unmap_ring(rb, r); pr_warn("ringbuf: failed to mmap data pages for map fd=%d: %d\n", map_fd, err); - return libbpf_err(err); + goto err_out; } r->producer_pos = tmp; r->data = tmp + rb->page_size; @@ -151,15 +149,18 @@ int ring_buffer__add(struct ring_buffer *rb, int map_fd, e->events = EPOLLIN; e->data.fd = rb->ring_cnt; if (epoll_ctl(rb->epoll_fd, EPOLL_CTL_ADD, map_fd, e) < 0) { - err = -errno; - ringbuf_unmap_ring(rb, r); pr_warn("ringbuf: failed to epoll add map fd=%d: %d\n", map_fd, err); - return libbpf_err(err); + goto err_out; } rb->ring_cnt++; return 0; + +err_out: + err = -errno; + ringbuf_unmap_ring(rb, r); + return libbpf_err(err); } void ring_buffer__free(struct ring_buffer *rb)
Refactor the cleanup code in ring_buffer__add to use a unified err_out label. This reduces code duplication, as well as plugging a potential leak if mmap_sz != (__u64)(size_t)mmap_sz (currently this would miss unmapping tmp because ringbuf_unmap_ring isn't called). Signed-off-by: Martin Kelly <martin.kelly@crowdstrike.com> --- tools/lib/bpf/ringbuf.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)