aio: block io_destroy() until all context requests are completed

PATCH In review Benjamin LaHaise 3 participants 5 messages May 1, 2014 – May 2, 2014 1 message not in the archive

Subsystems (2)

VFS Aio

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Benjamin LaHaise

author maintainer

2014-05-01 13:06 UTC #1 lore raw

PATCH 1/2 aio: block io_destroy() until all context requests are completed

deletes aio context and all resources related to. It makes sense that no IO operations connected to the context should be running after the context is destroyed. As we removed io_context we have no chance to
get requests status or call io_getevents().

man page for io_destroy says that this function may block until all context's requests are completed. Before kernel 3.11 io_destroy() blocked indeed, but since aio refactoring in 3.11 it is not true anymore.

Here is a pseudo-code that shows a testcase for a race condition discovered in 3.11:

initialize io_context
io_submit(read to buffer)
io_destroy()

// context is destroyed so we can free the resources free(buffers);

// if the buffer is allocated by some other user he'll be surprised // to learn that the buffer still filled by an outstanding operation // from the destroyed io_context

The fix is straight-forward - add a completion struct and wait on it in io_destroy, complete() should be called when number of in-fligh requests reaches zero.

If two or more io_destroy() called for the same context simultaneously then only the first one waits for IO completion, other calls behaviour is undefined.

Tested: ran http://pastebin.com/LrPsQ4RL testcase for several hours and
do not see the race condition anymore.

Signed-off-by Anatol Pomozov <[email protected]>

Signed-off-by Benjamin LaHaise <[email protected]>

---
 fs/aio.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

fs/aio.c +32 -4

index 12a3de0e..2adbb03 100644
@@ -112,6 +112,11 @@ struct kioctx {
 
 	struct work_struct	free_work;
 
+	/*
+	 * signals when all in-flight requests are done
+	 */
+	struct completion *requests_done;
+
 	struct {
 		/*
 		 * This counts the number of available slots in the ringbuffer,

@@ -508,6 +513,10 @@ static void free_ioctx_reqs(struct percpu_ref *ref)
 {
 	struct kioctx *ctx = container_of(ref, struct kioctx, reqs);
 
+	/* At this point we know that there are no any in-flight requests */
+	if (ctx->requests_done)
+		complete(ctx->requests_done);
+
 	INIT_WORK(&ctx->free_work, free_ioctx);
 	schedule_work(&ctx->free_work);
 }

@@ -718,7 +727,8 @@ err:
  *	when the processes owning a context have all exited to encourage
  *	the rapid destruction of the kioctx.
  */
-static void kill_ioctx(struct mm_struct *mm, struct kioctx *ctx)
+static void kill_ioctx(struct mm_struct *mm, struct kioctx *ctx,
+		struct completion *requests_done)
 {
 	if (!atomic_xchg(&ctx->dead, 1)) {
 		struct kioctx_table *table;

@@ -747,7 +757,11 @@ static void kill_ioctx(struct mm_struct *mm, struct kioctx *ctx)
 		if (ctx->mmap_size)
 			vm_munmap(ctx->mmap_base, ctx->mmap_size);
 
+		ctx->requests_done = requests_done;
 		percpu_ref_kill(&ctx->users);
+	} else {
+		if (requests_done)
+			complete(requests_done);
 	}
 }

@@ -809,7 +823,7 @@ void exit_aio(struct mm_struct *mm)
 		 */
 		ctx->mmap_size = 0;
 
-		kill_ioctx(mm, ctx);
+		kill_ioctx(mm, ctx, NULL);
 	}
 }

@@ -1185,7 +1199,7 @@ SYSCALL_DEFINE2(io_setup, unsigned, nr_events, aio_context_t __user *, ctxp)
 	if (!IS_ERR(ioctx)) {
 		ret = put_user(ioctx->user_id, ctxp);
 		if (ret)
-			kill_ioctx(current->mm, ioctx);
+			kill_ioctx(current->mm, ioctx, NULL);
 		percpu_ref_put(&ioctx->users);
 	}

@@ -1203,8 +1217,22 @@ SYSCALL_DEFINE1(io_destroy, aio_context_t, ctx)
 {
 	struct kioctx *ioctx = lookup_ioctx(ctx);
 	if (likely(NULL != ioctx)) {
-		kill_ioctx(current->mm, ioctx);
+		struct completion requests_done =
+			COMPLETION_INITIALIZER_ONSTACK(requests_done);
+
+		/* Pass requests_done to kill_ioctx() where it can be set
+		 * in a thread-safe way. If we try to set it here then we have
+		 * a race condition if two io_destroy() called simultaneously.
+		 */
+		kill_ioctx(current->mm, ioctx, &requests_done);
 		percpu_ref_put(&ioctx->users);
+
+		/* Wait until all IO for the context are done. Otherwise kernel
+		 * keep using user-space buffers even if user thinks the context
+		 * is destroyed.
+		 */
+		wait_for_completion(&requests_done);
+
 		return 0;
 	}
 	pr_debug("EINVAL: io_destroy: invalid context id\n");

Signature · 11 lines

1.8.2.1


-- 
"Thought is the essence of where you are now."

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to [email protected].  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"[email protected]">[email protected]</a>

Benjamin LaHaise

author maintainer

2014-05-01 13:07 UTC #2 lore raw

PATCH 2/2 aio: fix potential leak in aio_run_iocb().

In reply to a message not in the archive

iovec should be reclaimed whenever caller of rw_copy_check_uvector() returns, but it doesn't hold when failure happens right after aio_setup_vectored_rw().

Fix that in a such way to avoid hairy goto.

Signed-off-by Leon Yu <[email protected]>

Signed-off-by Benjamin LaHaise <[email protected]>

Cc [email protected]

---
 fs/aio.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

fs/aio.c +2 -4

index 2adbb03..a0ed6c7 100644
@@ -1327,10 +1327,8 @@ rw_common:
 						&iovec, compat)
 			: aio_setup_single_vector(req, rw, buf, &nr_segs,
 						  iovec);
