[PATCH 3.10] aio: restore locking of ioctx list on removal

[PATCH 3.10] aio: restore locking of ioctx list on removal

[Mateusz Guzik]

Commit 36f5588905c10a8c4568a210d601fe8c3c27e0f0
"aio: refcounting cleanup" resulted in ioctx_lock not being held
during ctx removal, leaving the list susceptible to corruptions.

In mainline kernel the issue went away as a side effect of
db446a08c23d5475e6b08c87acca79ebb20f283c "aio: convert the ioctx list to
table lookup v3".

Fix the problem by restoring appropriate locking.

Signed-off-by: Mateusz Guzik <mguzik@redhat.com>
Reported-by: Eryu Guan <eguan@redhat.com>
Cc: Jeff Moyer <jmoyer@redhat.com>
Cc: Kent Overstreet <kmo@daterainc.com>
Cc: linux-aio@kvack.org
Cc: linux-kernel@vger.kernel.org

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

diff --git a/fs/aio.c b/fs/aio.c
index 2bbcacf..ebd06fd 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -423,10 +423,12 @@ static void kill_ioctx_rcu(struct rcu_head *head)
  *	when the processes owning a context have all exited to encourage
  *	the rapid destruction of the kioctx.
  */
-static void kill_ioctx(struct kioctx *ctx)
+static void kill_ioctx(struct mm_struct *mm, struct kioctx *ctx)
 {
 	if (!atomic_xchg(&ctx->dead, 1)) {
+		spin_lock(&mm->ioctx_lock);
 		hlist_del_rcu(&ctx->list);
+		spin_unlock(&mm->ioctx_lock);
 
 		/*
 		 * It'd be more correct to do this in free_ioctx(), after all
@@ -494,7 +496,7 @@ void exit_aio(struct mm_struct *mm)
 		 */
 		ctx->mmap_size = 0;
 
-		kill_ioctx(ctx);
+		kill_ioctx(mm, ctx);
 	}
 }
 
@@ -852,7 +854,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(ioctx);
+			kill_ioctx(current->mm, ioctx);
 		put_ioctx(ioctx);
 	}
 
