On Fri, Aug 25, 2023 at 10:23 AM 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 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 >
