[PATCH 0/4] Fix possible use after free with AIO

[PATCH 0/4] Fix possible use after free with AIO

[Jan Kara]

  Hi,

  when simplifying ext4 IO completion code I realized that the only 
thing pinning inode while AIO is running is file reference from kiocb. 
Thus once aio_complete() is called, inode can be freed. So calling
inode_dio_complete() after aio_complete() is possibly modifying
already freed inode (although practically the race window is tiny).

This patch series fixes all the problematic sites. Patches are 
completely independent so each of them can go through the respective 
maintainer. 

                                                                Honza

[PATCH 1/4] ext4: Fix possible use-after-free with AIO

[Jan Kara]

Running AIO is pinning inode in memory using file reference. Once AIO
is completed using aio_complete(), file reference is put and inode can
be freed from memory. So we have to be sure that calling aio_complete()
is the last thing we do with the inode.

CC: linux-ext4@vger.kernel.org
CC: "Theodore Ts'o" <tytso@mit.edu>
CC: stable@vger.kernel.org
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c   |    2 +-
 fs/ext4/page-io.c |    9 ++++-----
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index cbfe13b..ba06638 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2977,9 +2977,9 @@ static void ext4_end_io_dio(struct kiocb *iocb, loff_t offset,
 	if (!(io_end->flag & EXT4_IO_END_UNWRITTEN)) {
 		ext4_free_io_end(io_end);
 out:
+		inode_dio_done(inode);
 		if (is_async)
 			aio_complete(iocb, ret, 0);
-		inode_dio_done(inode);
 		return;
 	}
 
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 0016fbc..b42d04f 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -103,14 +103,13 @@ static int ext4_end_io(ext4_io_end_t *io)
 			 "(inode %lu, offset %llu, size %zd, error %d)",
 			 inode->i_ino, offset, size, ret);
 	}
-	if (io->iocb)
-		aio_complete(io->iocb, io->result, 0);
-
-	if (io->flag & EXT4_IO_END_DIRECT)
-		inode_dio_done(inode);
 	/* Wake up anyone waiting on unwritten extent conversion */
 	if (atomic_dec_and_test(&EXT4_I(inode)->i_unwritten))
 		wake_up_all(ext4_ioend_wq(inode));
+	if (io->flag & EXT4_IO_END_DIRECT)
+		inode_dio_done(inode);
+	if (io->iocb)
+		aio_complete(io->iocb, io->result, 0);
 	return ret;
 }
 
-- 
1.7.1

Re: [PATCH 1/4] ext4: Fix possible use-after-free with AIO

[Carlos Maiolino]

Looks Good,

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
On Wed, Jan 23, 2013 at 01:56:17PM +0100, Jan Kara wrote:
> Running AIO is pinning inode in memory using file reference. Once AIO
> is completed using aio_complete(), file reference is put and inode can
> be freed from memory. So we have to be sure that calling aio_complete()
> is the last thing we do with the inode.
> 
> CC: linux-ext4@vger.kernel.org
> CC: "Theodore Ts'o" <tytso@mit.edu>
> CC: stable@vger.kernel.org
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/inode.c   |    2 +-
>  fs/ext4/page-io.c |    9 ++++-----
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index cbfe13b..ba06638 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2977,9 +2977,9 @@ static void ext4_end_io_dio(struct kiocb *iocb, loff_t offset,
>  	if (!(io_end->flag & EXT4_IO_END_UNWRITTEN)) {
>  		ext4_free_io_end(io_end);
>  out:
> +		inode_dio_done(inode);
>  		if (is_async)
>  			aio_complete(iocb, ret, 0);
> -		inode_dio_done(inode);
>  		return;
>  	}
>  
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 0016fbc..b42d04f 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -103,14 +103,13 @@ static int ext4_end_io(ext4_io_end_t *io)
>  			 "(inode %lu, offset %llu, size %zd, error %d)",
>  			 inode->i_ino, offset, size, ret);
>  	}
> -	if (io->iocb)
> -		aio_complete(io->iocb, io->result, 0);
> -
> -	if (io->flag & EXT4_IO_END_DIRECT)
> -		inode_dio_done(inode);
>  	/* Wake up anyone waiting on unwritten extent conversion */
>  	if (atomic_dec_and_test(&EXT4_I(inode)->i_unwritten))
>  		wake_up_all(ext4_ioend_wq(inode));
> +	if (io->flag & EXT4_IO_END_DIRECT)
> +		inode_dio_done(inode);
> +	if (io->iocb)
> +		aio_complete(io->iocb, io->result, 0);
>  	return ret;
>  }
>  
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Carlos

[PATCH 2/4] xfs: Fix possible use-after-free with AIO

[Jan Kara]

Running AIO is pinning inode in memory using file reference. Once AIO
is completed using aio_complete(), file reference is put and inode can
be freed from memory. So we have to be sure that calling aio_complete()
is the last thing we do with the inode.

