On Thu, Jul 27, 2023 at 7:48 AM Andrew Werner <andrew@xxxxxxxxxxxxxxxxx> wrote: > > On Thu, Jul 27, 2023 at 4:33 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote: > > > > On Mon, Jul 24, 2023 at 09:25:45AM -0400, Andrew Werner wrote: > > > Before this patch, the producer position could overflow `unsigned > > > long`, in which case libbpf would forever stop processing new writes to > > > the ringbuf. This patch addresses that bug by avoiding ordered > > > comparison between the consumer and producer position. If the consumer > > > position is greater than the producer position, the assumption is that > > > the producer has overflowed. > > > > > > A more defensive check could be to ensure that the delta is within > > > the allowed range, but such defensive checks are neither present in > > > the kernel side code nor in libbpf. The overflow that this patch > > > handles can occur while the producer and consumer follow a correct > > > protocol. > > > > > > A selftest was written to demonstrate the bug, and indeed this patch > > > allows the test to continue to make progress past the overflow. > > > However, the author was unable to create a testing environment on a > > > 32-bit machine, and the test requires substantial memory and over 4 > > > hours to hit the overflow point on a 64-bit machine. Thus, the test > > > is not included in this patch because of the impracticality of running > > > it. Are you saying on a 64-bit host you were able to overflow 64-bit integer in 4 hours? Interesting. Please share the test anyway. As a patch or github link. Anything works. > > > > > > Additionally, this patch adds commentary around a separate point to note > > > that the modular arithmetic is valid in the face of overflows, as that > > > fact may not be obvious to future readers. > > > > > > v1->v2: > > > - Fixed comment grammar. > > > - Properly formatted subject line. > > > > > > Reference: > > > [v1]: https://lore.kernel.org/bpf/20230724132404.1280848-1-awerner32@xxxxxxxxx/T/#u > > > > > > Signed-off-by: Andrew Werner <awerner32@xxxxxxxxx> > > > --- > > > tools/lib/bpf/ringbuf.c | 11 ++++++++++- > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c > > > index 02199364db13..2055f3099843 100644 > > > --- a/tools/lib/bpf/ringbuf.c > > > +++ b/tools/lib/bpf/ringbuf.c > > > @@ -237,7 +237,11 @@ static int64_t ringbuf_process_ring(struct ring *r) > > > do { > > > got_new_data = false; > > > prod_pos = smp_load_acquire(r->producer_pos); > > > - while (cons_pos < prod_pos) { > > > + > > > + /* Avoid signed comparisons between the positions; the producer > > > + * position can overflow before the consumer position. > > > + */ > > > + while (cons_pos != prod_pos) { > > > len_ptr = r->data + (cons_pos & r->mask); > > > len = smp_load_acquire(len_ptr); > > > > > > @@ -498,6 +502,11 @@ void *user_ring_buffer__reserve(struct user_ring_buffer *rb, __u32 size) > > > prod_pos = smp_load_acquire(rb->producer_pos); > > > > > > max_size = rb->mask + 1; > > > + > > > + /* Note that this formulation is valid in the face of overflow of > > > + * prod_pos so long as the delta between prod_pos and cons_pos is > > > + * no greater than max_size. > > > + */ > > > avail_size = max_size - (prod_pos - cons_pos); > > > > hi, > > the above hunk handles the case for 'prod_pos < cons_pos', > > > > but it looks like we assume 'cons_pos < prod_pos' in above calculation, > > should we check on that? > > > > jirka > > The code there does work (perhaps surprisingly) even if the cons_pos is > less than the prod_pos, so long as that delta is no greater than max_size. > I added the commentary there because I too found it to be unintuitive. > > Consider the following program. It will print "delta: 20". > > ```c > #include <stdio.h> > #include <stdlib.h> > #include <limits.h> > > int main() { > unsigned long cons_pos = ULONG_MAX - 9; > unsigned long prod_pos = 10; > printf("delta: %lu\n", prod_pos - cons_pos); > return 0; > } > ``` > > -Andrew > > > > > > > > /* Round up total size to a multiple of 8. */ > > > total_size = (size + BPF_RINGBUF_HDR_SZ + 7) / 8 * 8; > > > -- > > > 2.39.2 > > > > > > >
