[PATCH 0/3] io_uring: ensure that entries don't contain old data

[PATCH 0/3] io_uring: ensure that entries don't contain old data

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Hello,

I noticed a bug when testing cmdprio_percentage with io_uring.
Turns out that this was because sqe->ioprio contained old data.

Fixed one additional field that could contain old data (sqe->flags),
as well as one field that was always cleared _after_ the flags for
this field had been set (sqe->rw_flags).

There might be additional fields that might contain old data, e.g.
sqe->sync_range_flags and sqe->fsync_flags. However, I assume that
io_uring ignores those flags for read and write commands, however,
they might still be able to interfere with non-read/write commands.


Kind regards,
Niklas

Niklas Cassel (3):
  io_uring: always initialize sqe->flags
  io_uring: don't clear recently set sqe->rw_flags
  io_uring: fix misbehaving cmdprio_percentage option

 engines/io_uring.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

-- 
2.31.1

[PATCH 1/3] io_uring: always initialize sqe->flags

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Commit 7c70f506e438 ("engines/io_uring: move sqe clear out of hot path")
removed the memset of sqe from fio_ioring_prep().

Later, force_async was added in commit 5a59a81d2923 ("engines/io_uring:
allow setting of IOSQE_ASYNC").

The force_async commit sets sqe->flags every N requests, however,
since we no longer do a memset, this commit should have made sure that
flags is always initialized, such that we don't have sqe->flags set on
reused sqes where we didn't intend to.

Fixes: 5a59a81d2923 ("engines/io_uring: allow setting of IOSQE_ASYNC")
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 engines/io_uring.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/engines/io_uring.c b/engines/io_uring.c
index 9c091e37..269e501f 100644
--- a/engines/io_uring.c
+++ b/engines/io_uring.c
@@ -234,6 +234,7 @@ static int fio_ioring_prep(struct thread_data *td, struct io_u *io_u)
 		sqe->flags = IOSQE_FIXED_FILE;
 	} else {
 		sqe->fd = f->fd;
+		sqe->flags = 0;
 	}
 
 	if (io_u->ddir == DDIR_READ || io_u->ddir == DDIR_WRITE) {
-- 
2.31.1

[PATCH 3/3] io_uring: fix misbehaving cmdprio_percentage option

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Commit 7c70f506e438 ("engines/io_uring: move sqe clear out of hot path")
removed the memset of sqe from fio_ioring_prep().

Before this commit, fio_ioring_prio_prep() behaved properly, because
sqe->ioprio was always cleared by the memset in fio_ioring_prep().

cmdprio_percentage=20 is supposed to set the highest priority class for
20% of the total I/Os, however, because sqes got reused without clearing
the ioprio field, this meant that the number of I/Os sent with the highest
priority became 95% already after 10 seconds. Quite far off from the
intended 20%.

Fix this by explicitly clearing the priority in fio_ioring_prio_prep().
Note that prio/prioclass cannot be used together with cmdprio_percentage,
so we do not need to do an additional clear in fio_ioring_prep().

engines/libaio.c doesn't explicitly clear the ioprio, nor does it memset
the descriptor entry, this is because io_prep_pread()/io_prep_pwrite() in
libaio itself performs a memset.

Fixes: 7c70f506e438 ("engines/io_uring: move sqe clear out of hot path")
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 engines/io_uring.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/engines/io_uring.c b/engines/io_uring.c
index 2b65572d..b8d4cf91 100644
--- a/engines/io_uring.c
+++ b/engines/io_uring.c
@@ -384,6 +384,8 @@ static void fio_ioring_prio_prep(struct thread_data *td, struct io_u *io_u)
 	if (rand_between(&td->prio_state, 0, 99) < o->cmdprio_percentage) {
 		ld->sqes[io_u->index].ioprio = IOPRIO_CLASS_RT << IOPRIO_CLASS_SHIFT;
 		io_u->flags |= IO_U_F_PRIORITY;
+	} else {
+		ld->sqes[io_u->index].ioprio = 0;
 	}
 	return;
 }
-- 
2.31.1

[PATCH 2/3] io_uring: don't clear recently set sqe->rw_flags

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Commit 7c70f506e438 ("engines/io_uring: move sqe clear out of hot path")
removed the memset of sqe from fio_ioring_prep().

This commit did add a clear of the sqe->rw_flags, however, it did so
after both RWF_UNCACHED and RWF_NOWAIT flags might have been set,
effectively clearing these flags if they got set.

This doesn't make any sense. Make sure that we clear sqe->rw_flags
before, not after, setting the flags.

Fixes: 7c70f506e438 ("engines/io_uring: move sqe clear out of hot path")
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 engines/io_uring.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/engines/io_uring.c b/engines/io_uring.c
index 269e501f..2b65572d 100644
--- a/engines/io_uring.c
+++ b/engines/io_uring.c
@@ -262,8 +262,9 @@ static int fio_ioring_prep(struct thread_data *td, struct io_u *io_u)
 				sqe->len = 1;
 			}
 		}
+		sqe->rw_flags = 0;
 		if (!td->o.odirect && o->uncached)
-			sqe->rw_flags = RWF_UNCACHED;
+			sqe->rw_flags |= RWF_UNCACHED;
 		if (o->nowait)
 			sqe->rw_flags |= RWF_NOWAIT;
 		if (ld->ioprio_class_set)
@@ -271,7 +272,6 @@ static int fio_ioring_prep(struct thread_data *td, struct io_u *io_u)
 		if (ld->ioprio_set)
 			sqe->ioprio |= td->o.ioprio;
 		sqe->off = io_u->offset;
-		sqe->rw_flags = 0;
 	} else if (ddir_sync(io_u->ddir)) {
 		sqe->ioprio = 0;
 		if (io_u->ddir == DDIR_SYNC_FILE_RANGE) {
-- 
2.31.1

Re: [PATCH 0/3] io_uring: ensure that entries don't contain old data

[Jens Axboe]

On 8/26/21 10:45 AM, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Hello,
> 
> I noticed a bug when testing cmdprio_percentage with io_uring.
> Turns out that this was because sqe->ioprio contained old data.
> 
> Fixed one additional field that could contain old data (sqe->flags),
> as well as one field that was always cleared _after_ the flags for
> this field had been set (sqe->rw_flags).
> 
> There might be additional fields that might contain old data, e.g.
> sqe->sync_range_flags and sqe->fsync_flags. However, I assume that
> io_uring ignores those flags for read and write commands, however,
> they might still be able to interfere with non-read/write commands.

Thanks, looks good, applied.

-- 
Jens Axboe