As described in a previous commit 998b30c3948e, current->io_uring could
be NULL, and thus a NULL check is required for this variable.
In the same way, other functions that access current->io_uring also
require NULL checks of this variable.
Signed-off-by: Jia-Ju Bai <[email protected]>
Reported-by: TOTE Robot <[email protected]>
---
io_uring/io_uring.c | 3 ++-
io_uring/io_uring.h | 3 +++
io_uring/tctx.c | 9 ++++++++-
3 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 2ac1cd8d23ea..8075c0880c7a 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -2406,7 +2406,8 @@ int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr)
/* try again if it submitted nothing and can't allocate a req */
if (!ret && io_req_cache_empty(ctx))
ret = -EAGAIN;
- current->io_uring->cached_refs += left;
+ if (likely(current->io_uring))
+ current->io_uring->cached_refs += left;
}
io_submit_state_end(ctx);
diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
index ab4b2a1c3b7e..398c7c2ba22b 100644
--- a/io_uring/io_uring.h
+++ b/io_uring/io_uring.h
@@ -362,6 +362,9 @@ static inline void io_get_task_refs(int nr)
{
struct io_uring_task *tctx = current->io_uring;
+ if (unlikely(!tctx))
+ return;
+
tctx->cached_refs -= nr;
if (unlikely(tctx->cached_refs < 0))
io_task_refs_refill(tctx);
diff --git a/io_uring/tctx.c b/io_uring/tctx.c
index 4324b1cf1f6a..6574bbe82b5d 100644
--- a/io_uring/tctx.c
+++ b/io_uring/tctx.c
@@ -145,7 +145,8 @@ int __io_uring_add_tctx_node_from_submit(struct io_ring_ctx *ctx)
if (ret)
return ret;
- current->io_uring->last = ctx;
+ if (likely(current->io_uring))
+ current->io_uring->last = ctx;
return 0;
}
@@ -200,6 +201,9 @@ void io_uring_unreg_ringfd(void)
struct io_uring_task *tctx = current->io_uring;
int i;
+ if (unlikely(!tctx))
+ return;
+
for (i = 0; i < IO_RINGFD_REG_MAX; i++) {
if (tctx->registered_rings[i]) {
fput(tctx->registered_rings[i]);
@@ -259,6 +263,9 @@ int io_ringfd_register(struct io_ring_ctx *ctx, void __user *__arg,
return ret;
tctx = current->io_uring;
+ if (unlikely(!tctx))
+ return -EINVAL;
+
for (i = 0; i < nr_args; i++) {
int start, end;
--
2.34.1
On 1/11/23 3:19 AM, Jia-Ju Bai wrote:
> As described in a previous commit 998b30c3948e, current->io_uring could
> be NULL, and thus a NULL check is required for this variable.
>
> In the same way, other functions that access current->io_uring also
> require NULL checks of this variable.
This seems odd. Have you actually seen traces of this, or is it just
based on "guess it can be NULL sometimes, check it in all spots"?
--
Jens Axboe
On 1/11/23 10:19, Jia-Ju Bai wrote:
> As described in a previous commit 998b30c3948e, current->io_uring could
> be NULL, and thus a NULL check is required for this variable.
>
> In the same way, other functions that access current->io_uring also
> require NULL checks of this variable.
io_uring_enter() {
...
ret = io_uring_add_tctx_node(ctx);
if (unlikely(ret))
goto out;
mutex_lock(&ctx->uring_lock);
ret = io_submit_sqes(ctx, to_submit);
}
io_uring_add_tctx_node() should make sure to setup current->io_uring
or fail, so it should be NULL there, and SQPOLL should be fine as well.
Did you see it hitting NULL?
>
> Signed-off-by: Jia-Ju Bai <[email protected]>
> Reported-by: TOTE Robot <[email protected]>
> ---
> io_uring/io_uring.c | 3 ++-
> io_uring/io_uring.h | 3 +++
> io_uring/tctx.c | 9 ++++++++-
> 3 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index 2ac1cd8d23ea..8075c0880c7a 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -2406,7 +2406,8 @@ int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr)
> /* try again if it submitted nothing and can't allocate a req */
> if (!ret && io_req_cache_empty(ctx))
> ret = -EAGAIN;
> - current->io_uring->cached_refs += left;
> + if (likely(current->io_uring))
> + current->io_uring->cached_refs += left;
> }
>
> io_submit_state_end(ctx);
> diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
> index ab4b2a1c3b7e..398c7c2ba22b 100644
> --- a/io_uring/io_uring.h
> +++ b/io_uring/io_uring.h
> @@ -362,6 +362,9 @@ static inline void io_get_task_refs(int nr)
> {
> struct io_uring_task *tctx = current->io_uring;
>
> + if (unlikely(!tctx))
> + return;
> +
> tctx->cached_refs -= nr;
> if (unlikely(tctx->cached_refs < 0))
> io_task_refs_refill(tctx);
> diff --git a/io_uring/tctx.c b/io_uring/tctx.c
> index 4324b1cf1f6a..6574bbe82b5d 100644
> --- a/io_uring/tctx.c
> +++ b/io_uring/tctx.c
> @@ -145,7 +145,8 @@ int __io_uring_add_tctx_node_from_submit(struct io_ring_ctx *ctx)
> if (ret)
> return ret;
>
> - current->io_uring->last = ctx;
> + if (likely(current->io_uring))
> + current->io_uring->last = ctx;
> return 0;
> }
>
> @@ -200,6 +201,9 @@ void io_uring_unreg_ringfd(void)
> struct io_uring_task *tctx = current->io_uring;
> int i;
>
> + if (unlikely(!tctx))
> + return;
> +
> for (i = 0; i < IO_RINGFD_REG_MAX; i++) {
> if (tctx->registered_rings[i]) {
> fput(tctx->registered_rings[i]);
> @@ -259,6 +263,9 @@ int io_ringfd_register(struct io_ring_ctx *ctx, void __user *__arg,
> return ret;
>
> tctx = current->io_uring;
> + if (unlikely(!tctx))
> + return -EINVAL;
> +
> for (i = 0; i < nr_args; i++) {
> int start, end;
>
--
Pavel Begunkov
On 2023/1/11 22:49, Jens Axboe wrote:
> On 1/11/23 3:19 AM, Jia-Ju Bai wrote:
>> As described in a previous commit 998b30c3948e, current->io_uring could
>> be NULL, and thus a NULL check is required for this variable.
>>
>> In the same way, other functions that access current->io_uring also
>> require NULL checks of this variable.
> This seems odd. Have you actually seen traces of this, or is it just
> based on "guess it can be NULL sometimes, check it in all spots"?
>
Thanks for the reply!
I checked the previous commit and inferred that there may be some problems.
I am not quite sure of this, and thus want to listen to your opinions :)
Best wishes,
Jia-Ju Bai
On 1/11/23 7:10 PM, Jia-Ju Bai wrote:
>
>
> On 2023/1/11 22:49, Jens Axboe wrote:
>> On 1/11/23 3:19 AM, Jia-Ju Bai wrote:
>>> As described in a previous commit 998b30c3948e, current->io_uring could
>>> be NULL, and thus a NULL check is required for this variable.
>>>
>>> In the same way, other functions that access current->io_uring also
>>> require NULL checks of this variable.
>> This seems odd. Have you actually seen traces of this, or is it just
>> based on "guess it can be NULL sometimes, check it in all spots"?
>>
>
> Thanks for the reply!
> I checked the previous commit and inferred that there may be some problems.
> I am not quite sure of this, and thus want to listen to your opinions :)
I'd invite you to look over each of them separately, and see if that path
could potentially lead to current->io_uring == NULL and thus being an
issue. I think that'd be a useful exercise, and you never know that you
might find :-)
--
Jens Axboe