[Bug] io_uring_register_files_update broken

[Bug] io_uring_register_files_update broken

[Victor Stewart]

i'm fairly confident there is something broken with
io_uring_register_files_update,
especially the offset parameter.

when trying to update a single fd, and getting a successful result of
1, proceeding
operations with IOSQE_FIXED_FILE fail with -9. but if i update all of
the fds with
then my recv operations succeed, but close still fails with -9.

on Clear LInux 5.12.13-1050.native

here's a diff for liburing send_recv test, to demonstrate this.

diff --git a/test/send_recv.c b/test/send_recv.c
index 19adbdd..492b591 100644
--- a/test/send_recv.c
+++ b/test/send_recv.c
@@ -27,6 +27,8 @@ static char str[] = "This is a test of send and recv
over io_uring!";
 #      define io_uring_prep_recv io_uring_prep_read
 #endif

+static int *fds;
+
 static int recv_prep(struct io_uring *ring, struct iovec *iov, int *sock,
                     int registerfiles)
 {
@@ -54,17 +56,28 @@ static int recv_prep(struct io_uring *ring, struct
iovec *iov, int *sock,
                goto err;
        }

+       fds = malloc(100 * sizeof(int));
+       memset(fds, 0xff, sizeof(int) * 100);
+
        if (registerfiles) {
-               ret = io_uring_register_files(ring, &sockfd, 1);
+               ret = io_uring_register_files(ring, fds, 100);
                if (ret) {
                        fprintf(stderr, "file reg failed\n");
                        goto err;
                }
-               use_fd = 0;
-       } else {
-               use_fd = sockfd;
+
+               fds[sockfd] = sockfd;
+               int result = io_uring_register_files_update(ring,
sockfd, fds, 1);
+
+               if (result != 1)
+               {
+                       fprintf(stderr, "file update failed\n");
+                       goto err;
+               }
        }

+       use_fd = sockfd;
+
        sqe = io_uring_get_sqe(ring);
        io_uring_prep_recv(sqe, use_fd, iov->iov_base, iov->iov_len, 0);
        if (registerfiles)

Re: [Bug] io_uring_register_files_update broken

[Pavel Begunkov]

On 6/30/21 10:14 PM, Victor Stewart wrote:
> i'm fairly confident there is something broken with
> io_uring_register_files_update,
> especially the offset parameter.
> 
> when trying to update a single fd, and getting a successful result of
> 1, proceeding
> operations with IOSQE_FIXED_FILE fail with -9. but if i update all of
> the fds with
> then my recv operations succeed, but close still fails with -9.
> 
> on Clear LInux 5.12.13-1050.native

Thanks for letting know, I'll take a look

 
> here's a diff for liburing send_recv test, to demonstrate this.
> 
> diff --git a/test/send_recv.c b/test/send_recv.c
> index 19adbdd..492b591 100644
> --- a/test/send_recv.c
> +++ b/test/send_recv.c
> @@ -27,6 +27,8 @@ static char str[] = "This is a test of send and recv
> over io_uring!";
>  #      define io_uring_prep_recv io_uring_prep_read
>  #endif
> 
> +static int *fds;
> +
>  static int recv_prep(struct io_uring *ring, struct iovec *iov, int *sock,
>                      int registerfiles)
>  {
> @@ -54,17 +56,28 @@ static int recv_prep(struct io_uring *ring, struct
> iovec *iov, int *sock,
>                 goto err;
>         }
> 
> +       fds = malloc(100 * sizeof(int));
> +       memset(fds, 0xff, sizeof(int) * 100);
> +
>         if (registerfiles) {
> -               ret = io_uring_register_files(ring, &sockfd, 1);
> +               ret = io_uring_register_files(ring, fds, 100);
>                 if (ret) {
>                         fprintf(stderr, "file reg failed\n");
>                         goto err;
>                 }
> -               use_fd = 0;
> -       } else {
> -               use_fd = sockfd;
> +
> +               fds[sockfd] = sockfd;
> +               int result = io_uring_register_files_update(ring,
> sockfd, fds, 1);
> +
> +               if (result != 1)
> +               {
> +                       fprintf(stderr, "file update failed\n");
> +                       goto err;
> +               }
>         }
> 
> +       use_fd = sockfd;
> +
>         sqe = io_uring_get_sqe(ring);
>         io_uring_prep_recv(sqe, use_fd, iov->iov_base, iov->iov_len, 0);
>         if (registerfiles)
> 

-- 
Pavel Begunkov

Re: [Bug] io_uring_register_files_update broken

[Daniele Salvatore Albano]

On Thu, 1 Jul 2021 at 00:28, Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> On 6/30/21 10:14 PM, Victor Stewart wrote:
> > i'm fairly confident there is something broken with
> > io_uring_register_files_update,
> > especially the offset parameter.
> >
> > when trying to update a single fd, and getting a successful result of
> > 1, proceeding
> > operations with IOSQE_FIXED_FILE fail with -9. but if i update all of
> > the fds with
> > then my recv operations succeed, but close still fails with -9.
> >
> > on Clear LInux 5.12.13-1050.native
>
> Thanks for letting know, I'll take a look
>
>
> > here's a diff for liburing send_recv test, to demonstrate this.
> >
> > diff --git a/test/send_recv.c b/test/send_recv.c
> > index 19adbdd..492b591 100644
> > --- a/test/send_recv.c
> > +++ b/test/send_recv.c
> > @@ -27,6 +27,8 @@ static char str[] = "This is a test of send and recv
> > over io_uring!";
> >  #      define io_uring_prep_recv io_uring_prep_read
> >  #endif
> >
> > +static int *fds;
> > +
> >  static int recv_prep(struct io_uring *ring, struct iovec *iov, int *sock,
> >                      int registerfiles)
> >  {
> > @@ -54,17 +56,28 @@ static int recv_prep(struct io_uring *ring, struct
> > iovec *iov, int *sock,
> >                 goto err;
> >         }
> >
> > +       fds = malloc(100 * sizeof(int));
> > +       memset(fds, 0xff, sizeof(int) * 100);
> > +
> >         if (registerfiles) {
> > -               ret = io_uring_register_files(ring, &sockfd, 1);
> > +               ret = io_uring_register_files(ring, fds, 100);
> >                 if (ret) {
> >                         fprintf(stderr, "file reg failed\n");
> >                         goto err;
> >                 }
> > -               use_fd = 0;
> > -       } else {
> > -               use_fd = sockfd;
> > +
> > +               fds[sockfd] = sockfd;
> > +               int result = io_uring_register_files_update(ring,
> > sockfd, fds, 1);
> > +
> > +               if (result != 1)
> > +               {
> > +                       fprintf(stderr, "file update failed\n");
> > +                       goto err;
> > +               }
> >         }
> >
> > +       use_fd = sockfd;
> > +
> >         sqe = io_uring_get_sqe(ring);
> >         io_uring_prep_recv(sqe, use_fd, iov->iov_base, iov->iov_len, 0);
> >         if (registerfiles)
> >
>
> --
> Pavel Begunkov
I sent an email a while ago to raise a question about a potential bug
related to close.

Looks like the close doesn't support registered files (although I saw
some code within a patch from Jens to fix it while I was
investigating).

Attached below

On Fri, 21 May 2021 at 21:28, Daniele Salvatore Albano
<d.albano@gmail.com> wrote:
>
> Hi,
>
> Is there any specific reason for which io_close_prep returns EBADF if
> using REQ_F_FIXED_FILE?
>
> I discovered my software was failing to close sockets when using fixed
> files a while ago but I put it to the side, initially thinking it was
> a bug I introduced in my code.
> In recent days I picked it up again and after investigating it, it
> looks like that, instead, that's the expected behaviour.
>
> From what I see, although the behaviour was slightly changed with a
> couple of commits (ie. with
> https://github.com/torvalds/linux/commit/cf3040ca55f2085b0a372a620ee2cb93ae19b686
> ) the io_close_prep have had this behaviour from the very beginning
> https://github.com/torvalds/linux/commit/b5dba59e0cf7e2cc4d3b3b1ac5fe81ddf21959eb
> .
>
> @Jens during my researches I have also found
> https://lkml.org/lkml/2020/5/7/1575 where there is a patch that
> allows, at least from what it looks like at a first glance, fixed
> files with io_close_prep but seems that the email thread died there.
>
> Shouldn't the close op match the behaviour of the other I/O related
> ops when it comes to fds?
>
> If there aren't specific reasons, happy to look into it and write a patch.
>
>
> Thanks,
> Daniele

Re: [Bug] io_uring_register_files_update broken

[Pavel Begunkov]

On 7/1/21 3:01 PM, Daniele Salvatore Albano wrote:
> On Thu, 1 Jul 2021 at 00:28, Pavel Begunkov <asml.silence@gmail.com> wrote:
>>
>> On 6/30/21 10:14 PM, Victor Stewart wrote:
>>> i'm fairly confident there is something broken with
>>> io_uring_register_files_update,
>>> especially the offset parameter.
>>>
>>> when trying to update a single fd, and getting a successful result of
>>> 1, proceeding
>>> operations with IOSQE_FIXED_FILE fail with -9. but if i update all of
>>> the fds with
>>> then my recv operations succeed, but close still fails with -9.
>>>
>>> on Clear LInux 5.12.13-1050.native
>>
>> Thanks for letting know, I'll take a look
>>

[...]

> I sent an email a while ago to raise a question about a potential bug
> related to close.
> 
> Looks like the close doesn't support registered files (although I saw
> some code within a patch from Jens to fix it while I was
> investigating).

It have never been supported, don't remember it being discussed to
be implemented at any point, and the links below are about
unrelated things.

If you want to close a fixed file, you can use
IORING_OP_FILES_UPDATE with fd=-1 instead. Easy enough to
add it to close, but not sure I see the reason for that considering
existence of IORING_OP_FILES_UPDATE.

fwiw, it doesn't sound related to Victor's report, but still need
to look at it.

> Attached below
> 
> On Fri, 21 May 2021 at 21:28, Daniele Salvatore Albano
> <d.albano@gmail.com> wrote:
>>
>> Hi,
>>
>> Is there any specific reason for which io_close_prep returns EBADF if
>> using REQ_F_FIXED_FILE?
>>
>> I discovered my software was failing to close sockets when using fixed
>> files a while ago but I put it to the side, initially thinking it was
>> a bug I introduced in my code.
>> In recent days I picked it up again and after investigating it, it
>> looks like that, instead, that's the expected behaviour.
>>
>> From what I see, although the behaviour was slightly changed with a
>> couple of commits (ie. with
>> https://github.com/torvalds/linux/commit/cf3040ca55f2085b0a372a620ee2cb93ae19b686
>> ) the io_close_prep have had this behaviour from the very beginning
>> https://github.com/torvalds/linux/commit/b5dba59e0cf7e2cc4d3b3b1ac5fe81ddf21959eb
>> .
>>
>> @Jens during my researches I have also found
>> https://lkml.org/lkml/2020/5/7/1575 where there is a patch that
>> allows, at least from what it looks like at a first glance, fixed
>> files with io_close_prep but seems that the email thread died there.
>>
>> Shouldn't the close op match the behaviour of the other I/O related
>> ops when it comes to fds?
>>
>> If there aren't specific reasons, happy to look into it and write a patch.
>>
>>
>> Thanks,
>> Daniele

-- 
Pavel Begunkov

Re: [Bug] io_uring_register_files_update broken

[Pavel Begunkov]

On 6/30/21 10:14 PM, Victor Stewart wrote:
> i'm fairly confident there is something broken with
> io_uring_register_files_update,
> especially the offset parameter.
> 
> when trying to update a single fd, and getting a successful result of
> 1, proceeding
> operations with IOSQE_FIXED_FILE fail with -9. but if i update all of
> the fds with
> then my recv operations succeed, but close still fails with -9.
> 
> on Clear LInux 5.12.13-1050.native
> 
> here's a diff for liburing send_recv test, to demonstrate this.
> 
> diff --git a/test/send_recv.c b/test/send_recv.c
> index 19adbdd..492b591 100644
> --- a/test/send_recv.c
> +++ b/test/send_recv.c
> @@ -27,6 +27,8 @@ static char str[] = "This is a test of send and recv
> over io_uring!";
>  #      define io_uring_prep_recv io_uring_prep_read
>  #endif
> 
> +static int *fds;
> +
>  static int recv_prep(struct io_uring *ring, struct iovec *iov, int *sock,
>                      int registerfiles)
>  {
> @@ -54,17 +56,28 @@ static int recv_prep(struct io_uring *ring, struct
> iovec *iov, int *sock,
>                 goto err;
>         }
> 
> +       fds = malloc(100 * sizeof(int));
> +       memset(fds, 0xff, sizeof(int) * 100);
> +
>         if (registerfiles) {
> -               ret = io_uring_register_files(ring, &sockfd, 1);
> +               ret = io_uring_register_files(ring, fds, 100);
>                 if (ret) {
>                         fprintf(stderr, "file reg failed\n");
>                         goto err;
>                 }
> -               use_fd = 0;
> -       } else {
> -               use_fd = sockfd;
> +
> +               fds[sockfd] = sockfd;
> +               int result = io_uring_register_files_update(ring,
> sockfd, fds, 1);

s/fds/&fds[sockfd]/

Does it help? io_uring_register_files_update() doesn't
apply offset parameter to the array, it's used only as
an internal index. 

> +
> +               if (result != 1)
> +               {
> +                       fprintf(stderr, "file update failed\n");
> +                       goto err;
> +               }
>         }
> 
> +       use_fd = sockfd;
> +
>         sqe = io_uring_get_sqe(ring);
>         io_uring_prep_recv(sqe, use_fd, iov->iov_base, iov->iov_len, 0);
>         if (registerfiles)
> 

-- 
Pavel Begunkov

Re: [Bug] io_uring_register_files_update broken

[Victor Stewart]

On Thu, Jul 1, 2021 at 3:51 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> On 6/30/21 10:14 PM, Victor Stewart wrote:
> > i'm fairly confident there is something broken with
> > io_uring_register_files_update,
> > especially the offset parameter.
> >
> > when trying to update a single fd, and getting a successful result of
> > 1, proceeding
> > operations with IOSQE_FIXED_FILE fail with -9. but if i update all of
> > the fds with
> > then my recv operations succeed, but close still fails with -9.
> >
> > on Clear LInux 5.12.13-1050.native
> >
> > here's a diff for liburing send_recv test, to demonstrate this.
> >
> > diff --git a/test/send_recv.c b/test/send_recv.c
> > index 19adbdd..492b591 100644
> > --- a/test/send_recv.c
> > +++ b/test/send_recv.c
> > @@ -27,6 +27,8 @@ static char str[] = "This is a test of send and recv
> > over io_uring!";
> >  #      define io_uring_prep_recv io_uring_prep_read
> >  #endif
> >
> > +static int *fds;
> > +
> >  static int recv_prep(struct io_uring *ring, struct iovec *iov, int *sock,
> >                      int registerfiles)
> >  {
> > @@ -54,17 +56,28 @@ static int recv_prep(struct io_uring *ring, struct
> > iovec *iov, int *sock,
> >                 goto err;
> >         }
> >
> > +       fds = malloc(100 * sizeof(int));
> > +       memset(fds, 0xff, sizeof(int) * 100);
> > +
> >         if (registerfiles) {
> > -               ret = io_uring_register_files(ring, &sockfd, 1);
> > +               ret = io_uring_register_files(ring, fds, 100);
> >                 if (ret) {
> >                         fprintf(stderr, "file reg failed\n");
> >                         goto err;
> >                 }
> > -               use_fd = 0;
> > -       } else {
> > -               use_fd = sockfd;
> > +
> > +               fds[sockfd] = sockfd;
> > +               int result = io_uring_register_files_update(ring,
> > sockfd, fds, 1);
>
> s/fds/&fds[sockfd]/
>
> Does it help? io_uring_register_files_update() doesn't
> apply offset parameter to the array, it's used only as
> an internal index.

i see yes, it works it like this!

io_uring_register_files_update(&ring, fd, &(socketfds[fd]), 1);
io_uring_register_files_update(&ring, fd, &(socketfds[fd] = -1), 1);

and this behavior is clear upon a closer reading of...
https://github.com/axboe/liburing/blob/11f6d56302c177a96d7eb1df86995939a4feb736/test/file-register.c#L80

i guess it's sometimes ambiguous whether int* is requesting an array
or an actual pointer to a single int.

all good now.

>
> > +
> > +               if (result != 1)
> > +               {
> > +                       fprintf(stderr, "file update failed\n");
> > +                       goto err;
> > +               }
> >         }
> >
> > +       use_fd = sockfd;
> > +
> >         sqe = io_uring_get_sqe(ring);
> >         io_uring_prep_recv(sqe, use_fd, iov->iov_base, iov->iov_len, 0);
> >         if (registerfiles)
> >
>
> --
> Pavel Begunkov

Re: [Bug] io_uring_register_files_update broken

[Pavel Begunkov]

On 7/1/21 4:46 PM, Victor Stewart wrote:
> On Thu, Jul 1, 2021 at 3:51 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
[...]
>>> sockfd, fds, 1);
>>
>> s/fds/&fds[sockfd]/
>>
>> Does it help? io_uring_register_files_update() doesn't
>> apply offset parameter to the array, it's used only as
>> an internal index.
> 
> i see yes, it works it like this!
> 
> io_uring_register_files_update(&ring, fd, &(socketfds[fd]), 1);
> io_uring_register_files_update(&ring, fd, &(socketfds[fd] = -1), 1);
> 
> and this behavior is clear upon a closer reading of...
> https://github.com/axboe/liburing/blob/11f6d56302c177a96d7eb1df86995939a4feb736/test/file-register.c#L80
> 
> i guess it's sometimes ambiguous whether int* is requesting an array
> or an actual pointer to a single int.

It's an array, just element 0 is registered as a registered
file with index @off, element 1 as a reg-file with index
@(off+1) and so on.

> all good now.
> 
>>
>>> +
>>> +               if (result != 1)
>>> +               {
>>> +                       fprintf(stderr, "file update failed\n");
>>> +                       goto err;
>>> +               }
>>>         }
>>>
>>> +       use_fd = sockfd;
>>> +
>>>         sqe = io_uring_get_sqe(ring);
>>>         io_uring_prep_recv(sqe, use_fd, iov->iov_base, iov->iov_len, 0);
>>>         if (registerfiles)
>>>
>>
>> --
>> Pavel Begunkov

-- 
Pavel Begunkov