@@ -870,7 +872,7 @@ SYSCALL_DEFINE1(io_destroy, aio_context_t, ctx)
 {
 	struct kioctx *ioctx = lookup_ioctx(ctx);
 	if (likely(NULL != ioctx)) {
-		kill_ioctx(ioctx);
+		kill_ioctx(current->mm, ioctx);
 		put_ioctx(ioctx);
 		return 0;
 	}
-- 
1.8.3.1

Re: [PATCH 3.10] aio: restore locking of ioctx list on removal

[Greg KH]

On Thu, Dec 05, 2013 at 11:09:02AM +0100, Mateusz Guzik wrote:
> Commit 36f5588905c10a8c4568a210d601fe8c3c27e0f0
> "aio: refcounting cleanup" resulted in ioctx_lock not being held
> during ctx removal, leaving the list susceptible to corruptions.
> 
> In mainline kernel the issue went away as a side effect of
> db446a08c23d5475e6b08c87acca79ebb20f283c "aio: convert the ioctx list to
> table lookup v3".
> 
> Fix the problem by restoring appropriate locking.

Why can't I just take db446a08c23d5475e6b08c87acca79ebb20f283c instead?
Does it not work well enough, or is there other issues involved in it
that would keep it out of stable?

Also, it seems like the performance increase of that patch would be good
to have backported, right?

thanks,

greg k-h

Re: [PATCH 3.10] aio: restore locking of ioctx list on removal

[Mateusz Guzik]

On Thu, Dec 05, 2013 at 05:03:47PM -0800, Greg KH wrote:
> On Thu, Dec 05, 2013 at 11:09:02AM +0100, Mateusz Guzik wrote:
> > Commit 36f5588905c10a8c4568a210d601fe8c3c27e0f0
> > "aio: refcounting cleanup" resulted in ioctx_lock not being held
> > during ctx removal, leaving the list susceptible to corruptions.
> > 
> > In mainline kernel the issue went away as a side effect of
> > db446a08c23d5475e6b08c87acca79ebb20f283c "aio: convert the ioctx list to
> > table lookup v3".
> > 
> > Fix the problem by restoring appropriate locking.
> 
> Why can't I just take db446a08c23d5475e6b08c87acca79ebb20f283c instead?
> Does it not work well enough, or is there other issues involved in it
> that would keep it out of stable?
> 
> Also, it seems like the performance increase of that patch would be good
> to have backported, right?
> 

Sorry, should have noted this in my original message:
db446a08c23d5475e6b08c87acca79ebb20f283c is not trivial and applying it
results in some conflicts, in addition to that the patch itself had bugs
which were fixed in:
da90382c2ec367aac88ff6aa76afb659ee0e4235
f30d704fe1244c44a984d3d1f47bc648bcc6c9f7
77d30b14d24e557f89c41980011d72428514d729
d9b2c8714aef102dea95544a8cd9372b21af463f

It may be that the most convienent way to deal with this backport would
be to just sync the file with mainline.

As such, I think backporting is too risky at this stage.

Additionally my understanding of Documentation/stable_kernel_rules.txt
was that somewhat simpler patches are preferred.

So in the end I decided to fix the problem just by adding locking.

Unfortunately at this time I can't volunteer to do the work if
backporting is preferred.

-- 
Mateusz Guzik

Re: [PATCH 3.10] aio: restore locking of ioctx list on removal

[Benjamin LaHaise]

Hi Greg, Mateusz,

On Fri, Dec 06, 2013 at 09:51:37AM +0100, Mateusz Guzik wrote:
> On Thu, Dec 05, 2013 at 05:03:47PM -0800, Greg KH wrote:
> > On Thu, Dec 05, 2013 at 11:09:02AM +0100, Mateusz Guzik wrote:
> > > Commit 36f5588905c10a8c4568a210d601fe8c3c27e0f0
> > > "aio: refcounting cleanup" resulted in ioctx_lock not being held
> > > during ctx removal, leaving the list susceptible to corruptions.
> > > 
> > > In mainline kernel the issue went away as a side effect of
> > > db446a08c23d5475e6b08c87acca79ebb20f283c "aio: convert the ioctx list to
> > > table lookup v3".
> > > 
> > > Fix the problem by restoring appropriate locking.
> > 
> > Why can't I just take db446a08c23d5475e6b08c87acca79ebb20f283c instead?
> > Does it not work well enough, or is there other issues involved in it
> > that would keep it out of stable?
> > 
> > Also, it seems like the performance increase of that patch would be good
> > to have backported, right?
> > 
> 
> Sorry, should have noted this in my original message:
> db446a08c23d5475e6b08c87acca79ebb20f283c is not trivial and applying it
> results in some conflicts, in addition to that the patch itself had bugs
> which were fixed in:
> da90382c2ec367aac88ff6aa76afb659ee0e4235
> f30d704fe1244c44a984d3d1f47bc648bcc6c9f7
> 77d30b14d24e557f89c41980011d72428514d729
> d9b2c8714aef102dea95544a8cd9372b21af463f
> 
> It may be that the most convienent way to deal with this backport would
> be to just sync the file with mainline.
> 
> As such, I think backporting is too risky at this stage.

Mateusz's analysis is correct.  Backporting db446a08c23d5475e6b08c87acca79ebb20f283c
would essentially mean redoing the entire patch and would end up being a  
bunch more untested code.  Please take Mateusz's patch which fixes the bug 
within a narrow scope.  It is applicable to 3.10 and 3.11.  For 3.12 the 
issue is fixed by other changes.  Feel free to add my Acked-by.

                -ben


> Additionally my understanding of Documentation/stable_kernel_rules.txt
> was that somewhat simpler patches are preferred.
> 
> So in the end I decided to fix the problem just by adding locking.
> 
> Unfortunately at this time I can't volunteer to do the work if
> backporting is preferred.
> 
> -- 
> Mateusz Guzik
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-aio' in
> the body to majordomo@kvack.org.  For more info on Linux AIO,
> see: http://www.kvack.org/aio/
> Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

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

Re: [PATCH 3.10] aio: restore locking of ioctx list on removal

[Luis Henriques]

On Fri, Dec 06, 2013 at 09:53:43AM -0500, Benjamin LaHaise wrote:
> Hi Greg, Mateusz,
> 
> On Fri, Dec 06, 2013 at 09:51:37AM +0100, Mateusz Guzik wrote:
> > On Thu, Dec 05, 2013 at 05:03:47PM -0800, Greg KH wrote:
> > > On Thu, Dec 05, 2013 at 11:09:02AM +0100, Mateusz Guzik wrote:
> > > > Commit 36f5588905c10a8c4568a210d601fe8c3c27e0f0
> > > > "aio: refcounting cleanup" resulted in ioctx_lock not being held
> > > > during ctx removal, leaving the list susceptible to corruptions.
> > > > 
> > > > In mainline kernel the issue went away as a side effect of
> > > > db446a08c23d5475e6b08c87acca79ebb20f283c "aio: convert the ioctx list to
> > > > table lookup v3".
> > > > 
> > > > Fix the problem by restoring appropriate locking.
> > > 
> > > Why can't I just take db446a08c23d5475e6b08c87acca79ebb20f283c instead?
> > > Does it not work well enough, or is there other issues involved in it
> > > that would keep it out of stable?
> > > 
> > > Also, it seems like the performance increase of that patch would be good
> > > to have backported, right?
> > > 
> > 
> > Sorry, should have noted this in my original message:
> > db446a08c23d5475e6b08c87acca79ebb20f283c is not trivial and applying it
> > results in some conflicts, in addition to that the patch itself had bugs
> > which were fixed in:
> > da90382c2ec367aac88ff6aa76afb659ee0e4235
> > f30d704fe1244c44a984d3d1f47bc648bcc6c9f7
> > 77d30b14d24e557f89c41980011d72428514d729
> > d9b2c8714aef102dea95544a8cd9372b21af463f
> > 
> > It may be that the most convienent way to deal with this backport would
> > be to just sync the file with mainline.
> > 
> > As such, I think backporting is too risky at this stage.
> 
> Mateusz's analysis is correct.  Backporting db446a08c23d5475e6b08c87acca79ebb20f283c
> would essentially mean redoing the entire patch and would end up being a  
> bunch more untested code.  Please take Mateusz's patch which fixes the bug 
> within a narrow scope.  It is applicable to 3.10 and 3.11.  For 3.12 the 
> issue is fixed by other changes.  Feel free to add my Acked-by.
> 
>                 -ben

Thank you, I'm queuing it for the 3.11 kernel as well.

Cheers,
--
Luis


>
> 
> > Additionally my understanding of Documentation/stable_kernel_rules.txt
> > was that somewhat simpler patches are preferred.
> > 
> > So in the end I decided to fix the problem just by adding locking.
> > 
> > Unfortunately at this time I can't volunteer to do the work if
> > backporting is preferred.
> > 
> > -- 
> > Mateusz Guzik
> > 
> > --
> > To unsubscribe, send a message with 'unsubscribe linux-aio' in
> > the body to majordomo@kvack.org.  For more info on Linux AIO,
> > see: http://www.kvack.org/aio/
> > Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
> 
> -- 
> "Thought is the essence of where you are now."
> --
> To unsubscribe from this list: send the line "unsubscribe stable" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3.10] aio: restore locking of ioctx list on removal

