On Fri, Aug 25, 2023 at 1:23 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote: > > On 8/25/23 6:39 PM, Andrew Werner wrote: > > On Fri, Aug 25, 2023 at 11:28 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> 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
