diff mbox series

[2/4] mkfs: check root inode location

Message ID 157530817131.126767.4542572453231190489.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs_repair: do not trash valid root dirs | expand

Commit Message

Darrick J. Wong Dec. 2, 2019, 5:36 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Make sure the root inode gets created where repair thinks it should be
created.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/libxfs_api_defs.h |    1 +
 mkfs/xfs_mkfs.c          |   29 +++++++++++++++++++++++------
 2 files changed, 24 insertions(+), 6 deletions(-)

Comments

Brian Foster Dec. 3, 2019, 1:02 p.m. UTC | #1 
On Mon, Dec 02, 2019 at 09:36:11AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Make sure the root inode gets created where repair thinks it should be
> created.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  libxfs/libxfs_api_defs.h |    1 +
>  mkfs/xfs_mkfs.c          |   29 +++++++++++++++++++++++------
>  2 files changed, 24 insertions(+), 6 deletions(-)
> 
> 
> diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> index 645c9b1b..8f6b9fc2 100644
> --- a/libxfs/libxfs_api_defs.h
> +++ b/libxfs/libxfs_api_defs.h
> @@ -156,5 +156,6 @@
>  
>  #define xfs_ag_init_headers		libxfs_ag_init_headers
>  #define xfs_buf_delwri_submit		libxfs_buf_delwri_submit
> +#define xfs_ialloc_find_prealloc	libxfs_ialloc_find_prealloc
>  

Perhaps this should be in the previous patch..?


