| Message ID | 20230111101907.600820-1-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring: Add NULL checks for current->io_uring | expand |
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"?
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 <baijiaju1990@gmail.com> > Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> > --- > 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; >
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 :-)
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;
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 <baijiaju1990@gmail.com> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> --- io_uring/io_uring.c | 3 ++- io_uring/io_uring.h | 3 +++ io_uring/tctx.c | 9 ++++++++- 3 files changed, 13 insertions(+), 2 deletions(-)