CC: xfs@oss.sgi.com
CC: Ben Myers <bpm@sgi.com>
CC: stable@vger.kernel.org
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/xfs/xfs_aops.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 4111a40..5f707e5 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -86,11 +86,11 @@ xfs_destroy_ioend(
 	}
 
 	if (ioend->io_iocb) {
+		inode_dio_done(ioend->io_inode);
 		if (ioend->io_isasync) {
 			aio_complete(ioend->io_iocb, ioend->io_error ?
 					ioend->io_error : ioend->io_result, 0);
 		}
-		inode_dio_done(ioend->io_inode);
 	}
 
 	mempool_free(ioend, xfs_ioend_pool);
-- 
1.7.1

Re: [PATCH 2/4] xfs: Fix possible use-after-free with AIO

[Ben Myers]

On Wed, Jan 23, 2013 at 01:56:18PM +0100, Jan Kara wrote:
> Running AIO is pinning inode in memory using file reference. Once AIO
> is completed using aio_complete(), file reference is put and inode can
> be freed from memory. So we have to be sure that calling aio_complete()
> is the last thing we do with the inode.
> 
> CC: xfs@oss.sgi.com
> CC: Ben Myers <bpm@sgi.com>
> CC: stable@vger.kernel.org
> Signed-off-by: Jan Kara <jack@suse.cz>

Looks good!

Reviewed-by: Ben Myers <bpm@sgi.com>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/4] ocfs2: Fix possible use-after-free with AIO

[Jan Kara]

Running AIO is pinning inode in memory using file reference. Once AIO
is completed using aio_complete(), file reference is put and inode can
be freed from memory. So we have to be sure that calling aio_complete()
is the last thing we do with the inode.