>  #endif /* __LIBXFS_API_DEFS_H__ */
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 18338a61..5143d9b4 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3521,6 +3521,28 @@ rewrite_secondary_superblocks(
>  	libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);
>  }
>  
> +static void
> +check_root_ino(
> +	struct xfs_mount	*mp)
> +{
> +	xfs_agino_t		first, last;
> +
> +	if (XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino) != 0) {
> +		fprintf(stderr,
> +			_("%s: root inode created in AG %u, not AG 0\n"),
> +			progname, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino));
> +		exit(1);
> +	}
> +
> +	libxfs_ialloc_find_prealloc(mp, &first, &last);
> +	if (mp->m_sb.sb_rootino != XFS_AGINO_TO_INO(mp, 0, first)) {
> +		fprintf(stderr,
> +			_("%s: root inode (%llu) not created in first chunk\n"),
> +			progname, (unsigned long long)mp->m_sb.sb_rootino);

If the root inode ended up somewhere in the middle of the first chunk,
we'd fail (rightly), but with a misleading error message. Perhaps
something like "root inode (..) not allocated in expected location"
would be better? I'd also like to see a comment somewhere in here to
explain why we have this check. For example:

"The superblock refers directly to the root inode, but repair makes
hardcoded assumptions about its location based on filesystem geometry
for an extra level of verification. If this assumption ever breaks, we
should flag it immediately and fail the mkfs. Otherwise repair may
consider the filesystem corrupt and toss the root inode."

Feel free to reword that however appropriate (given the behavior change
in subsequent patches), of course..

Brian

> +		exit(1);
> +	}
> +}
> +
>  int
>  main(
>  	int			argc,
> @@ -3807,12 +3829,7 @@ main(
>  	/*
>  	 * Protect ourselves against possible stupidity
>  	 */
> -	if (XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino) != 0) {
> -		fprintf(stderr,
> -			_("%s: root inode created in AG %u, not AG 0\n"),
> -			progname, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino));
> -		exit(1);
> -	}
> +	check_root_ino(mp);
>  
>  	/*
>  	 * Re-write multiple secondary superblocks with rootinode field set
>
Darrick J. Wong Dec. 3, 2019, 11:40 p.m. UTC | #2 
On Tue, Dec 03, 2019 at 08:02:53AM -0500, Brian Foster wrote:
> On Mon, Dec 02, 2019 at 09:36:11AM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Make sure the root inode gets created where repair thinks it should be
> > created.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  libxfs/libxfs_api_defs.h |    1 +
> >  mkfs/xfs_mkfs.c          |   29 +++++++++++++++++++++++------
> >  2 files changed, 24 insertions(+), 6 deletions(-)
> > 
> > 
> > diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> > index 645c9b1b..8f6b9fc2 100644
> > --- a/libxfs/libxfs_api_defs.h
> > +++ b/libxfs/libxfs_api_defs.h
> > @@ -156,5 +156,6 @@
> >  
> >  #define xfs_ag_init_headers		libxfs_ag_init_headers
> >  #define xfs_buf_delwri_submit		libxfs_buf_delwri_submit
> > +#define xfs_ialloc_find_prealloc	libxfs_ialloc_find_prealloc
> >  
> 
> Perhaps this should be in the previous patch..?

<shrug> I think the libxfs wrapper macro things shouldn't be introduced
until there's a caller outside of libxfs.

> 
> >  #endif /* __LIBXFS_API_DEFS_H__ */
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index 18338a61..5143d9b4 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -3521,6 +3521,28 @@ rewrite_secondary_superblocks(
> >  	libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);
> >  }
> >  
> > +static void
> > +check_root_ino(
> > +	struct xfs_mount	*mp)
> > +{
> > +	xfs_agino_t		first, last;
> > +
> > +	if (XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino) != 0) {
> > +		fprintf(stderr,
> > +			_("%s: root inode created in AG %u, not AG 0\n"),
> > +			progname, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino));
> > +		exit(1);
> > +	}
> > +
> > +	libxfs_ialloc_find_prealloc(mp, &first, &last);
> > +	if (mp->m_sb.sb_rootino != XFS_AGINO_TO_INO(mp, 0, first)) {
> > +		fprintf(stderr,
> > +			_("%s: root inode (%llu) not created in first chunk\n"),
> > +			progname, (unsigned long long)mp->m_sb.sb_rootino);
> 
> If the root inode ended up somewhere in the middle of the first chunk,
> we'd fail (rightly), but with a misleading error message. Perhaps
> something like "root inode (..) not allocated in expected location"

Ok, fixed.

> would be better? I'd also like to see a comment somewhere in here to
> explain why we have this check. For example:
> 
> "The superblock refers directly to the root inode, but repair makes
> hardcoded assumptions about its location based on filesystem geometry
> for an extra level of verification. If this assumption ever breaks, we
> should flag it immediately and fail the mkfs. Otherwise repair may
> consider the filesystem corrupt and toss the root inode."

How about:

/*
 * The superblock points to the root directory inode, but xfs_repair
 * expects to find the root inode in a very specific location computed
 * from the filesystem geometry for an extra level of verification.
 *
 * Fail the format immediately if those assumptions ever break, because
 * repair will toss the root directory.
 */

> Feel free to reword that however appropriate (given the behavior change
> in subsequent patches), of course..

Ok.

--D

> Brian
> 
> > +		exit(1);
> > +	}
> > +}
> > +
> >  int
> >  main(
> >  	int			argc,
> > @@ -3807,12 +3829,7 @@ main(
> >  	/*
> >  	 * Protect ourselves against possible stupidity
> >  	 */
> > -	if (XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino) != 0) {
> > -		fprintf(stderr,
> > -			_("%s: root inode created in AG %u, not AG 0\n"),
> > -			progname, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino));
> > -		exit(1);
> > -	}
> > +	check_root_ino(mp);
> >  
> >  	/*
> >  	 * Re-write multiple secondary superblocks with rootinode field set
> > 
>
Brian Foster Dec. 4, 2019, 11:51 a.m. UTC | #3 
On Tue, Dec 03, 2019 at 03:40:07PM -0800, Darrick J. Wong wrote:
> On Tue, Dec 03, 2019 at 08:02:53AM -0500, Brian Foster wrote:
> > On Mon, Dec 02, 2019 at 09:36:11AM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Make sure the root inode gets created where repair thinks it should be
> > > created.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  libxfs/libxfs_api_defs.h |    1 +
> > >  mkfs/xfs_mkfs.c          |   29 +++++++++++++++++++++++------
> > >  2 files changed, 24 insertions(+), 6 deletions(-)
> > > 
> > > 
> > > diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> > > index 645c9b1b..8f6b9fc2 100644
> > > --- a/libxfs/libxfs_api_defs.h
> > > +++ b/libxfs/libxfs_api_defs.h
> > > @@ -156,5 +156,6 @@
> > >  
> > >  #define xfs_ag_init_headers		libxfs_ag_init_headers
> > >  #define xfs_buf_delwri_submit		libxfs_buf_delwri_submit
> > > +#define xfs_ialloc_find_prealloc	libxfs_ialloc_find_prealloc
> > >  
> > 
> > Perhaps this should be in the previous patch..?
> 
> <shrug> I think the libxfs wrapper macro things shouldn't be introduced
> until there's a caller outside of libxfs.
> 

Ok, fair enough..

> > 
> > >  #endif /* __LIBXFS_API_DEFS_H__ */
> > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > > index 18338a61..5143d9b4 100644
> > > --- a/mkfs/xfs_mkfs.c
> > > +++ b/mkfs/xfs_mkfs.c
> > > @@ -3521,6 +3521,28 @@ rewrite_secondary_superblocks(
> > >  	libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);
> > >  }
> > >  
> > > +static void
> > > +check_root_ino(
> > > +	struct xfs_mount	*mp)
> > > +{
> > > +	xfs_agino_t		first, last;
> > > +
> > > +	if (XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino) != 0) {
> > > +		fprintf(stderr,
> > > +			_("%s: root inode created in AG %u, not AG 0\n"),
> > > +			progname, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino));
> > > +		exit(1);
> > > +	}
> > > +
> > > +	libxfs_ialloc_find_prealloc(mp, &first, &last);
> > > +	if (mp->m_sb.sb_rootino != XFS_AGINO_TO_INO(mp, 0, first)) {
> > > +		fprintf(stderr,
> > > +			_("%s: root inode (%llu) not created in first chunk\n"),
> > > +			progname, (unsigned long long)mp->m_sb.sb_rootino);
> > 
> > If the root inode ended up somewhere in the middle of the first chunk,
> > we'd fail (rightly), but with a misleading error message. Perhaps
> > something like "root inode (..) not allocated in expected location"
> 
> Ok, fixed.
> 
> > would be better? I'd also like to see a comment somewhere in here to
> > explain why we have this check. For example:
> > 
> > "The superblock refers directly to the root inode, but repair makes
> > hardcoded assumptions about its location based on filesystem geometry
> > for an extra level of verification. If this assumption ever breaks, we
> > should flag it immediately and fail the mkfs. Otherwise repair may
> > consider the filesystem corrupt and toss the root inode."
> 
> How about:
> 
> /*
>  * The superblock points to the root directory inode, but xfs_repair
>  * expects to find the root inode in a very specific location computed
>  * from the filesystem geometry for an extra level of verification.
>  *
>  * Fail the format immediately if those assumptions ever break, because
>  * repair will toss the root directory.
>  */
> 

Sounds good, thanks!

Brian

> > Feel free to reword that however appropriate (given the behavior change
> > in subsequent patches), of course..
> 
> Ok.
> 
> --D
> 
> > Brian
> > 
> > > +		exit(1);
> > > +	}
> > > +}
> > > +
> > >  int
> > >  main(
> > >  	int			argc,
> > > @@ -3807,12 +3829,7 @@ main(
> > >  	/*
> > >  	 * Protect ourselves against possible stupidity
> > >  	 */
> > > -	if (XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino) != 0) {
> > > -		fprintf(stderr,
> > > -			_("%s: root inode created in AG %u, not AG 0\n"),
> > > -			progname, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino));
> > > -		exit(1);
> > > -	}
> > > +	check_root_ino(mp);
> > >  
> > >  	/*
> > >  	 * Re-write multiple secondary superblocks with rootinode field set
> > > 
> > 
>
diff mbox series

Patch

diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index 645c9b1b..8f6b9fc2 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -156,5 +156,6 @@ 
 
 #define xfs_ag_init_headers		libxfs_ag_init_headers
 #define xfs_buf_delwri_submit		libxfs_buf_delwri_submit
+#define xfs_ialloc_find_prealloc	libxfs_ialloc_find_prealloc
 
 #endif /* __LIBXFS_API_DEFS_H__ */
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 18338a61..5143d9b4 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3521,6 +3521,28 @@  rewrite_secondary_superblocks(
 	libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);
 }
 
+static void
+check_root_ino(
+	struct xfs_mount	*mp)
+{
+	xfs_agino_t		first, last;
+
+	if (XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino) != 0) {
+		fprintf(stderr,
+			_("%s: root inode created in AG %u, not AG 0\n"),
+			progname, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino));
+		exit(1);
+	}
+
+	libxfs_ialloc_find_prealloc(mp, &first, &last);
+	if (mp->m_sb.sb_rootino != XFS_AGINO_TO_INO(mp, 0, first)) {
+		fprintf(stderr,
+			_("%s: root inode (%llu) not created in first chunk\n"),
+			progname, (unsigned long long)mp->m_sb.sb_rootino);
+		exit(1);
+	}
+}
+
 int
 main(
 	int			argc,
@@ -3807,12 +3829,7 @@  main(
 	/*
 	 * Protect ourselves against possible stupidity
 	 */
-	if (XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino) != 0) {
-		fprintf(stderr,
-			_("%s: root inode created in AG %u, not AG 0\n"),
-			progname, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rootino));
-		exit(1);
-	}
+	check_root_ino(mp);
 
 	/*
 	 * Re-write multiple secondary superblocks with rootinode field set