io_uring file descriptor address already in use error

io_uring file descriptor address already in use error

[Josef]

Hi,

I found a bug submitting a server socket poll in io_uring. The file
descriptor is not really closed when calling close(2), if I bind a new
socket with the same address & port I'll get an "Already in use" error
message

example to reproduce it
https://gist.github.com/1Jo1/3ace601884b86f7495fd5241190494dc

tested on 5.9-rc2

---
Josef

Re: io_uring file descriptor address already in use error

[Jens Axboe]

On 8/25/20 9:00 AM, Josef wrote:
> Hi,
> 
> I found a bug submitting a server socket poll in io_uring. The file
> descriptor is not really closed when calling close(2), if I bind a new
> socket with the same address & port I'll get an "Already in use" error
> message
> 
> example to reproduce it
> https://gist.github.com/1Jo1/3ace601884b86f7495fd5241190494dc

Not sure this is an actual bug, but depends on how you look at it. Your
poll command has a reference to the file, which means that when you close
it here:

    assert(close(sock_listen_fd1) == 0); 

then that's not the final close. If you move the io_uring_queue_exit()
before that last create_server_socket() it should work, since the poll
will have been canceled (and hence the file closed) at that point.

That said, I don't believe we actually need the file after arming the
poll, so we could potentially close it once we've armed it. That would
make your example work.

-- 
Jens Axboe

Re: io_uring file descriptor address already in use error

[Jens Axboe]

On 8/25/20 9:12 AM, Jens Axboe wrote:
> On 8/25/20 9:00 AM, Josef wrote:
>> Hi,
>>
>> I found a bug submitting a server socket poll in io_uring. The file
>> descriptor is not really closed when calling close(2), if I bind a new
>> socket with the same address & port I'll get an "Already in use" error
>> message
>>
>> example to reproduce it
>> https://gist.github.com/1Jo1/3ace601884b86f7495fd5241190494dc
> 
> Not sure this is an actual bug, but depends on how you look at it. Your
> poll command has a reference to the file, which means that when you close
> it here:
> 
>     assert(close(sock_listen_fd1) == 0); 
> 
> then that's not the final close. If you move the io_uring_queue_exit()
> before that last create_server_socket() it should work, since the poll
> will have been canceled (and hence the file closed) at that point.
> 
> That said, I don't believe we actually need the file after arming the
> poll, so we could potentially close it once we've armed it. That would
> make your example work.

Actually we do need the file, in case we're re-arming poll. But as stated
in the above email, this isn't unexpected behavior. You could cancel the
poll before trying to setup the new server socket, that'd close it as
well. Then the close() would actually close it. Ordering of the two
operations wouldn't matter.

-- 
Jens Axboe

Re: io_uring file descriptor address already in use error

[Jens Axboe]

On 8/25/20 9:17 AM, Jens Axboe wrote:
> On 8/25/20 9:12 AM, Jens Axboe wrote:
>> On 8/25/20 9:00 AM, Josef wrote:
>>> Hi,
>>>
>>> I found a bug submitting a server socket poll in io_uring. The file
>>> descriptor is not really closed when calling close(2), if I bind a new
>>> socket with the same address & port I'll get an "Already in use" error
>>> message
>>>
>>> example to reproduce it
>>> https://gist.github.com/1Jo1/3ace601884b86f7495fd5241190494dc
>>
>> Not sure this is an actual bug, but depends on how you look at it. Your
>> poll command has a reference to the file, which means that when you close
>> it here:
>>
>>     assert(close(sock_listen_fd1) == 0); 
>>
>> then that's not the final close. If you move the io_uring_queue_exit()
>> before that last create_server_socket() it should work, since the poll
>> will have been canceled (and hence the file closed) at that point.
>>
>> That said, I don't believe we actually need the file after arming the
>> poll, so we could potentially close it once we've armed it. That would
>> make your example work.
> 
> Actually we do need the file, in case we're re-arming poll. But as stated
> in the above email, this isn't unexpected behavior. You could cancel the
> poll before trying to setup the new server socket, that'd close it as
> well. Then the close() would actually close it. Ordering of the two
> operations wouldn't matter.

