On Thu, Aug 24, 2023 at 5:09 PM David Vernet <void@xxxxxxxxxxxxx> wrote: > > 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. I handled it in v3 which I just posted here: https://lore.kernel.org/bpf/20230824220907.1172808-1-awerner32@xxxxxxxxx/T/#u > > Thanks, > David