[Benjamin LaHaise]

On Thu, Dec 05, 2013 at 05:03:47PM -0800, Greg KH wrote:
> On Thu, Dec 05, 2013 at 11:09:02AM +0100, Mateusz Guzik wrote:
> > Commit 36f5588905c10a8c4568a210d601fe8c3c27e0f0
> > "aio: refcounting cleanup" resulted in ioctx_lock not being held
> > during ctx removal, leaving the list susceptible to corruptions.
> > 
> > In mainline kernel the issue went away as a side effect of
> > db446a08c23d5475e6b08c87acca79ebb20f283c "aio: convert the ioctx list to
> > table lookup v3".
> > 
> > Fix the problem by restoring appropriate locking.
> 
> Why can't I just take db446a08c23d5475e6b08c87acca79ebb20f283c instead?
> Does it not work well enough, or is there other issues involved in it
> that would keep it out of stable?
> 
> Also, it seems like the performance increase of that patch would be good
> to have backported, right?

There are several aio patches that have to be applied to -stable, but I've 
just been waiting for some feedback now that they're in Linus' tree for a 
bit before pushing them to -stable.  I'll review which ones are needed 
today and send out a recommended list of patches to apply.

		-ben

> thanks,
> 
> greg k-h
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-aio' in
> the body to majordomo@kvack.org.  For more info on Linux AIO,
> see: http://www.kvack.org/aio/
> Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

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