Re: io_uring file descriptor address already in use error

[Jens Axboe <axboe@kernel.dk>]

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