On 7/30/20 11:33 AM, Pavel Begunkov wrote: > On 30/07/2020 20:18, Jens Axboe wrote: >> On 7/30/20 9:43 AM, Pavel Begunkov wrote: >>> All ->cq_overflow modifications should be under completion_lock, >>> otherwise it can report a wrong number to the userspace. Fix it in >>> io_uring_cancel_files(). >>> >>> Signed-off-by: Pavel Begunkov <asml.silence@xxxxxxxxx> >>> --- >>> fs/io_uring.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/fs/io_uring.c b/fs/io_uring.c >>> index 11f4ab87e08f..6e2322525da6 100644 >>> --- a/fs/io_uring.c >>> +++ b/fs/io_uring.c >>> @@ -7847,10 +7847,9 @@ static void io_uring_cancel_files(struct io_ring_ctx *ctx, >>> clear_bit(0, &ctx->cq_check_overflow); >>> ctx->rings->sq_flags &= ~IORING_SQ_CQ_OVERFLOW; >>> } >>> - spin_unlock_irq(&ctx->completion_lock); >>> - >>> WRITE_ONCE(ctx->rings->cq_overflow, >>> atomic_inc_return(&ctx->cached_cq_overflow)); >>> + spin_unlock_irq(&ctx->completion_lock); >> >> Torn writes? Not sure I see what the issue here, can you expand? > > No, just off-by-one(many). E.g. > > let: cached_overflow = 0; > > CPU 1 | CPU 2 > ==================================================================== > t = ++cached_overflow // t == 1 | > | t2 = ++cached_overflow // t2 == 2 > | WRITE_ONCE(cq_overflow, t2) > WRITE_ONCE(cq_overflow, t1) | > > > So, ctx->rings->cq_overflow == 1, but ctx->cached_cq_overflow == 2. > A minor problem and easy to fix. Ah yes, good point. -- Jens Axboe
