[PATCH bpf-next v3] libbpf: handle producer position overflow

[PATCH bpf-next v3] libbpf: handle producer position overflow

[Andrew Werner]

Before this patch, the producer position could overflow `unsigned
long`, in which case libbpf would forever stop processing new writes to
the ringbuf. Similarly, overflows of the producer position could result
in __bpf_user_ringbuf_peek not discovering available data. This patch
addresses that bug by computing using the signed delta between the
consumer and producer position to determine if data is available; the
delta computation is robust to overflow.

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.

Secondarily, the type used to represent the positions in the
user_ring_buffer functions in both libbpf and the kernel has been
changed from u64 to unsigned long to match the type used in the
kernel's representation of the structure. The change occurs in the
same patch because it's required to align the data availability
calculations between the userspace producing ringbuf and the bpf
producing ringbuf.

Not included in this patch, a selftest was written to demonstrate the
bug, and indeed this patch allows the test to continue to make progress
past the overflow. The shape of the self test was as follows:

 a) Set up ringbuf of 2GB size (the maximum permitted size).
 b) reserve+commit maximum-sized records (ULONG_MAX/4) constantly as
    fast as possible.

With 1 million records per second repro time should be about 4.7 hours.
Such a test duration is impractical to run, hence the omission.

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.

v2->v3:
  - Changed the representation of the consumer and producer positions
    from u64 to unsigned long in user_ring_buffer functions. 
  - Addressed overflow in __bpf_user_ringbuf_peek.
  - Changed data availability computations to use the signed delta
    between the consumer and producer positions rather than merely
    checking whether their values were unequal.
v1->v2:
  - Fixed comment grammar.
  - Properly formatted subject line.