CC: Joel Becker <jlbec@evilplan.org>
CC: ocfs2-devel@oss.oracle.com
CC: stable@vger.kernel.org
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ocfs2/aops.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
index 6577432..340bd02 100644
--- a/fs/ocfs2/aops.c
+++ b/fs/ocfs2/aops.c
@@ -593,9 +593,9 @@ static void ocfs2_dio_end_io(struct kiocb *iocb,
 	level = ocfs2_iocb_rw_locked_level(iocb);
 	ocfs2_rw_unlock(inode, level);
 
+	inode_dio_done(inode);
 	if (is_async)
 		aio_complete(iocb, ret, 0);
-	inode_dio_done(inode);
 }
 
 /*
-- 
1.7.1

[PATCH 4/4] fs: Fix possible use-after-free with AIO

[Jan Kara]

Running AIO is pinning inode in memory using file reference. Once AIO
is completed using aio_complete(), file reference is put and inode can
be freed from memory. So we have to be sure that calling aio_complete()
is the last thing we do with the inode.

CC: Christoph Hellwig <hch@infradead.org>
CC: Jens Axboe <axboe@kernel.dk>
CC: Jeff Moyer <jmoyer@redhat.com>
CC: stable@vger.kernel.org
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/direct-io.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index cf5b44b..f853263 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -261,9 +261,9 @@ static ssize_t dio_complete(struct dio *dio, loff_t offset, ssize_t ret, bool is
 		dio->end_io(dio->iocb, offset, transferred,
 			    dio->private, ret, is_async);
 	} else {
+		inode_dio_done(dio->inode);
 		if (is_async)
 			aio_complete(dio->iocb, ret, 0);
-		inode_dio_done(dio->inode);
 	}
 
 	return ret;
-- 
1.7.1

Re: [PATCH 4/4] fs: Fix possible use-after-free with AIO

[Jeff Moyer]

Jan Kara <jack@suse.cz> writes:

> Running AIO is pinning inode in memory using file reference. Once AIO
> is completed using aio_complete(), file reference is put and inode can
> be freed from memory. So we have to be sure that calling aio_complete()
> is the last thing we do with the inode.
>
> CC: Christoph Hellwig <hch@infradead.org>
> CC: Jens Axboe <axboe@kernel.dk>
> CC: Jeff Moyer <jmoyer@redhat.com>
> CC: stable@vger.kernel.org
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/direct-io.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index cf5b44b..f853263 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -261,9 +261,9 @@ static ssize_t dio_complete(struct dio *dio, loff_t offset, ssize_t ret, bool is
>  		dio->end_io(dio->iocb, offset, transferred,
>  			    dio->private, ret, is_async);
>  	} else {
> +		inode_dio_done(dio->inode);
>  		if (is_async)
>  			aio_complete(dio->iocb, ret, 0);
> -		inode_dio_done(dio->inode);
>  	}

OK, so this is only a problem if nobody is waiting in inode_dio_wait,
yes?  Good catch, though it seems incredibly unlikely anyone would trip
over this in practice (since fput is done in a worker thread, or
deferred).

Acked-by: Jeff Moyer <jmoyer@redhat.com>

Re: [PATCH 4/4] fs: Fix possible use-after-free with AIO

[Jan Kara]

On Wed 23-01-13 10:02:09, Jeff Moyer wrote:
> Jan Kara <jack@suse.cz> writes:
> 
> > Running AIO is pinning inode in memory using file reference. Once AIO
> > is completed using aio_complete(), file reference is put and inode can
> > be freed from memory. So we have to be sure that calling aio_complete()
> > is the last thing we do with the inode.
> >
> > CC: Christoph Hellwig <hch@infradead.org>
> > CC: Jens Axboe <axboe@kernel.dk>
> > CC: Jeff Moyer <jmoyer@redhat.com>
> > CC: stable@vger.kernel.org
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/direct-io.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/fs/direct-io.c b/fs/direct-io.c
> > index cf5b44b..f853263 100644
> > --- a/fs/direct-io.c
> > +++ b/fs/direct-io.c
> > @@ -261,9 +261,9 @@ static ssize_t dio_complete(struct dio *dio, loff_t offset, ssize_t ret, bool is
> >  		dio->end_io(dio->iocb, offset, transferred,
> >  			    dio->private, ret, is_async);
> >  	} else {
> > +		inode_dio_done(dio->inode);
> >  		if (is_async)
> >  			aio_complete(dio->iocb, ret, 0);
> > -		inode_dio_done(dio->inode);
> >  	}
> 
> OK, so this is only a problem if nobody is waiting in inode_dio_wait,
> yes?  Good catch, though it seems incredibly unlikely anyone would trip
> over this in practice (since fput is done in a worker thread, or
> deferred).
  Yes, it's mostly a theoretical race.

> Acked-by: Jeff Moyer <jmoyer@redhat.com>
  Thanks.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 0/4] Fix possible use after free with AIO

[Jeff Moyer]

Jan Kara <jack@suse.cz> writes:

>   Hi,
>
>   when simplifying ext4 IO completion code I realized that the only 
> thing pinning inode while AIO is running is file reference from kiocb. 
> Thus once aio_complete() is called, inode can be freed. So calling
> inode_dio_complete() after aio_complete() is possibly modifying
> already freed inode (although practically the race window is tiny).
>
> This patch series fixes all the problematic sites. Patches are 
> completely independent so each of them can go through the respective 
> maintainer. 

for the series:

Acked-by: Jeff Moyer <jmoyer@redhat.com>

[PATCH 0/4 v2] Fix possible use after free with AIO

[Jan Kara]

  Hi,

  since nobody seem to have picked up any of the patches (at least
nobody told me so), I've added acks I received to the patches and I'm
sending them to Al for merging. Al, can you please merge these? Thanks.

								Honza

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/4] xfs: Fix possible use-after-free with AIO

[Jan Kara]

Running AIO is pinning inode in memory using file reference. Once AIO
is completed using aio_complete(), file reference is put and inode can
be freed from memory. So we have to be sure that calling aio_complete()
is the last thing we do with the inode.

CC: xfs@oss.sgi.com
CC: Ben Myers <bpm@sgi.com>
CC: stable@vger.kernel.org
Reviewed-by: Ben Myers <bpm@sgi.com>
Acked-by: Jeff Moyer <jmoyer@redhat.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/xfs/xfs_aops.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 4111a40..5f707e5 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -86,11 +86,11 @@ xfs_destroy_ioend(
 	}
 
 	if (ioend->io_iocb) {
+		inode_dio_done(ioend->io_inode);
 		if (ioend->io_isasync) {
 			aio_complete(ioend->io_iocb, ioend->io_error ?
 					ioend->io_error : ioend->io_result, 0);
 		}
-		inode_dio_done(ioend->io_inode);
 	}
 
 	mempool_free(ioend, xfs_ioend_pool);
-- 
1.7.1

Re: [PATCH 2/4] xfs: Fix possible use-after-free with AIO

[Ben Myers]

Hi Jan,

On Wed, Jan 30, 2013 at 12:27:59AM +0100, Jan Kara wrote:
> Running AIO is pinning inode in memory using file reference. Once AIO
> is completed using aio_complete(), file reference is put and inode can
> be freed from memory. So we have to be sure that calling aio_complete()
> is the last thing we do with the inode.
> 
> CC: xfs@oss.sgi.com
> CC: Ben Myers <bpm@sgi.com>
> CC: stable@vger.kernel.org
> Reviewed-by: Ben Myers <bpm@sgi.com>
> Acked-by: Jeff Moyer <jmoyer@redhat.com>
> Signed-off-by: Jan Kara <jack@suse.cz>

We picked this up in the xfs tree.  Sorry to keep you hanging.

Regards,
	Ben