* [PATCH] t/io_uring: fix error handling for setup_ring [not found] <CGME20230427094952epcas5p32967ed548e18e60ea3134a03aa046bb8@epcas5p3.samsung.com> @ 2023-04-27 15:10 ` Anuj Gupta 2023-04-27 11:48 ` Jens Axboe 0 siblings, 1 reply; 5+ messages in thread From: Anuj Gupta @ 2023-04-27 15:10 UTC (permalink / raw) To: axboe; +Cc: vincent.fu, fio, ankit.kumar, anuj1072538, Anuj Gupta s->sq_ring.ring_entries and s->cq_ring.ring_entries will be NULL, incase setup_ring fails. This will cause a segmentation fault. In case setup_ring fails, bail out by setting finish. Signed-off-by: Anuj Gupta <anuj20.g@samsung.com> --- t/io_uring.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/t/io_uring.c b/t/io_uring.c index 6b0efef8..e00f278d 100644 --- a/t/io_uring.c +++ b/t/io_uring.c @@ -1059,12 +1059,14 @@ static int submitter_init(struct submitter *s) err = 0; } else if (!aio) { err = setup_ring(s); - sprintf(buf, "Engine=io_uring, sq_ring=%d, cq_ring=%d\n", *s->sq_ring.ring_entries, *s->cq_ring.ring_entries); + if (!err) + sprintf(buf, "Engine=io_uring, sq_ring=%d, cq_ring=%d\n", *s->sq_ring.ring_entries, *s->cq_ring.ring_entries); } else { sprintf(buf, "Engine=aio\n"); err = setup_aio(s); } if (err) { + finish = 1; printf("queue setup failed: %s, %d\n", strerror(errno), err); return 1; } @@ -1282,6 +1284,9 @@ static void *submitter_uring_fn(void *data) submitter_init(s); #endif + if (finish) + return NULL; + if (register_ring) io_uring_register_ring(s); -- 2.25.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] t/io_uring: fix error handling for setup_ring 2023-04-27 15:10 ` [PATCH] t/io_uring: fix error handling for setup_ring Anuj Gupta @ 2023-04-27 11:48 ` Jens Axboe 2023-04-27 11:53 ` Jens Axboe 0 siblings, 1 reply; 5+ messages in thread From: Jens Axboe @ 2023-04-27 11:48 UTC (permalink / raw) To: Anuj Gupta; +Cc: vincent.fu, fio, ankit.kumar, anuj1072538 On 4/27/23 9:10 AM, Anuj Gupta wrote: > s->sq_ring.ring_entries and s->cq_ring.ring_entries will be NULL, > incase setup_ring fails. This will cause a segmentation fault. > > In case setup_ring fails, bail out by setting finish. Any reason why we don't just use the return code of submitter_init() on whether to abort or not? -- Jens Axboe ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] t/io_uring: fix error handling for setup_ring 2023-04-27 11:48 ` Jens Axboe @ 2023-04-27 11:53 ` Jens Axboe 2023-04-27 12:26 ` Anuj gupta 0 siblings, 1 reply; 5+ messages in thread From: Jens Axboe @ 2023-04-27 11:53 UTC (permalink / raw) To: Anuj Gupta; +Cc: vincent.fu, fio, ankit.kumar, anuj1072538 On 4/27/23 5:48?AM, Jens Axboe wrote: > On 4/27/23 9:10?AM, Anuj Gupta wrote: >> s->sq_ring.ring_entries and s->cq_ring.ring_entries will be NULL, >> incase setup_ring fails. This will cause a segmentation fault. >> >> In case setup_ring fails, bail out by setting finish. > > Any reason why we don't just use the return code of submitter_init() > on whether to abort or not? Something like this as a prep patch, then do the trivial version of your patch after that. Totally untested... diff --git a/t/io_uring.c b/t/io_uring.c index 6b0efef85cb7..2f8d63fbe67e 100644 --- a/t/io_uring.c +++ b/t/io_uring.c @@ -1049,7 +1049,7 @@ static int submitter_init(struct submitter *s) buf = allocate_mem(s, bs); if (!buf) - return 1; + return -1; s->iovecs[i].iov_base = buf; s->iovecs[i].iov_len = bs; } @@ -1066,7 +1066,7 @@ static int submitter_init(struct submitter *s) } if (err) { printf("queue setup failed: %s, %d\n", strerror(errno), err); - return 1; + return -1; } if (!init_printed) { @@ -1172,9 +1172,15 @@ static void *submitter_aio_fn(void *data) struct iocb *iocbs; struct io_event *events; #ifdef ARCH_HAVE_CPU_CLOCK - int nr_batch = submitter_init(s); -#else - submitter_init(s); + int nr_batch; +#endif + + ret = submitter_init(s); + if (ret < 0) + goto done; + +#ifdef ARCH_HAVE_CPU_CLOCK + nr_batch = ret; #endif iocbsptr = calloc(depth, sizeof(struct iocb *)); @@ -1238,6 +1244,7 @@ static void *submitter_aio_fn(void *data) free(iocbsptr); free(iocbs); free(events); +done: finish = 1; return NULL; } @@ -1277,9 +1284,15 @@ static void *submitter_uring_fn(void *data) struct io_sq_ring *ring = &s->sq_ring; int ret, prepped; #ifdef ARCH_HAVE_CPU_CLOCK - int nr_batch = submitter_init(s); -#else - submitter_init(s); + int nr_batch; +#endif + + ret = submitter_init(s); + if (ret < 0) + goto done; + +#ifdef ARCH_HAVE_CPU_CLOCK + nr_batch = ret; #endif if (register_ring) @@ -1383,6 +1396,7 @@ submit: if (register_ring) io_uring_unregister_ring(s); +done: finish = 1; return NULL; } @@ -1393,7 +1407,8 @@ static void *submitter_sync_fn(void *data) struct submitter *s = data; int ret; - submitter_init(s); + if (submitter_init(s) < 0) + goto done; do { uint64_t offset; @@ -1429,6 +1444,7 @@ static void *submitter_sync_fn(void *data) add_stat(s, s->clock_index, 1); } while (!s->finish); +done: finish = 1; return NULL; } -- Jens Axboe ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] t/io_uring: fix error handling for setup_ring 2023-04-27 11:53 ` Jens Axboe @ 2023-04-27 12:26 ` Anuj gupta 2023-04-27 13:22 ` Jens Axboe 0 siblings, 1 reply; 5+ messages in thread From: Anuj gupta @ 2023-04-27 12:26 UTC (permalink / raw) To: Jens Axboe; +Cc: Anuj Gupta, vincent.fu, fio, ankit.kumar On Thu, Apr 27, 2023 at 5:23 PM Jens Axboe <axboe@kernel.dk> wrote: > > On 4/27/23 5:48?AM, Jens Axboe wrote: > > On 4/27/23 9:10?AM, Anuj Gupta wrote: > >> s->sq_ring.ring_entries and s->cq_ring.ring_entries will be NULL, > >> incase setup_ring fails. This will cause a segmentation fault. > >> > >> In case setup_ring fails, bail out by setting finish. > > > > Any reason why we don't just use the return code of submitter_init() > > on whether to abort or not? > > Something like this as a prep patch, then do the trivial version > of your patch after that. > > Totally untested... > > > diff --git a/t/io_uring.c b/t/io_uring.c > index 6b0efef85cb7..2f8d63fbe67e 100644 > --- a/t/io_uring.c > +++ b/t/io_uring.c > @@ -1049,7 +1049,7 @@ static int submitter_init(struct submitter *s) > > buf = allocate_mem(s, bs); > if (!buf) > - return 1; > + return -1; > s->iovecs[i].iov_base = buf; > s->iovecs[i].iov_len = bs; > } > @@ -1066,7 +1066,7 @@ static int submitter_init(struct submitter *s) > } > if (err) { > printf("queue setup failed: %s, %d\n", strerror(errno), err); > - return 1; > + return -1; > } > > if (!init_printed) { > @@ -1172,9 +1172,15 @@ static void *submitter_aio_fn(void *data) > struct iocb *iocbs; > struct io_event *events; > #ifdef ARCH_HAVE_CPU_CLOCK > - int nr_batch = submitter_init(s); > -#else > - submitter_init(s); > + int nr_batch; > +#endif > + > + ret = submitter_init(s); > + if (ret < 0) > + goto done; > + > +#ifdef ARCH_HAVE_CPU_CLOCK > + nr_batch = ret; > #endif > > iocbsptr = calloc(depth, sizeof(struct iocb *)); > @@ -1238,6 +1244,7 @@ static void *submitter_aio_fn(void *data) > free(iocbsptr); > free(iocbs); > free(events); > +done: > finish = 1; > return NULL; > } > @@ -1277,9 +1284,15 @@ static void *submitter_uring_fn(void *data) > struct io_sq_ring *ring = &s->sq_ring; > int ret, prepped; > #ifdef ARCH_HAVE_CPU_CLOCK > - int nr_batch = submitter_init(s); > -#else > - submitter_init(s); > + int nr_batch; > +#endif > + > + ret = submitter_init(s); > + if (ret < 0) > + goto done; > + > +#ifdef ARCH_HAVE_CPU_CLOCK > + nr_batch = ret; > #endif > > if (register_ring) > @@ -1383,6 +1396,7 @@ submit: > if (register_ring) > io_uring_unregister_ring(s); > > +done: > finish = 1; > return NULL; > } > @@ -1393,7 +1407,8 @@ static void *submitter_sync_fn(void *data) > struct submitter *s = data; > int ret; > > - submitter_init(s); > + if (submitter_init(s) < 0) > + goto done; > > do { > uint64_t offset; > @@ -1429,6 +1444,7 @@ static void *submitter_sync_fn(void *data) > add_stat(s, s->clock_index, 1); > } while (!s->finish); > > +done: > finish = 1; > return NULL; > } > Tested this and the patch works out fine. This can go on top of this to avoid access to NULL pointer - --- a/t/io_uring.c +++ b/t/io_uring.c @@ -1059,7 +1059,8 @@ static int submitter_init(struct submitter s) err = 0; } else if (!aio) { err = setup_ring(s); - sprintf(buf, "Engine=io_uring, sq_ring=%d, cq_ring=%d\n",s->sq_ring.ring_entries, s->cq_ring.ring_entries); + if (!err) + sprintf(buf, "Engine=io_uring, sq_ring=%d, cq_ring=%d\n",s->sq_ring.ring_entries, *s->cq_ring.ring_entries); } else { sprintf(buf, "Engine=aio\n"); err = setup_aio(s); > -- > Jens Axboe > -- Anuj Gupta ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] t/io_uring: fix error handling for setup_ring 2023-04-27 12:26 ` Anuj gupta @ 2023-04-27 13:22 ` Jens Axboe 0 siblings, 0 replies; 5+ messages in thread From: Jens Axboe @ 2023-04-27 13:22 UTC (permalink / raw) To: Anuj gupta; +Cc: Anuj Gupta, vincent.fu, fio, ankit.kumar On 4/27/23 6:26?AM, Anuj gupta wrote: > On Thu, Apr 27, 2023 at 5:23?PM Jens Axboe <axboe@kernel.dk> wrote: >> >> On 4/27/23 5:48?AM, Jens Axboe wrote: >>> On 4/27/23 9:10?AM, Anuj Gupta wrote: >>>> s->sq_ring.ring_entries and s->cq_ring.ring_entries will be NULL, >>>> incase setup_ring fails. This will cause a segmentation fault. >>>> >>>> In case setup_ring fails, bail out by setting finish. >>> >>> Any reason why we don't just use the return code of submitter_init() >>> on whether to abort or not? >> >> Something like this as a prep patch, then do the trivial version >> of your patch after that. >> >> Totally untested... >> >> >> diff --git a/t/io_uring.c b/t/io_uring.c >> index 6b0efef85cb7..2f8d63fbe67e 100644 >> --- a/t/io_uring.c >> +++ b/t/io_uring.c >> @@ -1049,7 +1049,7 @@ static int submitter_init(struct submitter *s) >> >> buf = allocate_mem(s, bs); >> if (!buf) >> - return 1; >> + return -1; >> s->iovecs[i].iov_base = buf; >> s->iovecs[i].iov_len = bs; >> } >> @@ -1066,7 +1066,7 @@ static int submitter_init(struct submitter *s) >> } >> if (err) { >> printf("queue setup failed: %s, %d\n", strerror(errno), err); >> - return 1; >> + return -1; >> } >> >> if (!init_printed) { >> @@ -1172,9 +1172,15 @@ static void *submitter_aio_fn(void *data) >> struct iocb *iocbs; >> struct io_event *events; >> #ifdef ARCH_HAVE_CPU_CLOCK >> - int nr_batch = submitter_init(s); >> -#else >> - submitter_init(s); >> + int nr_batch; >> +#endif >> + >> + ret = submitter_init(s); >> + if (ret < 0) >> + goto done; >> + >> +#ifdef ARCH_HAVE_CPU_CLOCK >> + nr_batch = ret; >> #endif >> >> iocbsptr = calloc(depth, sizeof(struct iocb *)); >> @@ -1238,6 +1244,7 @@ static void *submitter_aio_fn(void *data) >> free(iocbsptr); >> free(iocbs); >> free(events); >> +done: >> finish = 1; >> return NULL; >> } >> @@ -1277,9 +1284,15 @@ static void *submitter_uring_fn(void *data) >> struct io_sq_ring *ring = &s->sq_ring; >> int ret, prepped; >> #ifdef ARCH_HAVE_CPU_CLOCK >> - int nr_batch = submitter_init(s); >> -#else >> - submitter_init(s); >> + int nr_batch; >> +#endif >> + >> + ret = submitter_init(s); >> + if (ret < 0) >> + goto done; >> + >> +#ifdef ARCH_HAVE_CPU_CLOCK >> + nr_batch = ret; >> #endif >> >> if (register_ring) >> @@ -1383,6 +1396,7 @@ submit: >> if (register_ring) >> io_uring_unregister_ring(s); >> >> +done: >> finish = 1; >> return NULL; >> } >> @@ -1393,7 +1407,8 @@ static void *submitter_sync_fn(void *data) >> struct submitter *s = data; >> int ret; >> >> - submitter_init(s); >> + if (submitter_init(s) < 0) >> + goto done; >> >> do { >> uint64_t offset; >> @@ -1429,6 +1444,7 @@ static void *submitter_sync_fn(void *data) >> add_stat(s, s->clock_index, 1); >> } while (!s->finish); >> >> +done: >> finish = 1; >> return NULL; >> } >> > > Tested this and the patch works out fine. This can go on top of this > to avoid access to NULL pointer - Thanks! Can you send this one as a real patch to make my life just a tiny bit easier? -- Jens Axboe ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-04-27 13:22 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20230427094952epcas5p32967ed548e18e60ea3134a03aa046bb8@epcas5p3.samsung.com>
2023-04-27 15:10 ` [PATCH] t/io_uring: fix error handling for setup_ring Anuj Gupta
2023-04-27 11:48 ` Jens Axboe
2023-04-27 11:53 ` Jens Axboe
2023-04-27 12:26 ` Anuj gupta
2023-04-27 13:22 ` Jens Axboe
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.