On Wed, Aug 23, 2023 at 05:07:42PM -0700, Andrii Nakryiko wrote: [...] > > > > > BTW, I think kernel code needs fixing as well in > > > > > __bpf_user_ringbuf_peek (we should compare consumer/producer positions > > > > > directly, only through subtraction and casting to signed long as > > > > > above), would you be able to fix it at the same time with libbpf? > > > > > Would be good to also double-check the rest of kernel/bpf/ringbuf.c to > > > > > make sure we don't directly compare positions anywhere else. > > > > > > > > I missed __bpf_user_ringbuf_peek() when reviewed the patch. I also can > > > > help to double-check kernel/bpf/ringbuf.c. > > > > > > great, thanks! Good catch, and thanks! > > > > I'll update the code in __bpf_user_ringbuf_peek. One observation is > > that the code there differs from the regular (non-user) ringbuf and > > the libbpf code in that it uses u64 for the positions whereas > > everywhere else (including in the struct definition) the types of > > these positions are unsigned long. I get that on 64-bit architectures, > > practically speaking, these are the same. What I'm less clear on is > > whether there's anything that prevents this code from running (perhaps > > with bugs) on 32-bit machines. I suppose that on little-endian > > machines things will just work out until the positions overflow 32 > > bits. > > > > Would it make sense for me to make the change to always use unsigned > > long for these values in the same change? > > I'm not sure, but I suspect that u64 usage is not intentional, so we > probably do want to use unsigned long consistently. Yeah, that was an oversight and should be fixed. Thanks for pointing it out Andrew. FYI, we'd also need to fix user_ring_buffer__reserve() in libbpf. Andrew -- I'm happy to send a patch that fixes this. If you'd like to send it out, please feel free to do so. Just let me know. Thanks, David