Signed-off-by: Andrew Werner <awerner32@gmail.com>
---
 kernel/bpf/ringbuf.c    | 11 ++++++++---
 tools/lib/bpf/ringbuf.c | 16 +++++++++++++---
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
index f045fde632e5..0c48673520fb 100644
--- a/kernel/bpf/ringbuf.c
+++ b/kernel/bpf/ringbuf.c
@@ -658,7 +658,7 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
 {
 	int err;
 	u32 hdr_len, sample_len, total_len, flags, *hdr;
-	u64 cons_pos, prod_pos;
+	unsigned long cons_pos, prod_pos;
 
 	/* Synchronizes with smp_store_release() in user-space producer. */
 	prod_pos = smp_load_acquire(&rb->producer_pos);
@@ -667,7 +667,12 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
 
 	/* Synchronizes with smp_store_release() in __bpf_user_ringbuf_sample_release() */
 	cons_pos = smp_load_acquire(&rb->consumer_pos);
-	if (cons_pos >= prod_pos)
+
+	/* Check if there's data available by computing the signed delta between
+	 * cons_pos and prod_pos; a negative delta indicates that the consumer has
+	 * not caught up. This formulation is robust to prod_pos wrapping around.
+	 */
+	if ((long)(cons_pos - prod_pos) >= 0)
 		return -ENODATA;
 
 	hdr = (u32 *)((uintptr_t)rb->data + (uintptr_t)(cons_pos & rb->mask));
@@ -711,7 +716,7 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
 
 static void __bpf_user_ringbuf_sample_release(struct bpf_ringbuf *rb, size_t size, u64 flags)
 {
-	u64 consumer_pos;
+	unsigned long consumer_pos;
 	u32 rounded_size = round_up(size + BPF_RINGBUF_HDR_SZ, 8);
 
 	/* Using smp_load_acquire() is unnecessary here, as the busy-bit
diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c
index 02199364db13..141030a89370 100644
--- a/tools/lib/bpf/ringbuf.c
+++ b/tools/lib/bpf/ringbuf.c
@@ -237,7 +237,13 @@ 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) {
+
+		/* Check if there's data available by computing the signed delta
+		 * between cons_pos and prod_pos; a negative delta indicates that the
+		 * consumer has not caught up. This formulation is robust to prod_pos
+		 * wrapping around.
+		 */
+		while ((long)(cons_pos - prod_pos) < 0) {
 			len_ptr = r->data + (cons_pos & r->mask);
 			len = smp_load_acquire(len_ptr);
 
@@ -482,8 +488,7 @@ void user_ring_buffer__submit(struct user_ring_buffer *rb, void *sample)
 void *user_ring_buffer__reserve(struct user_ring_buffer *rb, __u32 size)
 {
 	__u32 avail_size, total_size, max_size;
-	/* 64-bit to avoid overflow in case of extreme application behavior */
-	__u64 cons_pos, prod_pos;
+	unsigned long cons_pos, prod_pos;
 	struct ringbuf_hdr *hdr;
 
 	/* The top two bits are used as special flags */
@@ -498,6 +503,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);
 	/* Round up total size to a multiple of 8. */
 	total_size = (size + BPF_RINGBUF_HDR_SZ + 7) / 8 * 8;
-- 
2.39.2

Re: [PATCH bpf-next v3] libbpf: handle producer position overflow

[David Vernet]

On Thu, Aug 24, 2023 at 06:09:08PM -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. Similarly, overflows of the producer position could result
> in __bpf_user_ringbuf_peek not discovering available data. This patch
> addresses that bug by computing using the signed delta between the
> consumer and producer position to determine if data is available; the
> delta computation is robust to overflow.
> 
> 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.
> 
> Secondarily, the type used to represent the positions in the
> user_ring_buffer functions in both libbpf and the kernel has been
> changed from u64 to unsigned long to match the type used in the
> kernel's representation of the structure. The change occurs in the
> same patch because it's required to align the data availability
> calculations between the userspace producing ringbuf and the bpf
> producing ringbuf.
> 
> Not included in this patch, a selftest was written to demonstrate the
> bug, and indeed this patch allows the test to continue to make progress
> past the overflow. The shape of the self test was as follows:
> 
>  a) Set up ringbuf of 2GB size (the maximum permitted size).
>  b) reserve+commit maximum-sized records (ULONG_MAX/4) constantly as
>     fast as possible.
> 
> With 1 million records per second repro time should be about 4.7 hours.
> Such a test duration is impractical to run, hence the omission.
> 
> 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.
> 
> v2->v3:
>   - Changed the representation of the consumer and producer positions
>     from u64 to unsigned long in user_ring_buffer functions. 
>   - Addressed overflow in __bpf_user_ringbuf_peek.
>   - Changed data availability computations to use the signed delta
>     between the consumer and producer positions rather than merely
>     checking whether their values were unequal.
> v1->v2:
>   - Fixed comment grammar.
>   - Properly formatted subject line.
> 
> Signed-off-by: Andrew Werner <awerner32@gmail.com>

Hey Andrew,

This LGTM, thanks for finding and fixing this. I left a comment below
about the cast >= vs. equality comparison, but I won't block on it given
that it's already been discussed on another thread. Here's my tag:

Reviewed-by: David Vernet<void@manifault.com>

> ---
>  kernel/bpf/ringbuf.c    | 11 ++++++++---
>  tools/lib/bpf/ringbuf.c | 16 +++++++++++++---
>  2 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> index f045fde632e5..0c48673520fb 100644
> --- a/kernel/bpf/ringbuf.c
> +++ b/kernel/bpf/ringbuf.c
> @@ -658,7 +658,7 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
>  {
>  	int err;
>  	u32 hdr_len, sample_len, total_len, flags, *hdr;
> -	u64 cons_pos, prod_pos;
> +	unsigned long cons_pos, prod_pos;
>  
>  	/* Synchronizes with smp_store_release() in user-space producer. */
>  	prod_pos = smp_load_acquire(&rb->producer_pos);
> @@ -667,7 +667,12 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
>  
>  	/* Synchronizes with smp_store_release() in __bpf_user_ringbuf_sample_release() */
>  	cons_pos = smp_load_acquire(&rb->consumer_pos);
> -	if (cons_pos >= prod_pos)
> +
> +	/* Check if there's data available by computing the signed delta between
> +	 * cons_pos and prod_pos; a negative delta indicates that the consumer has
> +	 * not caught up. This formulation is robust to prod_pos wrapping around.
> +	 */
> +	if ((long)(cons_pos - prod_pos) >= 0)

I see that Andrii suggested doing it this way in [0] so I won't insist
on changing it, but IMO this is much less readable and more confusing
than just doing an if (cond_pos == prod_pos) with a comment. The way
it's written this way, it makes it look like there could be a situation
where cond_pos could be ahead of prod_pos, whereas that would actually
just be a bug elsewhere that we'd be papering over. I guess this is more
defensive. In any case, I won't insit on it needing to change.

[0]: https://lore.kernel.org/all/CAEf4BzZQQ=fz+NqFHhJcqKoVAvh4=XbH7HWaHKjUg5OOzi-PTw@mail.gmail.com/

>  		return -ENODATA;
>  
>  	hdr = (u32 *)((uintptr_t)rb->data + (uintptr_t)(cons_pos & rb->mask));
> @@ -711,7 +716,7 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
>  
>  static void __bpf_user_ringbuf_sample_release(struct bpf_ringbuf *rb, size_t size, u64 flags)
>  {
> -	u64 consumer_pos;
> +	unsigned long consumer_pos;
>  	u32 rounded_size = round_up(size + BPF_RINGBUF_HDR_SZ, 8);
>  
>  	/* Using smp_load_acquire() is unnecessary here, as the busy-bit
> diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c
> index 02199364db13..141030a89370 100644
> --- a/tools/lib/bpf/ringbuf.c
> +++ b/tools/lib/bpf/ringbuf.c
> @@ -237,7 +237,13 @@ 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) {
> +
> +		/* Check if there's data available by computing the signed delta
> +		 * between cons_pos and prod_pos; a negative delta indicates that the
> +		 * consumer has not caught up. This formulation is robust to prod_pos
> +		 * wrapping around.
> +		 */
> +		while ((long)(cons_pos - prod_pos) < 0) {

Same here. Doing a != is much more clear, in my opinion. Not a blocker
though.

[...]

Thanks,
David

Re: [PATCH bpf-next v3] libbpf: handle producer position overflow

[Andrew Werner]

On Thu, Aug 24, 2023 at 6:46 PM David Vernet <void@manifault.com> wrote:
>
> On Thu, Aug 24, 2023 at 06:09:08PM -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. Similarly, overflows of the producer position could result
> > in __bpf_user_ringbuf_peek not discovering available data. This patch
> > addresses that bug by computing using the signed delta between the
> > consumer and producer position to determine if data is available; the
> > delta computation is robust to overflow.
> >
> > 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.
> >
> > Secondarily, the type used to represent the positions in the
> > user_ring_buffer functions in both libbpf and the kernel has been
> > changed from u64 to unsigned long to match the type used in the
> > kernel's representation of the structure. The change occurs in the
> > same patch because it's required to align the data availability
> > calculations between the userspace producing ringbuf and the bpf
> > producing ringbuf.
> >
> > Not included in this patch, a selftest was written to demonstrate the
> > bug, and indeed this patch allows the test to continue to make progress
> > past the overflow. The shape of the self test was as follows:
> >
> >  a) Set up ringbuf of 2GB size (the maximum permitted size).
> >  b) reserve+commit maximum-sized records (ULONG_MAX/4) constantly as
> >     fast as possible.
> >
> > With 1 million records per second repro time should be about 4.7 hours.
> > Such a test duration is impractical to run, hence the omission.
> >
> > 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.
> >
> > v2->v3:
> >   - Changed the representation of the consumer and producer positions
> >     from u64 to unsigned long in user_ring_buffer functions.
> >   - Addressed overflow in __bpf_user_ringbuf_peek.
> >   - Changed data availability computations to use the signed delta
> >     between the consumer and producer positions rather than merely
> >     checking whether their values were unequal.
> > v1->v2:
> >   - Fixed comment grammar.
> >   - Properly formatted subject line.
> >
> > Signed-off-by: Andrew Werner <awerner32@gmail.com>
>
> Hey Andrew,
>
> This LGTM, thanks for finding and fixing this. I left a comment below
> about the cast >= vs. equality comparison, but I won't block on it given
> that it's already been discussed on another thread. Here's my tag:
>
> Reviewed-by: David Vernet<void@manifault.com>
>
> > ---
> >  kernel/bpf/ringbuf.c    | 11 ++++++++---
> >  tools/lib/bpf/ringbuf.c | 16 +++++++++++++---
> >  2 files changed, 21 insertions(+), 6 deletions(-)
> >
> > diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> > index f045fde632e5..0c48673520fb 100644
> > --- a/kernel/bpf/ringbuf.c
> > +++ b/kernel/bpf/ringbuf.c
> > @@ -658,7 +658,7 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
> >  {
> >       int err;
> >       u32 hdr_len, sample_len, total_len, flags, *hdr;
> > -     u64 cons_pos, prod_pos;
> > +     unsigned long cons_pos, prod_pos;
> >
> >       /* Synchronizes with smp_store_release() in user-space producer. */
> >       prod_pos = smp_load_acquire(&rb->producer_pos);
> > @@ -667,7 +667,12 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
> >
> >       /* Synchronizes with smp_store_release() in __bpf_user_ringbuf_sample_release() */
> >       cons_pos = smp_load_acquire(&rb->consumer_pos);
> > -     if (cons_pos >= prod_pos)
> > +
> > +     /* Check if there's data available by computing the signed delta between
> > +      * cons_pos and prod_pos; a negative delta indicates that the consumer has
> > +      * not caught up. This formulation is robust to prod_pos wrapping around.
> > +      */
> > +     if ((long)(cons_pos - prod_pos) >= 0)
>
> I see that Andrii suggested doing it this way in [0] so I won't insist
> on changing it, but IMO this is much less readable and more confusing
> than just doing an if (cond_pos == prod_pos) with a comment. The way
> it's written this way, it makes it look like there could be a situation
> where cond_pos could be ahead of prod_pos, whereas that would actually
> just be a bug elsewhere that we'd be papering over. I guess this is more
> defensive. In any case, I won't insit on it needing to change.

I am also happy with the !=/== checks vs this delta checking. Any cast
in C makes me worry I'm doing something wrong. My understanding is
that the C spec doesn't state that `unsigned long` and `long` have to
be the same bit width even in the same program, but that's not
practically going to happen. Even the wraparound behavior and the
representation of negative numbers aren't defined in the spec as I
understand.

I'm going to wait for Andrii to opine before doing anything to avoid
excessive churn.

>
> [0]: https://lore.kernel.org/all/CAEf4BzZQQ=fz+NqFHhJcqKoVAvh4=XbH7HWaHKjUg5OOzi-PTw@mail.gmail.com/
>
> >               return -ENODATA;
> >
> >       hdr = (u32 *)((uintptr_t)rb->data + (uintptr_t)(cons_pos & rb->mask));
> > @@ -711,7 +716,7 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
> >
> >  static void __bpf_user_ringbuf_sample_release(struct bpf_ringbuf *rb, size_t size, u64 flags)
> >  {
> > -     u64 consumer_pos;
> > +     unsigned long consumer_pos;
> >       u32 rounded_size = round_up(size + BPF_RINGBUF_HDR_SZ, 8);
> >
> >       /* Using smp_load_acquire() is unnecessary here, as the busy-bit
> > diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c
> > index 02199364db13..141030a89370 100644
> > --- a/tools/lib/bpf/ringbuf.c
> > +++ b/tools/lib/bpf/ringbuf.c
> > @@ -237,7 +237,13 @@ 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) {
> > +
> > +             /* Check if there's data available by computing the signed delta
> > +              * between cons_pos and prod_pos; a negative delta indicates that the
> > +              * consumer has not caught up. This formulation is robust to prod_pos
> > +              * wrapping around.
> > +              */
> > +             while ((long)(cons_pos - prod_pos) < 0) {
>
> Same here. Doing a != is much more clear, in my opinion. Not a blocker
> though.
>
> [...]
>
> Thanks,
> David

Re: [PATCH bpf-next v3] libbpf: handle producer position overflow

[Andrii Nakryiko]

On Thu, Aug 24, 2023 at 4:42 PM Andrew Werner <awerner32@gmail.com> wrote:
>
> On Thu, Aug 24, 2023 at 6:46 PM David Vernet <void@manifault.com> wrote:
> >
> > On Thu, Aug 24, 2023 at 06:09:08PM -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. Similarly, overflows of the producer position could result
> > > in __bpf_user_ringbuf_peek not discovering available data. This patch
> > > addresses that bug by computing using the signed delta between the
> > > consumer and producer position to determine if data is available; the
> > > delta computation is robust to overflow.
> > >
> > > 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.
> > >
> > > Secondarily, the type used to represent the positions in the
> > > user_ring_buffer functions in both libbpf and the kernel has been
> > > changed from u64 to unsigned long to match the type used in the
> > > kernel's representation of the structure. The change occurs in the
> > > same patch because it's required to align the data availability
> > > calculations between the userspace producing ringbuf and the bpf
> > > producing ringbuf.
> > >
> > > Not included in this patch, a selftest was written to demonstrate the
> > > bug, and indeed this patch allows the test to continue to make progress
> > > past the overflow. The shape of the self test was as follows:
> > >
> > >  a) Set up ringbuf of 2GB size (the maximum permitted size).
> > >  b) reserve+commit maximum-sized records (ULONG_MAX/4) constantly as
> > >     fast as possible.
> > >
> > > With 1 million records per second repro time should be about 4.7 hours.
> > > Such a test duration is impractical to run, hence the omission.
> > >
> > > 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.
> > >
> > > v2->v3:
> > >   - Changed the representation of the consumer and producer positions
> > >     from u64 to unsigned long in user_ring_buffer functions.
> > >   - Addressed overflow in __bpf_user_ringbuf_peek.
> > >   - Changed data availability computations to use the signed delta
> > >     between the consumer and producer positions rather than merely
> > >     checking whether their values were unequal.
> > > v1->v2:
> > >   - Fixed comment grammar.
> > >   - Properly formatted subject line.
> > >
> > > Signed-off-by: Andrew Werner <awerner32@gmail.com>
> >
> > Hey Andrew,
> >
> > This LGTM, thanks for finding and fixing this. I left a comment below
> > about the cast >= vs. equality comparison, but I won't block on it given
> > that it's already been discussed on another thread. Here's my tag:
> >
> > Reviewed-by: David Vernet<void@manifault.com>
> >
> > > ---
> > >  kernel/bpf/ringbuf.c    | 11 ++++++++---
> > >  tools/lib/bpf/ringbuf.c | 16 +++++++++++++---
> > >  2 files changed, 21 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> > > index f045fde632e5..0c48673520fb 100644
> > > --- a/kernel/bpf/ringbuf.c
> > > +++ b/kernel/bpf/ringbuf.c
> > > @@ -658,7 +658,7 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
> > >  {
> > >       int err;
> > >       u32 hdr_len, sample_len, total_len, flags, *hdr;
> > > -     u64 cons_pos, prod_pos;
> > > +     unsigned long cons_pos, prod_pos;
> > >
> > >       /* Synchronizes with smp_store_release() in user-space producer. */
> > >       prod_pos = smp_load_acquire(&rb->producer_pos);
> > > @@ -667,7 +667,12 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
> > >
> > >       /* Synchronizes with smp_store_release() in __bpf_user_ringbuf_sample_release() */
> > >       cons_pos = smp_load_acquire(&rb->consumer_pos);
> > > -     if (cons_pos >= prod_pos)
> > > +
> > > +     /* Check if there's data available by computing the signed delta between
> > > +      * cons_pos and prod_pos; a negative delta indicates that the consumer has
> > > +      * not caught up. This formulation is robust to prod_pos wrapping around.
> > > +      */
> > > +     if ((long)(cons_pos - prod_pos) >= 0)
> >
> > I see that Andrii suggested doing it this way in [0] so I won't insist
> > on changing it, but IMO this is much less readable and more confusing
> > than just doing an if (cond_pos == prod_pos) with a comment. The way

this is a canonical way of comparing two counters that can wrap over,
just like time_before() for jiffies. We can add a small macro similar
to time_before(), but I figured given it's in one place in kernel's
ringbuf, it should be fine

as for just using inequality here, I still would prefer to preserve
original semantics (cons_pos >= prod_pos) instead of having to analyze
anew whether just doing inequality here and handling potentially
garbage data later on would be always correct.

For libbpf side, given we expect kernel to behave correctly always,
inequality would be a bit more acceptable, but I'd still keep
inequality just like in the original code (except taking into account
wraparound possibility)


> > it's written this way, it makes it look like there could be a situation
> > where cond_pos could be ahead of prod_pos, whereas that would actually
> > just be a bug elsewhere that we'd be papering over. I guess this is more
> > defensive. In any case, I won't insit on it needing to change.
>
> I am also happy with the !=/== checks vs this delta checking. Any cast
> in C makes me worry I'm doing something wrong. My understanding is
> that the C spec doesn't state that `unsigned long` and `long` have to
> be the same bit width even in the same program, but that's not
> practically going to happen. Even the wraparound behavior and the
> representation of negative numbers aren't defined in the spec as I
> understand.

I haven't studied C spec and don't really want to :) but if ever
signed vs unsigned changed the size of int/long/long long, so much
code would be broken, that I refuse to worry about this :) Similarly
such comparison-in-the-face-of-wrapping is done all the time in kernel
for jiffies comparison and such, so we are not charting a new
territory here.

>
> I'm going to wait for Andrii to opine before doing anything to avoid
> excessive churn.
>
> >
> > [0]: https://lore.kernel.org/all/CAEf4BzZQQ=fz+NqFHhJcqKoVAvh4=XbH7HWaHKjUg5OOzi-PTw@mail.gmail.com/
> >
> > >               return -ENODATA;
> > >
> > >       hdr = (u32 *)((uintptr_t)rb->data + (uintptr_t)(cons_pos & rb->mask));
> > > @@ -711,7 +716,7 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
> > >
> > >  static void __bpf_user_ringbuf_sample_release(struct bpf_ringbuf *rb, size_t size, u64 flags)
> > >  {
> > > -     u64 consumer_pos;
> > > +     unsigned long consumer_pos;
> > >       u32 rounded_size = round_up(size + BPF_RINGBUF_HDR_SZ, 8);
> > >
> > >       /* Using smp_load_acquire() is unnecessary here, as the busy-bit
> > > diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c
> > > index 02199364db13..141030a89370 100644
> > > --- a/tools/lib/bpf/ringbuf.c
> > > +++ b/tools/lib/bpf/ringbuf.c
> > > @@ -237,7 +237,13 @@ 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) {
> > > +
> > > +             /* Check if there's data available by computing the signed delta
> > > +              * between cons_pos and prod_pos; a negative delta indicates that the
> > > +              * consumer has not caught up. This formulation is robust to prod_pos
> > > +              * wrapping around.
> > > +              */
> > > +             while ((long)(cons_pos - prod_pos) < 0) {
> >
> > Same here. Doing a != is much more clear, in my opinion. Not a blocker
> > though.
> >
> > [...]
> >
> > Thanks,
> > David

Re: [PATCH bpf-next v3] libbpf: handle producer position overflow

[Hou Tao]

Hi,

On 8/25/2023 6:09 AM, 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. Similarly, overflows of the producer position could result
> in __bpf_user_ringbuf_peek not discovering available data. This patch
> addresses that bug by computing using the signed delta between the
> consumer and producer position to determine if data is available; the
> delta computation is robust to overflow.
>
> 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.
>
> Secondarily, the type used to represent the positions in the
> user_ring_buffer functions in both libbpf and the kernel has been
> changed from u64 to unsigned long to match the type used in the
> kernel's representation of the structure. The change occurs in the
> same patch because it's required to align the data availability
> calculations between the userspace producing ringbuf and the bpf
> producing ringbuf.

Because the changes include both the change for ring buffer and user
ring buffer. I think it is better to split the changes into three
patches to ease the backports of these changes: one patch for change in
libbpf for ring buffer, and another two patches for changes in libbpf
and kernel for user ring buffer.
>
> Not included in this patch, a selftest was written to demonstrate the
> bug, and indeed this patch allows the test to continue to make progress
> past the overflow. The shape of the self test was as follows:
>
>  a) Set up ringbuf of 2GB size (the maximum permitted size).
>  b) reserve+commit maximum-sized records (ULONG_MAX/4) constantly as
>     fast as possible.

ULONG_MAX -> UINT_MAX ?
>
> With 1 million records per second repro time should be about 4.7 hours.
> Such a test duration is impractical to run, hence the omission.
>
> 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.
>
> v2->v3:
>   - Changed the representation of the consumer and producer positions
>     from u64 to unsigned long in user_ring_buffer functions. 
>   - Addressed overflow in __bpf_user_ringbuf_peek.
>   - Changed data availability computations to use the signed delta
>     between the consumer and producer positions rather than merely
>     checking whether their values were unequal.
> v1->v2:
>   - Fixed comment grammar.
>   - Properly formatted subject line.
>
> Signed-off-by: Andrew Werner <awerner32@gmail.com>
> ---
>  kernel/bpf/ringbuf.c    | 11 ++++++++---
>  tools/lib/bpf/ringbuf.c | 16 +++++++++++++---
>  2 files changed, 21 insertions(+), 6 deletions(-)

Otherwise, these changes look good to me:

Acked-by: Hou Tao <houtao1@huawei.com>

>
> diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> index f045fde632e5..0c48673520fb 100644
> --- a/kernel/bpf/ringbuf.c
> +++ b/kernel/bpf/ringbuf.c
> @@ -658,7 +658,7 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
>  {
>  	int err;
>  	u32 hdr_len, sample_len, total_len, flags, *hdr;
> -	u64 cons_pos, prod_pos;
> +	unsigned long cons_pos, prod_pos;
>  
>  	/* Synchronizes with smp_store_release() in user-space producer. */
>  	prod_pos = smp_load_acquire(&rb->producer_pos);
> @@ -667,7 +667,12 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
>  
>  	/* Synchronizes with smp_store_release() in __bpf_user_ringbuf_sample_release() */
>  	cons_pos = smp_load_acquire(&rb->consumer_pos);
> -	if (cons_pos >= prod_pos)
> +
> +	/* Check if there's data available by computing the signed delta between
> +	 * cons_pos and prod_pos; a negative delta indicates that the consumer has
> +	 * not caught up. This formulation is robust to prod_pos wrapping around.
> +	 */
> +	if ((long)(cons_pos - prod_pos) >= 0)
>  		return -ENODATA;
>  
>  	hdr = (u32 *)((uintptr_t)rb->data + (uintptr_t)(cons_pos & rb->mask));
> @@ -711,7 +716,7 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
>  
>  static void __bpf_user_ringbuf_sample_release(struct bpf_ringbuf *rb, size_t size, u64 flags)
>  {
> -	u64 consumer_pos;
> +	unsigned long consumer_pos;
>  	u32 rounded_size = round_up(size + BPF_RINGBUF_HDR_SZ, 8);
>  
>  	/* Using smp_load_acquire() is unnecessary here, as the busy-bit
> diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c
> index 02199364db13..141030a89370 100644
> --- a/tools/lib/bpf/ringbuf.c
> +++ b/tools/lib/bpf/ringbuf.c
> @@ -237,7 +237,13 @@ 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) {
> +
> +		/* Check if there's data available by computing the signed delta
> +		 * between cons_pos and prod_pos; a negative delta indicates that the
> +		 * consumer has not caught up. This formulation is robust to prod_pos
> +		 * wrapping around.
> +		 */
> +		while ((long)(cons_pos - prod_pos) < 0) {
>  			len_ptr = r->data + (cons_pos & r->mask);
>  			len = smp_load_acquire(len_ptr);
>  
> @@ -482,8 +488,7 @@ void user_ring_buffer__submit(struct user_ring_buffer *rb, void *sample)
>  void *user_ring_buffer__reserve(struct user_ring_buffer *rb, __u32 size)
>  {
>  	__u32 avail_size, total_size, max_size;
> -	/* 64-bit to avoid overflow in case of extreme application behavior */
> -	__u64 cons_pos, prod_pos;
> +	unsigned long cons_pos, prod_pos;
>  	struct ringbuf_hdr *hdr;
>  
>  	/* The top two bits are used as special flags */
> @@ -498,6 +503,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);
>  	/* Round up total size to a multiple of 8. */
>  	total_size = (size + BPF_RINGBUF_HDR_SZ + 7) / 8 * 8;

Re: [PATCH bpf-next v3] libbpf: handle producer position overflow

[Daniel Borkmann]

On 8/25/23 5:17 AM, Hou Tao wrote:
> On 8/25/2023 6:09 AM, 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. Similarly, overflows of the producer position could result
>> in __bpf_user_ringbuf_peek not discovering available data. This patch
>> addresses that bug by computing using the signed delta between the
>> consumer and producer position to determine if data is available; the
>> delta computation is robust to overflow.
>>
>> 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.
>>
>> Secondarily, the type used to represent the positions in the
>> user_ring_buffer functions in both libbpf and the kernel has been
>> changed from u64 to unsigned long to match the type used in the
>> kernel's representation of the structure. The change occurs in the
>> same patch because it's required to align the data availability
>> calculations between the userspace producing ringbuf and the bpf
>> producing ringbuf.
> 
> Because the changes include both the change for ring buffer and user
> ring buffer. I think it is better to split the changes into three
> patches to ease the backports of these changes: one patch for change in
> libbpf for ring buffer, and another two patches for changes in libbpf
> and kernel for user ring buffer.

Splitting off the kernel parts into a separate patch would indeed be
good so that stable team can pull it. (Pls also add a Fixes tag.)

Thanks,
Daniel

Re: [PATCH bpf-next v3] libbpf: handle producer position overflow

[Andrew Werner]

On Fri, Aug 25, 2023 at 12:00 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 8/25/23 5:17 AM, Hou Tao wrote:
> > On 8/25/2023 6:09 AM, 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. Similarly, overflows of the producer position could result
> >> in __bpf_user_ringbuf_peek not discovering available data. This patch
> >> addresses that bug by computing using the signed delta between the
> >> consumer and producer position to determine if data is available; the
> >> delta computation is robust to overflow.
> >>
> >> 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.
> >>
> >> Secondarily, the type used to represent the positions in the
> >> user_ring_buffer functions in both libbpf and the kernel has been
> >> changed from u64 to unsigned long to match the type used in the
> >> kernel's representation of the structure. The change occurs in the
> >> same patch because it's required to align the data availability
> >> calculations between the userspace producing ringbuf and the bpf
> >> producing ringbuf.
> >
> > Because the changes include both the change for ring buffer and user
> > ring buffer. I think it is better to split the changes into three
> > patches to ease the backports of these changes: one patch for change in
> > libbpf for ring buffer, and another two patches for changes in libbpf
> > and kernel for user ring buffer.
>
> Splitting off the kernel parts into a separate patch would indeed be
> good so that stable team can pull it. (Pls also add a Fixes tag.)

Ack, will do this for the next revision when the dust settles.

>
> Thanks,
> Daniel

Re: [PATCH bpf-next v3] libbpf: handle producer position overflow

[Daniel Borkmann]

On 8/25/23 12:09 AM, 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. Similarly, overflows of the producer position could result
> in __bpf_user_ringbuf_peek not discovering available data. This patch
> addresses that bug by computing using the signed delta between the
> consumer and producer position to determine if data is available; the
> delta computation is robust to overflow.
> 
> 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.
> 
> Secondarily, the type used to represent the positions in the
> user_ring_buffer functions in both libbpf and the kernel has been
> changed from u64 to unsigned long to match the type used in the
> kernel's representation of the structure. The change occurs in the

Hm, but won't this mismatch for 64bit kernel and 32bit user space? Why
not fixate both on u64 instead so types are consistent?

> same patch because it's required to align the data availability
> calculations between the userspace producing ringbuf and the bpf
> producing ringbuf.
> 
> Not included in this patch, a selftest was written to demonstrate the
> bug, and indeed this patch allows the test to continue to make progress
> past the overflow. The shape of the self test was as follows:
> 
>   a) Set up ringbuf of 2GB size (the maximum permitted size).
>   b) reserve+commit maximum-sized records (ULONG_MAX/4) constantly as
>      fast as possible.
> 
> With 1 million records per second repro time should be about 4.7 hours.
> Such a test duration is impractical to run, hence the omission.
> 
> 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.
> 
> v2->v3:
>    - Changed the representation of the consumer and producer positions
>      from u64 to unsigned long in user_ring_buffer functions.
>    - Addressed overflow in __bpf_user_ringbuf_peek.
>    - Changed data availability computations to use the signed delta
>      between the consumer and producer positions rather than merely
>      checking whether their values were unequal.
> v1->v2:
>    - Fixed comment grammar.
>    - Properly formatted subject line.
> 
> Signed-off-by: Andrew Werner <awerner32@gmail.com>
> ---
>   kernel/bpf/ringbuf.c    | 11 ++++++++---
>   tools/lib/bpf/ringbuf.c | 16 +++++++++++++---
>   2 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> index f045fde632e5..0c48673520fb 100644
> --- a/kernel/bpf/ringbuf.c
> +++ b/kernel/bpf/ringbuf.c
> @@ -658,7 +658,7 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
>   {
>   	int err;
>   	u32 hdr_len, sample_len, total_len, flags, *hdr;
> -	u64 cons_pos, prod_pos;
> +	unsigned long cons_pos, prod_pos;
>   
>   	/* Synchronizes with smp_store_release() in user-space producer. */
>   	prod_pos = smp_load_acquire(&rb->producer_pos);
> @@ -667,7 +667,12 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
>   
>   	/* Synchronizes with smp_store_release() in __bpf_user_ringbuf_sample_release() */
>   	cons_pos = smp_load_acquire(&rb->consumer_pos);
> -	if (cons_pos >= prod_pos)
> +
> +	/* Check if there's data available by computing the signed delta between
> +	 * cons_pos and prod_pos; a negative delta indicates that the consumer has
> +	 * not caught up. This formulation is robust to prod_pos wrapping around.
> +	 */
> +	if ((long)(cons_pos - prod_pos) >= 0)
>   		return -ENODATA;
>   
>   	hdr = (u32 *)((uintptr_t)rb->data + (uintptr_t)(cons_pos & rb->mask));
> @@ -711,7 +716,7 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
>   
>   static void __bpf_user_ringbuf_sample_release(struct bpf_ringbuf *rb, size_t size, u64 flags)
>   {
> -	u64 consumer_pos;
> +	unsigned long consumer_pos;
>   	u32 rounded_size = round_up(size + BPF_RINGBUF_HDR_SZ, 8);
>   
>   	/* Using smp_load_acquire() is unnecessary here, as the busy-bit
> diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c
> index 02199364db13..141030a89370 100644
> --- a/tools/lib/bpf/ringbuf.c
> +++ b/tools/lib/bpf/ringbuf.c
> @@ -237,7 +237,13 @@ 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) {
> +
> +		/* Check if there's data available by computing the signed delta
> +		 * between cons_pos and prod_pos; a negative delta indicates that the
> +		 * consumer has not caught up. This formulation is robust to prod_pos
> +		 * wrapping around.
> +		 */
> +		while ((long)(cons_pos - prod_pos) < 0) {
>   			len_ptr = r->data + (cons_pos & r->mask);
>   			len = smp_load_acquire(len_ptr);
>   
> @@ -482,8 +488,7 @@ void user_ring_buffer__submit(struct user_ring_buffer *rb, void *sample)
>   void *user_ring_buffer__reserve(struct user_ring_buffer *rb, __u32 size)
>   {
>   	__u32 avail_size, total_size, max_size;
> -	/* 64-bit to avoid overflow in case of extreme application behavior */
> -	__u64 cons_pos, prod_pos;
> +	unsigned long cons_pos, prod_pos;
>   	struct ringbuf_hdr *hdr;
>   
>   	/* The top two bits are used as special flags */
> @@ -498,6 +503,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);
>   	/* Round up total size to a multiple of 8. */
>   	total_size = (size + BPF_RINGBUF_HDR_SZ + 7) / 8 * 8;
> 

Re: [PATCH bpf-next v3] libbpf: handle producer position overflow

[Andrew Werner]

On Fri, Aug 25, 2023 at 11:28 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 8/25/23 12:09 AM, 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. Similarly, overflows of the producer position could result
> > in __bpf_user_ringbuf_peek not discovering available data. This patch
> > addresses that bug by computing using the signed delta between the
> > consumer and producer position to determine if data is available; the
> > delta computation is robust to overflow.
> >
> > 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.
> >
> > Secondarily, the type used to represent the positions in the
> > user_ring_buffer functions in both libbpf and the kernel has been
> > changed from u64 to unsigned long to match the type used in the
> > kernel's representation of the structure. The change occurs in the
>
> Hm, but won't this mismatch for 64bit kernel and 32bit user space? Why
> not fixate both on u64 instead so types are consistent?

Sure. It feels like if we do that then we'd break existing 32bit
big-endian clients, though I am not sure those exist. Concretely, the
request here would be to change the kernel structure and all library
usages to use u64, right?

>
> > same patch because it's required to align the data availability
> > calculations between the userspace producing ringbuf and the bpf
> > producing ringbuf.
> >
> > Not included in this patch, a selftest was written to demonstrate the
> > bug, and indeed this patch allows the test to continue to make progress
> > past the overflow. The shape of the self test was as follows:
> >
> >   a) Set up ringbuf of 2GB size (the maximum permitted size).
> >   b) reserve+commit maximum-sized records (ULONG_MAX/4) constantly as
> >      fast as possible.
> >
> > With 1 million records per second repro time should be about 4.7 hours.
> > Such a test duration is impractical to run, hence the omission.
> >
> > 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.
> >
> > v2->v3:
> >    - Changed the representation of the consumer and producer positions
> >      from u64 to unsigned long in user_ring_buffer functions.
> >    - Addressed overflow in __bpf_user_ringbuf_peek.
> >    - Changed data availability computations to use the signed delta
> >      between the consumer and producer positions rather than merely
> >      checking whether their values were unequal.
> > v1->v2:
> >    - Fixed comment grammar.
> >    - Properly formatted subject line.
> >
> > Signed-off-by: Andrew Werner <awerner32@gmail.com>
> > ---
> >   kernel/bpf/ringbuf.c    | 11 ++++++++---
> >   tools/lib/bpf/ringbuf.c | 16 +++++++++++++---
> >   2 files changed, 21 insertions(+), 6 deletions(-)
> >
> > diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> > index f045fde632e5..0c48673520fb 100644
> > --- a/kernel/bpf/ringbuf.c
> > +++ b/kernel/bpf/ringbuf.c
> > @@ -658,7 +658,7 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
> >   {
> >       int err;
> >       u32 hdr_len, sample_len, total_len, flags, *hdr;
> > -     u64 cons_pos, prod_pos;
> > +     unsigned long cons_pos, prod_pos;
> >
> >       /* Synchronizes with smp_store_release() in user-space producer. */
> >       prod_pos = smp_load_acquire(&rb->producer_pos);
> > @@ -667,7 +667,12 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
> >
> >       /* Synchronizes with smp_store_release() in __bpf_user_ringbuf_sample_release() */
> >       cons_pos = smp_load_acquire(&rb->consumer_pos);
> > -     if (cons_pos >= prod_pos)
> > +
> > +     /* Check if there's data available by computing the signed delta between
> > +      * cons_pos and prod_pos; a negative delta indicates that the consumer has
> > +      * not caught up. This formulation is robust to prod_pos wrapping around.
> > +      */
> > +     if ((long)(cons_pos - prod_pos) >= 0)
> >               return -ENODATA;
> >
> >       hdr = (u32 *)((uintptr_t)rb->data + (uintptr_t)(cons_pos & rb->mask));
> > @@ -711,7 +716,7 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
> >
> >   static void __bpf_user_ringbuf_sample_release(struct bpf_ringbuf *rb, size_t size, u64 flags)
> >   {
> > -     u64 consumer_pos;
> > +     unsigned long consumer_pos;
> >       u32 rounded_size = round_up(size + BPF_RINGBUF_HDR_SZ, 8);
> >
> >       /* Using smp_load_acquire() is unnecessary here, as the busy-bit
> > diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c
> > index 02199364db13..141030a89370 100644
> > --- a/tools/lib/bpf/ringbuf.c
> > +++ b/tools/lib/bpf/ringbuf.c
> > @@ -237,7 +237,13 @@ 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) {
> > +
> > +             /* Check if there's data available by computing the signed delta
> > +              * between cons_pos and prod_pos; a negative delta indicates that the
> > +              * consumer has not caught up. This formulation is robust to prod_pos
> > +              * wrapping around.
> > +              */
> > +             while ((long)(cons_pos - prod_pos) < 0) {
> >                       len_ptr = r->data + (cons_pos & r->mask);
> >                       len = smp_load_acquire(len_ptr);
> >
> > @@ -482,8 +488,7 @@ void user_ring_buffer__submit(struct user_ring_buffer *rb, void *sample)
> >   void *user_ring_buffer__reserve(struct user_ring_buffer *rb, __u32 size)
> >   {
> >       __u32 avail_size, total_size, max_size;
> > -     /* 64-bit to avoid overflow in case of extreme application behavior */
> > -     __u64 cons_pos, prod_pos;
> > +     unsigned long cons_pos, prod_pos;
> >       struct ringbuf_hdr *hdr;
> >
> >       /* The top two bits are used as special flags */
> > @@ -498,6 +503,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);
> >       /* Round up total size to a multiple of 8. */
> >       total_size = (size + BPF_RINGBUF_HDR_SZ + 7) / 8 * 8;
> >
>

Re: [PATCH bpf-next v3] libbpf: handle producer position overflow

[Daniel Borkmann]

On 8/25/23 6:39 PM, Andrew Werner wrote:
> On Fri, Aug 25, 2023 at 11:28 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 8/25/23 12:09 AM, 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. Similarly, overflows of the producer position could result
>>> in __bpf_user_ringbuf_peek not discovering available data. This patch
>>> addresses that bug by computing using the signed delta between the
>>> consumer and producer position to determine if data is available; the
>>> delta computation is robust to overflow.
>>>
>>> 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.
>>>
>>> Secondarily, the type used to represent the positions in the
>>> user_ring_buffer functions in both libbpf and the kernel has been
>>> changed from u64 to unsigned long to match the type used in the
>>> kernel's representation of the structure. The change occurs in the
>>
>> Hm, but won't this mismatch for 64bit kernel and 32bit user space? Why
>> not fixate both on u64 instead so types are consistent?
> 
> Sure. It feels like if we do that then we'd break existing 32bit
> big-endian clients, though I am not sure those exist. Concretely, the
> request here would be to change the kernel structure and all library
> usages to use u64, right?

Yes, to align all consistently on u64. From your diff, I read that for
the kernel its the case already.

Thanks,
Daniel

Re: [PATCH bpf-next v3] libbpf: handle producer position overflow

[Andrew Werner]

On Fri, Aug 25, 2023 at 1:23 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 8/25/23 6:39 PM, Andrew Werner wrote:
> > On Fri, Aug 25, 2023 at 11:28 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >> On 8/25/23 12:09 AM, 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. Similarly, overflows of the producer position could result
> >>> in __bpf_user_ringbuf_peek not discovering available data. This patch
> >>> addresses that bug by computing using the signed delta between the
> >>> consumer and producer position to determine if data is available; the
> >>> delta computation is robust to overflow.
> >>>
> >>> 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.
> >>>
> >>> Secondarily, the type used to represent the positions in the
> >>> user_ring_buffer functions in both libbpf and the kernel has been
> >>> changed from u64 to unsigned long to match the type used in the
> >>> kernel's representation of the structure. The change occurs in the
> >>
> >> Hm, but won't this mismatch for 64bit kernel and 32bit user space? Why
> >> not fixate both on u64 instead so types are consistent?
> >
> > Sure. It feels like if we do that then we'd break existing 32bit
> > big-endian clients, though I am not sure those exist. Concretely, the
> > request here would be to change the kernel structure and all library
> > usages to use u64, right?
>
> Yes, to align all consistently on u64. From your diff, I read that for
> the kernel its the case already.

I would not say that for the kernel it's the case that these values
are u64. Today the kernel representation of the positions are both
`unsigned long`, and almost all functions which read or write to those
locations in memory use `unsigned long`. Two kernel functions related
to the user_ring_buffer and one libbpf function related to the
user_ring_buffer were reading the data at those addresses in the data
structure into u64.

>
> Thanks,
> Daniel

Re: [PATCH bpf-next v3] libbpf: handle producer position overflow

[Andrii Nakryiko]

On Fri, Aug 25, 2023 at 10:23 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 8/25/23 6:39 PM, Andrew Werner wrote:
> > On Fri, Aug 25, 2023 at 11:28 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >> On 8/25/23 12:09 AM, 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. Similarly, overflows of the producer position could result
> >>> in __bpf_user_ringbuf_peek not discovering available data. This patch
> >>> addresses that bug by computing using the signed delta between the
> >>> consumer and producer position to determine if data is available; the
> >>> delta computation is robust to overflow.
> >>>
> >>> 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.
> >>>
> >>> Secondarily, the type used to represent the positions in the
> >>> user_ring_buffer functions in both libbpf and the kernel has been
> >>> changed from u64 to unsigned long to match the type used in the
> >>> kernel's representation of the structure. The change occurs in the
> >>
> >> Hm, but won't this mismatch for 64bit kernel and 32bit user space? Why
> >> not fixate both on u64 instead so types are consistent?
> >
> > Sure. It feels like if we do that then we'd break existing 32bit
> > big-endian clients, though I am not sure those exist. Concretely, the
> > request here would be to change the kernel structure and all library
> > usages to use u64, right?
>
> Yes, to align all consistently on u64. From your diff, I read that for
> the kernel its the case already.
>

I don't think we can change it. It was intentionally specified as
`long` for consumer and producer positions, to match native word size
for atomic operations and smp_load_acquire/smp_store_release. Using
u64 was a mistake that slipped through.

Further, using u64 doesn't really help with anything, just makes
32-bit code slower (and maybe/probably would allow teared reads/writes
for position counters). Switching to unsigned long is the right move.

> Thanks,
> Daniel
>