diff mbox series

[1/3] xfs: relax shortform directory size checks

Message ID 157281982989.4150947.13552708841526849932.stgit@magnolia (mailing list archive)
State Accepted
Headers show
Series xfs: tidy up corruption reporting | expand

Commit Message

Darrick J. Wong Nov. 3, 2019, 10:23 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Each of the four functions that operate on shortform directories checks
that the directory's di_size is at least as large as the shortform
directory header.  This is now checked by the inode fork verifiers
(di_size is used to allocate if_bytes, and if_bytes is checked against
the header structure size) so we can turn these checks into ASSERTions.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_dir2_block.c |    8 +-------
 fs/xfs/libxfs/xfs_dir2_sf.c    |   32 ++++----------------------------
 2 files changed, 5 insertions(+), 35 deletions(-)

Comments

Carlos Maiolino Nov. 4, 2019, 3:22 p.m. UTC | #1 
On Sun, Nov 03, 2019 at 02:23:49PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Each of the four functions that operate on shortform directories checks
> that the directory's di_size is at least as large as the shortform
> directory header.  This is now checked by the inode fork verifiers
> (di_size is used to allocate if_bytes, and if_bytes is checked against
> the header structure size) so we can turn these checks into ASSERTions.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good:

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>


> ---
>  fs/xfs/libxfs/xfs_dir2_block.c |    8 +-------
>  fs/xfs/libxfs/xfs_dir2_sf.c    |   32 ++++----------------------------
>  2 files changed, 5 insertions(+), 35 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
> index 49e4bc39e7bb..e1afa35141c5 100644
> --- a/fs/xfs/libxfs/xfs_dir2_block.c
> +++ b/fs/xfs/libxfs/xfs_dir2_block.c
> @@ -1073,13 +1073,7 @@ xfs_dir2_sf_to_block(
>  	mp = dp->i_mount;
>  	ifp = XFS_IFORK_PTR(dp, XFS_DATA_FORK);
>  	ASSERT(ifp->if_flags & XFS_IFINLINE);
> -	/*
> -	 * Bomb out if the shortform directory is way too short.
> -	 */
> -	if (dp->i_d.di_size < offsetof(xfs_dir2_sf_hdr_t, parent)) {
> -		ASSERT(XFS_FORCED_SHUTDOWN(mp));
> -		return -EIO;
> -	}
> +	ASSERT(dp->i_d.di_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
>  
>  	oldsfp = (xfs_dir2_sf_hdr_t *)ifp->if_u1.if_data;
>  
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index ae16ca7c422a..d6b164a2fe57 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -277,13 +277,7 @@ xfs_dir2_sf_addname(
>  	ASSERT(xfs_dir2_sf_lookup(args) == -ENOENT);
>  	dp = args->dp;
>  	ASSERT(dp->i_df.if_flags & XFS_IFINLINE);
> -	/*
> -	 * Make sure the shortform value has some of its header.
> -	 */
> -	if (dp->i_d.di_size < offsetof(xfs_dir2_sf_hdr_t, parent)) {
> -		ASSERT(XFS_FORCED_SHUTDOWN(dp->i_mount));
> -		return -EIO;
> -	}
> +	ASSERT(dp->i_d.di_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
>  	ASSERT(dp->i_df.if_bytes == dp->i_d.di_size);
>  	ASSERT(dp->i_df.if_u1.if_data != NULL);
>  	sfp = (xfs_dir2_sf_hdr_t *)dp->i_df.if_u1.if_data;
> @@ -793,13 +787,7 @@ xfs_dir2_sf_lookup(
>  	dp = args->dp;
>  
>  	ASSERT(dp->i_df.if_flags & XFS_IFINLINE);
> -	/*
> -	 * Bail out if the directory is way too short.
> -	 */
> -	if (dp->i_d.di_size < offsetof(xfs_dir2_sf_hdr_t, parent)) {
> -		ASSERT(XFS_FORCED_SHUTDOWN(dp->i_mount));
> -		return -EIO;
> -	}
> +	ASSERT(dp->i_d.di_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
>  	ASSERT(dp->i_df.if_bytes == dp->i_d.di_size);
>  	ASSERT(dp->i_df.if_u1.if_data != NULL);
>  	sfp = (xfs_dir2_sf_hdr_t *)dp->i_df.if_u1.if_data;
> @@ -879,13 +867,7 @@ xfs_dir2_sf_removename(
>  
>  	ASSERT(dp->i_df.if_flags & XFS_IFINLINE);
>  	oldsize = (int)dp->i_d.di_size;
> -	/*
> -	 * Bail out if the directory is way too short.
> -	 */
> -	if (oldsize < offsetof(xfs_dir2_sf_hdr_t, parent)) {
> -		ASSERT(XFS_FORCED_SHUTDOWN(dp->i_mount));
> -		return -EIO;
> -	}
> +	ASSERT(oldsize >= offsetof(struct xfs_dir2_sf_hdr, parent));
>  	ASSERT(dp->i_df.if_bytes == oldsize);
>  	ASSERT(dp->i_df.if_u1.if_data != NULL);
>  	sfp = (xfs_dir2_sf_hdr_t *)dp->i_df.if_u1.if_data;
> @@ -963,13 +945,7 @@ xfs_dir2_sf_replace(
>  	dp = args->dp;
>  
>  	ASSERT(dp->i_df.if_flags & XFS_IFINLINE);
> -	/*
> -	 * Bail out if the shortform directory is way too small.
> -	 */
> -	if (dp->i_d.di_size < offsetof(xfs_dir2_sf_hdr_t, parent)) {
> -		ASSERT(XFS_FORCED_SHUTDOWN(dp->i_mount));
> -		return -EIO;
> -	}
> +	ASSERT(dp->i_d.di_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
>  	ASSERT(dp->i_df.if_bytes == dp->i_d.di_size);
>  	ASSERT(dp->i_df.if_u1.if_data != NULL);
>  	sfp = (xfs_dir2_sf_hdr_t *)dp->i_df.if_u1.if_data;
>
Christoph Hellwig Nov. 4, 2019, 7:23 p.m. UTC | #2 
On Sun, Nov 03, 2019 at 02:23:49PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Each of the four functions that operate on shortform directories checks
> that the directory's di_size is at least as large as the shortform
> directory header.  This is now checked by the inode fork verifiers
> (di_size is used to allocate if_bytes, and if_bytes is checked against
> the header structure size) so we can turn these checks into ASSERTions.

Looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
index 49e4bc39e7bb..e1afa35141c5 100644
--- a/fs/xfs/libxfs/xfs_dir2_block.c
+++ b/fs/xfs/libxfs/xfs_dir2_block.c
@@ -1073,13 +1073,7 @@  xfs_dir2_sf_to_block(
 	mp = dp->i_mount;
 	ifp = XFS_IFORK_PTR(dp, XFS_DATA_FORK);
 	ASSERT(ifp->if_flags & XFS_IFINLINE);
-	/*
-	 * Bomb out if the shortform directory is way too short.
-	 */
-	if (dp->i_d.di_size < offsetof(xfs_dir2_sf_hdr_t, parent)) {
-		ASSERT(XFS_FORCED_SHUTDOWN(mp));
-		return -EIO;
-	}
+	ASSERT(dp->i_d.di_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
 
 	oldsfp = (xfs_dir2_sf_hdr_t *)ifp->if_u1.if_data;
 
diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index ae16ca7c422a..d6b164a2fe57 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -277,13 +277,7 @@  xfs_dir2_sf_addname(
 	ASSERT(xfs_dir2_sf_lookup(args) == -ENOENT);
 	dp = args->dp;
 	ASSERT(dp->i_df.if_flags & XFS_IFINLINE);
-	/*
-	 * Make sure the shortform value has some of its header.
-	 */
-	if (dp->i_d.di_size < offsetof(xfs_dir2_sf_hdr_t, parent)) {
-		ASSERT(XFS_FORCED_SHUTDOWN(dp->i_mount));
-		return -EIO;
-	}
+	ASSERT(dp->i_d.di_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
 	ASSERT(dp->i_df.if_bytes == dp->i_d.di_size);
 	ASSERT(dp->i_df.if_u1.if_data != NULL);
 	sfp = (xfs_dir2_sf_hdr_t *)dp->i_df.if_u1.if_data;
@@ -793,13 +787,7 @@  xfs_dir2_sf_lookup(
 	dp = args->dp;
 
 	ASSERT(dp->i_df.if_flags & XFS_IFINLINE);
-	/*
-	 * Bail out if the directory is way too short.
-	 */
-	if (dp->i_d.di_size < offsetof(xfs_dir2_sf_hdr_t, parent)) {
-		ASSERT(XFS_FORCED_SHUTDOWN(dp->i_mount));
-		return -EIO;
-	}
+	ASSERT(dp->i_d.di_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
 	ASSERT(dp->i_df.if_bytes == dp->i_d.di_size);
 	ASSERT(dp->i_df.if_u1.if_data != NULL);
 	sfp = (xfs_dir2_sf_hdr_t *)dp->i_df.if_u1.if_data;
@@ -879,13 +867,7 @@  xfs_dir2_sf_removename(
 
 	ASSERT(dp->i_df.if_flags & XFS_IFINLINE);
 	oldsize = (int)dp->i_d.di_size;
-	/*
-	 * Bail out if the directory is way too short.
-	 */
-	if (oldsize < offsetof(xfs_dir2_sf_hdr_t, parent)) {
-		ASSERT(XFS_FORCED_SHUTDOWN(dp->i_mount));
-		return -EIO;
-	}
+	ASSERT(oldsize >= offsetof(struct xfs_dir2_sf_hdr, parent));
 	ASSERT(dp->i_df.if_bytes == oldsize);
 	ASSERT(dp->i_df.if_u1.if_data != NULL);
 	sfp = (xfs_dir2_sf_hdr_t *)dp->i_df.if_u1.if_data;
@@ -963,13 +945,7 @@  xfs_dir2_sf_replace(
 	dp = args->dp;
 
 	ASSERT(dp->i_df.if_flags & XFS_IFINLINE);
-	/*
-	 * Bail out if the shortform directory is way too small.
-	 */
-	if (dp->i_d.di_size < offsetof(xfs_dir2_sf_hdr_t, parent)) {
-		ASSERT(XFS_FORCED_SHUTDOWN(dp->i_mount));
-		return -EIO;
-	}
+	ASSERT(dp->i_d.di_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
 	ASSERT(dp->i_df.if_bytes == dp->i_d.di_size);
 	ASSERT(dp->i_df.if_u1.if_data != NULL);
 	sfp = (xfs_dir2_sf_hdr_t *)dp->i_df.if_u1.if_data;