On Mon, May 12, 2025 at 7:27 PM Ming Lei <ming.lei@xxxxxxxxxx> wrote: > > On Mon, May 12, 2025 at 10:39:57AM -0700, Caleb Sander Mateos wrote: > > On Fri, May 9, 2025 at 8:06 AM Ming Lei <ming.lei@xxxxxxxxxx> wrote: > > > > > > `ublk_queue` is read only for io buffer register/unregister command. Both > > > `ublk_io` and block layer request are read-only for IO buffer register/ > > > unregister command. > > > > > > So the two command can be issued from other task contexts. > > > > I mentioned this before in > > https://lore.kernel.org/linux-block/CADUfDZqZ_9O7vUAYtxrrujWqPBuP05nBhCbzNuNsc9kJTmX2sA@xxxxxxxxxxxxxx/ > > > > But UBLK_IO_(UN)REGISTER_IO_BUF still reads io->flags. So it would be > > a race condition to handle it on a thread other than ubq_daemon, as > > ubq_daemon may concurrently modify io->flags. If you do want to > > support UBLK_IO_(UN)REGISTER_IO_BUF on other threads, the writes to > > io->flags should use WRITE_ONCE() and the reads on other threads > > should use READ_ONCE(). With those modifications, it should be safe > > because __ublk_check_and_get_req() atomically checks the state of the > > request and increments its reference count. > > UBLK_IO_(UN)REGISTER_IO_BUF just reads the flag, if > UBLK_IO_FLAG_OWNED_BY_SRV is cleared, the OP is failed. > > Otherwise, __ublk_check_and_get_req() covers everything because both > 'ublk_io' and 'request' are pre-allocation. The only race is that new > recycled request buffer is registered, that is fine, because it can be > treated as logic bug. > > So I think it isn't necessary to use READ_ONCE/WRITE_ONCE, or can you show > what the exact issue is? It's undefined behavior in C for a value to be concurrently accessed by multiple threads, at least one of which is writing to it. I agree the UB is unlikely to be exploited by the compiler or processor (at least on x86 or ARM), but it is a theoretical possibility. At the very least, KCSAN could detect a race condition here. But even without evidence of observable bugs, I think we should strive to avoid introducing UB, especially when it can be avoided fairly easily with READ_ONCE() and WRITE_ONCE(). Best, Caleb
