[SRU,X,v2] xfs: fix xfs_log_ticket leak in xfs_end_io() after fs shutdown

Message ID 20170802172723.732-1-rafael.tinoco@canonical.com
State New
Headers show

Commit Message

Rafael David Tinoco Aug. 2, 2017, 5:27 p.m.
From: Brian Foster <bfoster@redhat.com>

BugLink: https://bugs.launchpad.net/bugs/1706132

If the filesystem has shut down, xfs_end_io() currently sets an
error on the ioend and proceeds to ioend destruction. The ioend
might contain a truncate transaction if the I/O extended the size of
the file. This transaction is only cleaned up in
xfs_setfilesize_ioend(), however, which is skipped in this case.
This results in an xfs_log_ticket leak message when the associate
cache slab is destroyed (e.g., on rmmod).

This was originally reproduced by xfs/141 on a distro kernel. The
problem is reproducible on an upstream kernel, but not easily
detected in current upstream if the xfs_log_ticket cache happens to
be merged with another cache. This can be reproduced more
deterministically with the 'slab_nomerge' kernel boot option.

Update xfs_end_io() to proceed with normal end I/O processing after
an error is set on an ioend due to fs shutdown. The I/O type-based
processing is already designed to handle an I/O error and ensure
that the ioend is cleaned up correctly.

Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Dave Chinner <david@fromorbit.com>
(cherry picked from commit af055e37a91d215d7174d0b84c86795ca81086a7)
Signed-off-by: Rafael David Tinoco <rafael.tinoco@canonical.com>
---
 fs/xfs/xfs_aops.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Stefan Bader Aug. 3, 2017, 1:31 p.m. | #1
On 02.08.2017 19:27, Rafael David Tinoco wrote:
> From: Brian Foster <bfoster@redhat.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1706132
> 
> If the filesystem has shut down, xfs_end_io() currently sets an
> error on the ioend and proceeds to ioend destruction. The ioend
> might contain a truncate transaction if the I/O extended the size of
> the file. This transaction is only cleaned up in
> xfs_setfilesize_ioend(), however, which is skipped in this case.
> This results in an xfs_log_ticket leak message when the associate
> cache slab is destroyed (e.g., on rmmod).
> 
> This was originally reproduced by xfs/141 on a distro kernel. The
> problem is reproducible on an upstream kernel, but not easily
> detected in current upstream if the xfs_log_ticket cache happens to
> be merged with another cache. This can be reproduced more
> deterministically with the 'slab_nomerge' kernel boot option.
> 
> Update xfs_end_io() to proceed with normal end I/O processing after
> an error is set on an ioend due to fs shutdown. The I/O type-based
> processing is already designed to handle an I/O error and ensure
> that the ioend is cleaned up correctly.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Dave Chinner <david@fromorbit.com>
> (cherry picked from commit af055e37a91d215d7174d0b84c86795ca81086a7)
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>

> ---
>  fs/xfs/xfs_aops.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 4a13f5311a41..0c8dacea411e 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -214,10 +214,12 @@ xfs_end_io(
>  	struct xfs_inode *ip = XFS_I(ioend->io_inode);
>  	int		error = 0;
>  
> -	if (XFS_FORCED_SHUTDOWN(ip->i_mount)) {
> +	/*
> +	 * Set an error if the mount has shut down and proceed with end I/O
> +	 * processing so it can perform whatever cleanups are necessary.
> +	 */
> +	if (XFS_FORCED_SHUTDOWN(ip->i_mount))
>  		ioend->io_error = -EIO;
> -		goto done;
> -	}
>  
>  	/*
>  	 * For unwritten extents we need to issue transactions to convert a
>
Kleber Sacilotto de Souza Aug. 4, 2017, 8:04 a.m. | #2
On 08/02/17 19:27, Rafael David Tinoco wrote:
> From: Brian Foster <bfoster@redhat.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1706132
> 
> If the filesystem has shut down, xfs_end_io() currently sets an
> error on the ioend and proceeds to ioend destruction. The ioend
> might contain a truncate transaction if the I/O extended the size of
> the file. This transaction is only cleaned up in
> xfs_setfilesize_ioend(), however, which is skipped in this case.
> This results in an xfs_log_ticket leak message when the associate
> cache slab is destroyed (e.g., on rmmod).
> 
> This was originally reproduced by xfs/141 on a distro kernel. The
> problem is reproducible on an upstream kernel, but not easily
> detected in current upstream if the xfs_log_ticket cache happens to
> be merged with another cache. This can be reproduced more
> deterministically with the 'slab_nomerge' kernel boot option.
> 
> Update xfs_end_io() to proceed with normal end I/O processing after
> an error is set on an ioend due to fs shutdown. The I/O type-based
> processing is already designed to handle an I/O error and ensure
> that the ioend is cleaned up correctly.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Dave Chinner <david@fromorbit.com>
> (cherry picked from commit af055e37a91d215d7174d0b84c86795ca81086a7)
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@canonical.com>

Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

> ---
>  fs/xfs/xfs_aops.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 4a13f5311a41..0c8dacea411e 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -214,10 +214,12 @@ xfs_end_io(
>  	struct xfs_inode *ip = XFS_I(ioend->io_inode);
>  	int		error = 0;
>  
> -	if (XFS_FORCED_SHUTDOWN(ip->i_mount)) {
> +	/*
> +	 * Set an error if the mount has shut down and proceed with end I/O
> +	 * processing so it can perform whatever cleanups are necessary.
> +	 */
> +	if (XFS_FORCED_SHUTDOWN(ip->i_mount))
>  		ioend->io_error = -EIO;
> -		goto done;
> -	}
>  
>  	/*
>  	 * For unwritten extents we need to issue transactions to convert a
>
Kleber Sacilotto de Souza Aug. 8, 2017, 8:13 a.m. | #3
Applied on xenial/master-next branch. Thanks.

Patch

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 4a13f5311a41..0c8dacea411e 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -214,10 +214,12 @@  xfs_end_io(
 	struct xfs_inode *ip = XFS_I(ioend->io_inode);
 	int		error = 0;
 
-	if (XFS_FORCED_SHUTDOWN(ip->i_mount)) {
+	/*
+	 * Set an error if the mount has shut down and proceed with end I/O
+	 * processing so it can perform whatever cleanups are necessary.
+	 */
+	if (XFS_FORCED_SHUTDOWN(ip->i_mount))
 		ioend->io_error = -EIO;
-		goto done;
-	}
 
 	/*
 	 * For unwritten extents we need to issue transactions to convert a