-		if (ret)
-			return ret;
-
-		ret = rw_verify_area(rw, file, &req->ki_pos, req->ki_nbytes);
+		if (!ret)
+			ret = rw_verify_area(rw, file, &req->ki_pos, req->ki_nbytes);
 		if (ret < 0) {
 			if (iovec != &inline_vec)
 				kfree(iovec);

Signature · 11 lines

1.8.2.1


-- 
"Thought is the essence of where you are now."

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to [email protected].  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"[email protected]">[email protected]</a>

Lukáš Czerner

2014-05-02 08:56 UTC #3 lore raw

Re: [PATCH 2/2] aio: fix potential leak in aio_run_iocb().

Quoted 11 lines · Benjamin LaHaise

Date: Thu, 1 May 2014 09:07:09 -0400
From: Benjamin LaHaise <[email protected]>
To: [email protected]
Cc: [email protected], [email protected],
[email protected], Leon Yu <[email protected]>
Subject: [PATCH 2/2] aio: fix potential leak in aio_run_iocb().
iovec should be reclaimed whenever caller of rw_copy_check_uvector() returns, but it doesn't hold when failure happens right after aio_setup_vectored_rw().
Fix that in a such way to avoid hairy goto.

As I already replied to Leon,

this does not seem right.

Quoted 13 lines

Signed-off-by Leon Yu <[email protected]>
Signed-off-by Benjamin LaHaise <[email protected]>
Cc [email protected]
---
 fs/aio.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
fs/aio.c +0 -0
index 2adbb03..a0ed6c7 100644

fs/aio.c rw_common: · line 1333

Quoted 7 lines

fs/aio.c +0 -4

@@ -1327,10 +1327,8 @@ rw_common:
 						&iovec, compat)
 			: aio_setup_single_vector(req, rw, buf, &nr_segs,
 						  iovec);
-		if (ret)
-			return ret;
-
-		ret = rw_verify_area(rw, file, &req->ki_pos, req->ki_nbytes);

here ret could be possibly set to a positive number.

fs/aio.c rw_common: · line 1332

Quoted 3 lines

fs/aio.c +2 -0

+		if (!ret)
+			ret = rw_verify_area(rw, file, &req->ki_pos, req->ki_nbytes);
 		if (ret < 0) {

but here we're checking for negative and bail out. However this changes the way it worked before this patch and the iovec would not be reclaimed anyway as you mentioned in the commit description.

Thanks!
-Lukas

fs/aio.c rw_common: · line 1334

Quoted 3 lines

fs/aio.c +0 -0

 			if (iovec != &inline_vec)
 				kfree(iovec);

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in the body to [email protected]. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"[email protected]">[email protected]</a>

Mateusz Guzik

2014-05-02 11:53 UTC #4 lore raw

Re: [PATCH 2/2] aio: fix potential leak in aio_run_iocb().

Quoted 26 lines · Lukáš Czerner

Quoted 11 lines · Benjamin LaHaise
Date: Thu, 1 May 2014 09:07:09 -0400
From: Benjamin LaHaise <[email protected]>
To: [email protected]
Cc: [email protected], [email protected],
[email protected], Leon Yu <[email protected]>
Subject: [PATCH 2/2] aio: fix potential leak in aio_run_iocb().
iovec should be reclaimed whenever caller of rw_copy_check_uvector() returns, but it doesn't hold when failure happens right after aio_setup_vectored_rw().
Fix that in a such way to avoid hairy goto.
As I already replied to Leon,
this does not seem right.
Quoted 8 lines
Signed-off-by Leon Yu <[email protected]>
Signed-off-by Benjamin LaHaise <[email protected]>
Cc [email protected]
---
 fs/aio.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Quoted 15 lines

fs/aio.c rw_common: · line 1333

Quoted 12 lines

fs/aio.c +0 -4

index 2adbb03..a0ed6c7 100644
@@ -1327,10 +1327,8 @@ rw_common:
 						&iovec, compat)
 			: aio_setup_single_vector(req, rw, buf, &nr_segs,
 						  iovec);