Just to wrap this one up, the below patch would make it behave like you
expect, and still retain the re-poll behavior we use on poll armed on
behalf of an IO request. At this point we're not holding a reference to
the file across the poll handler, and your close() would actually close
the file since it's putting the last reference to it.

But... Not actually sure this is warranted. Any io_uring request that
operates on a file will hold a reference to it until it completes. The
poll request in your example never completes. If you run poll(2) on a
file and you close that file, you won't get a poll event triggered.
It'll just sit there and wait on events that won't come in. poll(2)
doesn't hold a reference to the file once it's armed the handler, so
your example would work there.

What do you think?


diff --git a/fs/io_uring.c b/fs/io_uring.c
index 384df86dfc69..e3de6846d91a 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4617,7 +4617,7 @@ static bool io_poll_rewait(struct io_kiocb *req, struct io_poll_iocb *poll)
 {
 	struct io_ring_ctx *ctx = req->ctx;
 
-	if (!req->result && !READ_ONCE(poll->canceled)) {
+	if (!req->result && req->file && !READ_ONCE(poll->canceled)) {
 		struct poll_table_struct pt = { ._key = poll->events };
 
 		req->result = vfs_poll(req->file, &pt) & poll->events;
@@ -4845,10 +4845,11 @@ static void io_poll_req_insert(struct io_kiocb *req)
 static __poll_t __io_arm_poll_handler(struct io_kiocb *req,
 				      struct io_poll_iocb *poll,
 				      struct io_poll_table *ipt, __poll_t mask,
-				      wait_queue_func_t wake_func)
+				      wait_queue_func_t wake_func, bool hold)
 	__acquires(&ctx->completion_lock)
 {
 	struct io_ring_ctx *ctx = req->ctx;
+	struct file *file = req->file;
 	bool cancel = false;
 
 	io_init_poll_iocb(poll, mask, wake_func);
@@ -4859,7 +4860,13 @@ static __poll_t __io_arm_poll_handler(struct io_kiocb *req,
 	ipt->req = req;
 	ipt->error = -EINVAL;
 
-	mask = vfs_poll(req->file, &ipt->pt) & poll->events;
+	if (!hold)
+		req->file = poll->file = NULL;
+
+	mask = vfs_poll(file, &ipt->pt) & poll->events;
+
+	if (!hold)
+		io_put_file(req, file, req->flags & REQ_F_FIXED_FILE);
 
 	spin_lock_irq(&ctx->completion_lock);
 	if (likely(poll->head)) {
@@ -4917,7 +4924,7 @@ static bool io_arm_poll_handler(struct io_kiocb *req)
 	ipt.pt._qproc = io_async_queue_proc;
 
 	ret = __io_arm_poll_handler(req, &apoll->poll, &ipt, mask,
-					io_async_wake);
+					io_async_wake, true);
 	if (ret || ipt.error) {
 		io_poll_remove_double(req);
 		spin_unlock_irq(&ctx->completion_lock);
@@ -5100,7 +5107,7 @@ static int io_poll_add(struct io_kiocb *req)
 	ipt.pt._qproc = io_poll_queue_proc;
 
 	mask = __io_arm_poll_handler(req, &req->poll, &ipt, poll->events,
-					io_poll_wake);
+					io_poll_wake, false);
 
 	if (mask) { /* no async, we'd stolen it */
 		ipt.error = 0;

-- 
Jens Axboe

Re: io_uring file descriptor address already in use error

[Josef]

> Not sure this is an actual bug, but depends on how you look at it. Your
> poll command has a reference to the file, which means that when you close
> it here:
>
>     assert(close(sock_listen_fd1) == 0);
>
> then that's not the final close. If you move the io_uring_queue_exit()
> before that last create_server_socket() it should work, since the poll
> will have been canceled (and hence the file closed) at that point.
>
>  That said, I don't believe we actually need the file after arming the
>  poll, so we could potentially close it once we've armed it. That would
>  make your example work.


ah okay that makes sense

> Actually we do need the file, in case we're re-arming poll. But as stated
> in the above email, this isn't unexpected behavior. You could cancel the
> poll before trying to setup the new server socket, that'd close it as
> well. Then the close() would actually close it. Ordering of the two
> operations wouldn't matter.
>
> Just to wrap this one up, the below patch would make it behave like you
> expect, and still retain the re-poll behavior we use on poll armed on
> behalf of an IO request. At this point we're not holding a reference to
> the file across the poll handler, and your close() would actually close
> the file since it's putting the last reference to it.
>
> But... Not actually sure this is warranted. Any io_uring request that
> operates on a file will hold a reference to it until it completes. The
> poll request in your example never completes. If you run poll(2) on a
> file and you close that file, you won't get a poll event triggered.
> It'll just sit there and wait on events that won't come in. poll(2)
> doesn't hold a reference to the file once it's armed the handler, so
> your example would work there.

oh thanks I'm gonna test that :) yeah I expected exactly the same
behaviour as in epoll(2) & pol(2) that's why I'm asking
to be honest it would be quite handy to have this patch(for netty), so
I don't have to cancel a poll or close ring file descriptor(I do of
course understand that if you won't push this patch)

is there no other way around to close the file descriptor? Even if I
remove the poll, it doesn't work
btw if understood correctly poll remove operation refers to all file
descriptors which arming a poll in the ring buffer right?
Is there a way to cancel a specific file descriptor poll?


---
Josef Grieb

Re: io_uring file descriptor address already in use error

[Jens Axboe]

On 8/25/20 10:38 AM, Josef wrote:
>> Not sure this is an actual bug, but depends on how you look at it. Your
>> poll command has a reference to the file, which means that when you close
>> it here:
>>
>>     assert(close(sock_listen_fd1) == 0);
>>
>> then that's not the final close. If you move the io_uring_queue_exit()
>> before that last create_server_socket() it should work, since the poll
>> will have been canceled (and hence the file closed) at that point.
>>
>>  That said, I don't believe we actually need the file after arming the
>>  poll, so we could potentially close it once we've armed it. That would
>>  make your example work.
> 
> 
> ah okay that makes sense
> 
>> Actually we do need the file, in case we're re-arming poll. But as stated
>> in the above email, this isn't unexpected behavior. You could cancel the
>> poll before trying to setup the new server socket, that'd close it as
>> well. Then the close() would actually close it. Ordering of the two
>> operations wouldn't matter.
>>
>> Just to wrap this one up, the below patch would make it behave like you
>> expect, and still retain the re-poll behavior we use on poll armed on
>> behalf of an IO request. At this point we're not holding a reference to
>> the file across the poll handler, and your close() would actually close
>> the file since it's putting the last reference to it.
>>
>> But... Not actually sure this is warranted. Any io_uring request that
>> operates on a file will hold a reference to it until it completes. The
>> poll request in your example never completes. If you run poll(2) on a
>> file and you close that file, you won't get a poll event triggered.
>> It'll just sit there and wait on events that won't come in. poll(2)
>> doesn't hold a reference to the file once it's armed the handler, so
>> your example would work there.
> 
> oh thanks I'm gonna test that :) yeah I expected exactly the same
> behaviour as in epoll(2) & pol(2) that's why I'm asking
> to be honest it would be quite handy to have this patch(for netty), so
> I don't have to cancel a poll or close ring file descriptor(I do of
> course understand that if you won't push this patch)

In order for the patch to be able to move ahead, we'd need to be able
to control this behavior. Right now we rely on the file being there if
we need to repoll, see:

commit a6ba632d2c249a4390289727c07b8b55eb02a41d
Author: Jens Axboe <axboe@kernel.dk>
Date:   Fri Apr 3 11:10:14 2020 -0600

    io_uring: retry poll if we got woken with non-matching mask

If this never happened, we would not need the file at all and we could
make it the default behavior. But don't think that's solvable.

> is there no other way around to close the file descriptor? Even if I
> remove the poll, it doesn't work

If you remove the poll it should definitely work, as nobody is holding a
reference to it as you have nothing else in flight. Can you clarify what
you mean here?

I don't think there's another way, outside of having a poll (io_uring
or poll(2), doesn't matter, the behavior is the same) being triggered in
error. That doesn't happen, as mentioned if you do epoll/poll on a file
and you close it, it won't trigger an event.

> btw if understood correctly poll remove operation refers to all file
> descriptors which arming a poll in the ring buffer right?
> Is there a way to cancel a specific file descriptor poll?

You can cancel specific requests by identifying them with their
->user_data. You can cancel a poll either with POLL_REMOVE or
ASYNC_CANCEL, either one will find it. So as long as you have that, and
it's unique, it'll only cancel that one specific request.

-- 
Jens Axboe

Re: io_uring file descriptor address already in use error

[Josef]

> In order for the patch to be able to move ahead, we'd need to be able
> to control this behavior. Right now we rely on the file being there if
> we need to repoll, see:
>
> commit a6ba632d2c249a4390289727c07b8b55eb02a41d
> Author: Jens Axboe <axboe@kernel.dk>
> Date:   Fri Apr 3 11:10:14 2020 -0600
>
>     io_uring: retry poll if we got woken with non-matching mask
>
> If this never happened, we would not need the file at all and we could
> make it the default behavior. But don't think that's solvable.
>
> > is there no other way around to close the file descriptor? Even if I
> > remove the poll, it doesn't work
>
> If you remove the poll it should definitely work, as nobody is holding a
> reference to it as you have nothing else in flight. Can you clarify what
> you mean here?
>
> I don't think there's another way, outside of having a poll (io_uring
> or poll(2), doesn't matter, the behavior is the same) being triggered in
> error. That doesn't happen, as mentioned if you do epoll/poll on a file
> and you close it, it won't trigger an event.
>
> > btw if understood correctly poll remove operation refers to all file
> > descriptors which arming a poll in the ring buffer right?
> > Is there a way to cancel a specific file descriptor poll?
>
> You can cancel specific requests by identifying them with their
> ->user_data. You can cancel a poll either with POLL_REMOVE or
> ASYNC_CANCEL, either one will find it. So as long as you have that, and
> it's unique, it'll only cancel that one specific request.

thanks it works, my bad, I was not aware that user_data is associated
with the poll request user_data...just need to remove my server socket
poll which binds to an address so I think this patch is not really
necessary

btw IORING_FEAT_FAST_POLL feature which arming poll for read events,
how does it work when the file descriptor(not readable yet) wants to
read(non blocking) something and I close(2) the file descriptor? I'm
guessing io_uring doesn't hold any reference to it anymore right?


---
Josef Grieb

Re: io_uring file descriptor address already in use error

[Jens Axboe]

On 8/25/20 9:01 PM, Josef wrote:
>> In order for the patch to be able to move ahead, we'd need to be able
>> to control this behavior. Right now we rely on the file being there if
>> we need to repoll, see:
>>
>> commit a6ba632d2c249a4390289727c07b8b55eb02a41d
>> Author: Jens Axboe <axboe@kernel.dk>
>> Date:   Fri Apr 3 11:10:14 2020 -0600
>>
>>     io_uring: retry poll if we got woken with non-matching mask
>>
>> If this never happened, we would not need the file at all and we could
>> make it the default behavior. But don't think that's solvable.
>>
>>> is there no other way around to close the file descriptor? Even if I
>>> remove the poll, it doesn't work
>>
>> If you remove the poll it should definitely work, as nobody is holding a
>> reference to it as you have nothing else in flight. Can you clarify what
>> you mean here?
>>
>> I don't think there's another way, outside of having a poll (io_uring
>> or poll(2), doesn't matter, the behavior is the same) being triggered in
>> error. That doesn't happen, as mentioned if you do epoll/poll on a file
>> and you close it, it won't trigger an event.
>>
>>> btw if understood correctly poll remove operation refers to all file
>>> descriptors which arming a poll in the ring buffer right?
>>> Is there a way to cancel a specific file descriptor poll?
>>
>> You can cancel specific requests by identifying them with their
>> ->user_data. You can cancel a poll either with POLL_REMOVE or
>> ASYNC_CANCEL, either one will find it. So as long as you have that, and
>> it's unique, it'll only cancel that one specific request.
> 
> thanks it works, my bad, I was not aware that user_data is associated
> with the poll request user_data...just need to remove my server socket
> poll which binds to an address so I think this patch is not really
> necessary
> 
> btw IORING_FEAT_FAST_POLL feature which arming poll for read events,
> how does it work when the file descriptor(not readable yet) wants to
> read(non blocking) something and I close(2) the file descriptor? I'm
> guessing io_uring doesn't hold any reference to it anymore right?

Most file types will *not* notify you through poll if they get closed,
so it'll just sit there until canceled. This is the same with poll(2) or
epoll(2). io_uring will continue to hold a reference to the file, it
does that over request completion for any request that uses a file.

-- 
Jens Axboe

Re: io_uring file descriptor address already in use error

[Josef]

On Wed, 26 Aug 2020 at 15:44, Jens Axboe <axboe@kernel.dk> wrote:
>
> On 8/25/20 9:01 PM, Josef wrote:
> >> In order for the patch to be able to move ahead, we'd need to be able
> >> to control this behavior. Right now we rely on the file being there if
> >> we need to repoll, see:
> >>
> >> commit a6ba632d2c249a4390289727c07b8b55eb02a41d
> >> Author: Jens Axboe <axboe@kernel.dk>
> >> Date:   Fri Apr 3 11:10:14 2020 -0600
> >>
> >>     io_uring: retry poll if we got woken with non-matching mask
> >>
> >> If this never happened, we would not need the file at all and we could
> >> make it the default behavior. But don't think that's solvable.
> >>
> >>> is there no other way around to close the file descriptor? Even if I
> >>> remove the poll, it doesn't work
> >>
> >> If you remove the poll it should definitely work, as nobody is holding a
> >> reference to it as you have nothing else in flight. Can you clarify what
> >> you mean here?
> >>
> >> I don't think there's another way, outside of having a poll (io_uring
> >> or poll(2), doesn't matter, the behavior is the same) being triggered in
> >> error. That doesn't happen, as mentioned if you do epoll/poll on a file
> >> and you close it, it won't trigger an event.
> >>
> >>> btw if understood correctly poll remove operation refers to all file
> >>> descriptors which arming a poll in the ring buffer right?
> >>> Is there a way to cancel a specific file descriptor poll?
> >>
> >> You can cancel specific requests by identifying them with their
> >> ->user_data. You can cancel a poll either with POLL_REMOVE or
> >> ASYNC_CANCEL, either one will find it. So as long as you have that, and
> >> it's unique, it'll only cancel that one specific request.
> >
> > thanks it works, my bad, I was not aware that user_data is associated
> > with the poll request user_data...just need to remove my server socket
> > poll which binds to an address so I think this patch is not really
> > necessary
> >
> > btw IORING_FEAT_FAST_POLL feature which arming poll for read events,
> > how does it work when the file descriptor(not readable yet) wants to
> > read(non blocking) something and I close(2) the file descriptor? I'm
> > guessing io_uring doesn't hold any reference to it anymore right?
>
> Most file types will *not* notify you through poll if they get closed,
> so it'll just sit there until canceled. This is the same with poll(2) or
> epoll(2). io_uring will continue to hold a reference to the file, it
> does that over request completion for any request that uses a file.


okay, btw according to the man page IORING_OP_POLL_REMOVE it's unclear
to me what value user_data contains in the cqe
I'm guessing user_data is always 0 right? just tested it with liburing
I got always 0(user_data) even if there is no polling request


---
Josef Grieb

Re: io_uring file descriptor address already in use error

[Jens Axboe]

> On Aug 26, 2020, at 12:26 PM, Josef <josef.grieb@gmail.com> wrote:
> 
> On Wed, 26 Aug 2020 at 15:44, Jens Axboe <axboe@kernel.dk> wrote:
>> 
>> On 8/25/20 9:01 PM, Josef wrote:
>>>> In order for the patch to be able to move ahead, we'd need to be able
>>>> to control this behavior. Right now we rely on the file being there if
>>>> we need to repoll, see:
>>>> 
>>>> commit a6ba632d2c249a4390289727c07b8b55eb02a41d
>>>> Author: Jens Axboe <axboe@kernel.dk>
>>>> Date:   Fri Apr 3 11:10:14 2020 -0600
>>>> 
>>>>    io_uring: retry poll if we got woken with non-matching mask
>>>> 
>>>> If this never happened, we would not need the file at all and we could
>>>> make it the default behavior. But don't think that's solvable.
>>>> 
>>>>> is there no other way around to close the file descriptor? Even if I
>>>>> remove the poll, it doesn't work
>>>> 
>>>> If you remove the poll it should definitely work, as nobody is holding a
>>>> reference to it as you have nothing else in flight. Can you clarify what
>>>> you mean here?
>>>> 
>>>> I don't think there's another way, outside of having a poll (io_uring
>>>> or poll(2), doesn't matter, the behavior is the same) being triggered in
>>>> error. That doesn't happen, as mentioned if you do epoll/poll on a file
>>>> and you close it, it won't trigger an event.
>>>> 
>>>>> btw if understood correctly poll remove operation refers to all file
>>>>> descriptors which arming a poll in the ring buffer right?
>>>>> Is there a way to cancel a specific file descriptor poll?
>>>> 
>>>> You can cancel specific requests by identifying them with their
>>>> ->user_data. You can cancel a poll either with POLL_REMOVE or
>>>> ASYNC_CANCEL, either one will find it. So as long as you have that, and
>>>> it's unique, it'll only cancel that one specific request.
>>> 
>>> thanks it works, my bad, I was not aware that user_data is associated
>>> with the poll request user_data...just need to remove my server socket
>>> poll which binds to an address so I think this patch is not really
>>> necessary
>>> 
>>> btw IORING_FEAT_FAST_POLL feature which arming poll for read events,
>>> how does it work when the file descriptor(not readable yet) wants to
>>> read(non blocking) something and I close(2) the file descriptor? I'm
>>> guessing io_uring doesn't hold any reference to it anymore right?
>> 
>> Most file types will *not* notify you through poll if they get closed,
>> so it'll just sit there until canceled. This is the same with poll(2) or
>> epoll(2). io_uring will continue to hold a reference to the file, it
>> does that over request completion for any request that uses a file.
> 
> 
> okay, btw according to the man page IORING_OP_POLL_REMOVE it's unclear
> to me what value user_data contains in the cqe
> I'm guessing user_data is always 0 right? just tested it with liburing
> I got always 0(user_data) even if there is no polling request

The user_data is *always* passed straight from the sqe, so it is set to whatever you put in there. That’s a key component of the protocol, allowing you to tie a completion event back to your submission. And also allows you to do a directed cancel event for a given pending request.