[PATCH 1/2] src/queue: don't swallow -ETIME when SQEs were submitted

[PATCH 1/2] src/queue: don't swallow -ETIME when SQEs were submitted

[Prateek]

If _io_uring_get_cqe() submits SQEs and then times out waiting for
completions, it returns the submit count instead of -ETIME:

  1. The first enter submits the SQEs; because submit > 0 the kernel
     returns the submit count, not -ETIME, and it is stored in err.
  2. On the next iteration the has_ts shortcut wants to report -ETIME,
     but the 'if (!err)' guard sees the non-zero submit count and keeps
     it, so -ETIME is dropped.

That contradicts io_uring_submit_and_wait_timeout(3) and
io_uring_wait_cqes(3), which document -ETIME on timeout.

At these two sites (lines 113 and 118) err is only ever 0 or a positive
submit count. A negative error from __io_uring_peek_cqe() or a prior
enter breaks out of the loop before reaching here. So the change is
functionally equivalent to dropping the err condition entirely; we change
'!err' to 'err >= 0' so -ETIME is successfully synthesized whenever no
CQE was seen.

The guards were added in 2f61e849 ("src/queue: don't wait twice if
looping in _io_uring_get_cqe()") to carry the submit count across
iterations for the partial-completion case (got some CQEs, no error);
that case still returns the count because both sites remain guarded by
!cqe.

Signed-off-by: Prateek <kprateek283@gmail.com>
---
 src/queue.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/queue.c b/src/queue.c
index fcd3c702..e2e5a061 100644
--- a/src/queue.c
+++ b/src/queue.c
@@ -110,12 +110,12 @@ static int _io_uring_get_cqe(struct io_uring *ring,
 			 * timeout, so treat any timeout the same as -ETIME here.
 			 */
 			if (data->get_flags & IORING_ENTER_EXT_ARG_REG) {
-				if (!cqe && !err)
+				if (!cqe && err >= 0)
 					err = -ETIME;
 			} else {
 				struct io_uring_getevents_arg *arg = data->arg;
 
-				if (!cqe && arg->ts && !err)
+				if (!cqe && arg->ts && err >= 0)
 					err = -ETIME;
 			}
 			break;
-- 
2.43.0

[PATCH 2/2] test/timeout-swallow: verify -ETIME is not swallowed

[Prateek]

Regression test for the previous commit. Submits an SQE and waits with a
zero timeout for more completions than can arrive; the result must be
-ETIME, not the positive submit count. Covers the normal EXT_ARG wait
path and the registered-wait path, each skipped gracefully where
unsupported.

Signed-off-by: Prateek <kprateek283@gmail.com>
---
 test/Makefile          |   1 +
 test/timeout-swallow.c | 117 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 118 insertions(+)
 create mode 100644 test/timeout-swallow.c

diff --git a/test/Makefile b/test/Makefile
index d6358a93..ae23ef6d 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -293,6 +293,7 @@ test_srcs := \
 	timerfd-short-read.c \
 	timeout.c \
 	timeout-new.c \
+	timeout-swallow.c \
 	timestamp.c \
 	timestamp-bug.c \
 	truncate.c \
diff --git a/test/timeout-swallow.c b/test/timeout-swallow.c
new file mode 100644
index 00000000..9fb4ab01
--- /dev/null
+++ b/test/timeout-swallow.c
@@ -0,0 +1,117 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Description: tests that io_uring_wait_cqes() and variants do not swallow 
+ *              -ETIME when loop-fetching CQEs if some SQEs were submitted.
+ */
+#include <stdio.h>
+#include <unistd.h>
+#include <errno.h>
+#include <sys/time.h>
+#include "liburing.h"
+#include "helpers.h"
+
+/*
+ * Test the normal -ETIME swallow path. (line 118 in queue.c)
+ */
+static int test_timeout(struct io_uring *ring)
+{
+    struct io_uring_sqe *sqe;
+    struct io_uring_cqe *cqe;
+    struct __kernel_timespec long_ts = { .tv_sec = 10, .tv_nsec = 0 };
+    struct __kernel_timespec zero_ts = { .tv_sec = 0, .tv_nsec = 0 };
+    int ret;
+
+    if (!(ring->features & IORING_FEAT_EXT_ARG))
+        return T_EXIT_SKIP;
+
+    sqe = io_uring_get_sqe(ring);
+    if (!sqe) {
+        fprintf(stderr, "get_sqe failed\n");
+        return T_EXIT_FAIL;
+    }
+    /* Long timeout, won't complete immediately */
+    io_uring_prep_timeout(sqe, &long_ts, 1, 0);
+    
+    /* Zero timeout forces immediate expiry inside the syscall on first/second pass */
+    ret = io_uring_submit_and_wait_timeout(ring, &cqe, 2, &zero_ts, NULL);
+    
+    if (ret == -ETIME) {
+        return T_EXIT_PASS;
+    }
+
+    fprintf(stderr, "test_timeout failed: expected -ETIME, got %d\n", ret);
+    return T_EXIT_FAIL;
+}
+
+/*
+ * Test the registered-wait -ETIME swallow path. (line 113 in queue.c)
+ */
+static int test_timeout_reg(struct io_uring *ring)
+{
+    struct io_uring_sqe *sqe;
+    struct io_uring_cqe *cqe;
+    struct __kernel_timespec long_ts = { .tv_sec = 10, .tv_nsec = 0 };
+    struct io_uring_reg_wait reg = { .ts = { .tv_sec = 0, .tv_nsec = 0 } };
+    int ret;
+
+    if (!(ring->features & IORING_FEAT_EXT_ARG))
+        return T_EXIT_SKIP;
+
+    ret = io_uring_register_wait_reg(ring, &reg, 1);
+    if (ret) {
+        /* Not supported on this kernel */
+        return T_EXIT_SKIP;
+    }
+
+    sqe = io_uring_get_sqe(ring);
+    if (!sqe) {
+        fprintf(stderr, "get_sqe failed\n");
+        return T_EXIT_FAIL;
+    }
+    io_uring_prep_timeout(sqe, &long_ts, 1, 0);
+    
+    ret = io_uring_submit_and_wait_reg(ring, &cqe, 2, 0);
+    if (ret == -ETIME) {
+        return T_EXIT_PASS;
+    }
+
+    fprintf(stderr, "test_timeout_reg failed: expected -ETIME, got %d\n", ret);
+    return T_EXIT_FAIL;
+}
+
+int main(int argc, char *argv[])
+{
+    struct io_uring ring;
+    int ret, ret2, final_ret = T_EXIT_PASS;
+
+    if (argc > 1)
+        return T_EXIT_SKIP;
+
+    ret = io_uring_queue_init(8, &ring, 0);
+    if (ret) {
+        if (ret == -ENOSYS)
+            return T_EXIT_SKIP;
+        fprintf(stderr, "queue_init failed: %d\n", ret);
+        return T_EXIT_FAIL;
+    }
+
+    ret = test_timeout(&ring);
+    if (ret == T_EXIT_FAIL) {
+        fprintf(stderr, "test_timeout failed\n");
+        final_ret = T_EXIT_FAIL;
+    }
+
+    ret2 = test_timeout_reg(&ring);
+    if (ret2 == T_EXIT_FAIL) {
+        fprintf(stderr, "test_timeout_reg failed\n");
+        final_ret = T_EXIT_FAIL;
+    }
+
+    io_uring_queue_exit(&ring);
+
+    if (final_ret == T_EXIT_FAIL)
+        return T_EXIT_FAIL;
+    if (ret == T_EXIT_SKIP && ret2 == T_EXIT_SKIP)
+        return T_EXIT_SKIP;
+    return T_EXIT_PASS;
+}
-- 
2.43.0

Re: [PATCH 1/2] src/queue: don't swallow -ETIME when SQEs were submitted

[Gabriel Krisman Bertazi]

Prateek <kprateek283@gmail.com> writes:

> If _io_uring_get_cqe() submits SQEs and then times out waiting for
> completions, it returns the submit count instead of -ETIME:
>
>   1. The first enter submits the SQEs; because submit > 0 the kernel
>      returns the submit count, not -ETIME, and it is stored in err.
>   2. On the next iteration the has_ts shortcut wants to report -ETIME,
>      but the 'if (!err)' guard sees the non-zero submit count and keeps
>      it, so -ETIME is dropped.
>
> That contradicts io_uring_submit_and_wait_timeout(3) and
> io_uring_wait_cqes(3), which document -ETIME on timeout.
>
> At these two sites (lines 113 and 118) err is only ever 0 or a positive
> submit count. A negative error from __io_uring_peek_cqe() or a prior
> enter breaks out of the loop before reaching here. So the change is
> functionally equivalent to dropping the err condition entirely; we change
> '!err' to 'err >= 0' so -ETIME is successfully synthesized whenever no
> CQE was seen.
>
> The guards were added in 2f61e849 ("src/queue: don't wait twice if
> looping in _io_uring_get_cqe()") to carry the submit count across
> iterations for the partial-completion case (got some CQEs, no error);
> that case still returns the count because both sites remain guarded by
> !cqe.
>
> Signed-off-by: Prateek <kprateek283@gmail.com>


Reviewed-by: Gabriel Krisman Bertazi <krisman@suse.de>

-- 
Gabriel Krisman Bertazi

Re: [PATCH 2/2] test/timeout-swallow: verify -ETIME is not swallowed

[Gabriel Krisman Bertazi]

Prateek <kprateek283@gmail.com> writes:

> Regression test for the previous commit. Submits an SQE and waits with a
> zero timeout for more completions than can arrive; the result must be
> -ETIME, not the positive submit count. Covers the normal EXT_ARG wait
> path and the registered-wait path, each skipped gracefully where
> unsupported.
>
> Signed-off-by: Prateek <kprateek283@gmail.com>
> ---
>  test/Makefile          |   1 +
>  test/timeout-swallow.c | 117 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 118 insertions(+)
>  create mode 100644 test/timeout-swallow.c
>
> diff --git a/test/Makefile b/test/Makefile
> index d6358a93..ae23ef6d 100644
> --- a/test/Makefile
> +++ b/test/Makefile
> @@ -293,6 +293,7 @@ test_srcs := \
>  	timerfd-short-read.c \
>  	timeout.c \
>  	timeout-new.c \
> +	timeout-swallow.c \
>  	timestamp.c \
>  	timestamp-bug.c \
>  	truncate.c \
> diff --git a/test/timeout-swallow.c b/test/timeout-swallow.c
> new file mode 100644
> index 00000000..9fb4ab01
> --- /dev/null
> +++ b/test/timeout-swallow.c
> @@ -0,0 +1,117 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Description: tests that io_uring_wait_cqes() and variants do not swallow 
> + *              -ETIME when loop-fetching CQEs if some SQEs were submitted.
> + */
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <errno.h>
> +#include <sys/time.h>
> +#include "liburing.h"
> +#include "helpers.h"
> +
> +/*
> + * Test the normal -ETIME swallow path. (line 118 in queue.c)

This type of comment is useless as line number changes.  Better to refer to
function name in the API and describe the situation or not have anything
at all.  Other than that,


Reviewed-by: Gabriel Krisman Bertazi <krisman@suse.de>




-- 
Gabriel Krisman Bertazi