-		if (ret)
-			return ret;
-
-		ret = rw_verify_area(rw, file, &req->ki_pos, req->ki_nbytes);

here ret could be possibly set to a positive number.

How?

                ret = (opcode == IOCB_CMD_PREADV ||
                       opcode == IOCB_CMD_PWRITEV)
                        ? aio_setup_vectored_rw(req, rw, buf, &nr_segs,
                                                &iovec, compat)
                        : aio_setup_single_vector(req, rw, buf, &nr_segs,
                                                  iovec);

Where aio_setup_vectored_rw:

        if (ret < 0)
                return ret;
[..]
	return 0;

and aio_setup_single_vector:

        if (unlikely(!access_ok(!rw, buf, kiocb->ki_nbytes)))
                return -EFAULT;
[..]
	        return 0;

Both functions are returning ssize_t, thus it's either 0 on success or negative on failure.

"if (ret)" replaced by "if (ret < 0)" should indeed set off alarm bells, but turns it turns out to be fine here.

Quoted 4 lines

fs/aio.c rw_common: · line 1332
Quoted 3 lines
fs/aio.c +2 -0
+		if (!ret)
+			ret = rw_verify_area(rw, file, &req->ki_pos, req->ki_nbytes);
 		if (ret < 0) {

So this check is fine and cleanup will be called.

However, there is a yet to be merged patch which fixes actual problem which is weird rw_copy_check_uvector semantics:
https://lkml.org/lkml/2014/4/25/778

rendering this patch unnecessary

Signature · 7 lines

Mateusz Guzik

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to [email protected].  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"[email protected]">[email protected]</a>

Lukáš Czerner

2014-05-02 12:16 UTC #5 lore raw

Re: [PATCH 2/2] aio: fix potential leak in aio_run_iocb().

Quoted 60 lines · Mateusz Guzik

Date: Fri, 2 May 2014 13:53:11 +0200
From: Mateusz Guzik <[email protected]>
To: Lukáš Czerner <[email protected]>
Cc: Benjamin LaHaise <[email protected]>, [email protected],
[email protected], [email protected], [email protected], Leon Yu <[email protected]>
Subject: Re: [PATCH 2/2] aio: fix potential leak in aio_run_iocb().
Quoted 26 lines · Lukáš Czerner
Quoted 11 lines · Benjamin LaHaise
Date: Thu, 1 May 2014 09:07:09 -0400
From: Benjamin LaHaise <[email protected]>
To: [email protected]
Cc: [email protected], [email protected],
[email protected], Leon Yu <[email protected]>
Subject: [PATCH 2/2] aio: fix potential leak in aio_run_iocb().
iovec should be reclaimed whenever caller of rw_copy_check_uvector() returns, but it doesn't hold when failure happens right after aio_setup_vectored_rw().
Fix that in a such way to avoid hairy goto.
As I already replied to Leon,
this does not seem right.
Quoted 8 lines
Signed-off-by Leon Yu <[email protected]>
Signed-off-by Benjamin LaHaise <[email protected]>
Cc [email protected]
---
 fs/aio.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
Quoted 15 lines
fs/aio.c rw_common: · line 1333
Quoted 12 lines
fs/aio.c +0 -4
index 2adbb03..a0ed6c7 100644
@@ -1327,10 +1327,8 @@ rw_common:
 						&iovec, compat)
 			: aio_setup_single_vector(req, rw, buf, &nr_segs,
 						  iovec);
-		if (ret)
-			return ret;
-
-		ret = rw_verify_area(rw, file, &req->ki_pos, req->ki_nbytes);
here ret could be possibly set to a positive number.
How?
                ret = (opcode == IOCB_CMD_PREADV ||
                       opcode == IOCB_CMD_PWRITEV)
                        ? aio_setup_vectored_rw(req, rw, buf, &nr_segs,
                                                &iovec, compat)
                        : aio_setup_single_vector(req, rw, buf, &nr_segs,
                                                  iovec);

Quoted 5 lines

Where aio_setup_vectored_rw:

        if (ret < 0)
                return ret;
[..]
	return 0;

ah right, it replaces the return value. Ignore me then.

-Lukas

Quoted 28 lines

and aio_setup_single_vector:
        if (unlikely(!access_ok(!rw, buf, kiocb->ki_nbytes)))
                return -EFAULT;
[..]
	        return 0;
Both functions are returning ssize_t, thus it's either 0 on success or negative on failure.
"if (ret)" replaced by "if (ret < 0)" should indeed set off alarm bells, but turns it turns out to be fine here.
Quoted 4 lines
fs/aio.c rw_common: · line 1332
Quoted 3 lines
fs/aio.c +2 -0
+		if (!ret)
+			ret = rw_verify_area(rw, file, &req->ki_pos, req->ki_nbytes);
 		if (ret < 0) {
So this check is fine and cleanup will be called.
However, there is a yet to be merged patch which fixes actual problem which is weird rw_copy_check_uvector semantics:
https://lkml.org/lkml/2014/4/25/778
rendering this patch unnecessary