Re: [PATCH] t/io_uring: fix error handling for setup_ring

Re: [PATCH] t/io_uring: fix error handling for setup_ring

[Jens Axboe]

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

Re: [PATCH] t/io_uring: fix error handling for setup_ring

[Jens Axboe]

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

Re: [PATCH] t/io_uring: fix error handling for setup_ring

[Anuj gupta]

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

Re: [PATCH] t/io_uring: fix error handling for setup_ring

[Jens Axboe]

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

[PATCH] t/io_uring: fix error handling for setup_ring

[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