On Fri, Aug 25, 2023 at 12:00 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> 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
