Re: [PATCH 13/13] repair: phase 1 does not handle superblock CRCs

[Brian Foster <bfoster@redhat.com>]

On Tue, Mar 04, 2014 at 07:51:57PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Phase 1 of xfs_repair verifies and corrects the primary
> superblock of the filesystem. It does not verify that the CRC of the
> superblock that is found is correct, nor does it recalculate the CRC
> of the superblock it rewrites.
> 
> This happens because phase1 does not use the libxfs buffer cache -
> it just uses pread/pwrite on a memory buffer, and works directly
> from that buffer. Hence we need to add CRC verification to
> verify_sb(), and CRC recalculation to write_primary_sb() so that it
> works correctly.
> 
> This also enables us to use get_sb() as the method of fetching the
> superblock from disk after phase 1 without needing to use the libxfs
> buffer cache and guessing at the sector size. This prevents a
> verifier error because it attempts to CRC a superblock buffer that
> is much longer than the usual sector sizes.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  repair/agheader.c   |  2 +-
>  repair/globals.h    |  3 ++-
>  repair/phase1.c     |  5 ++--
>  repair/protos.h     |  3 ++-
>  repair/sb.c         | 71 +++++++++++++++++++++++++++++------------------------
>  repair/xfs_repair.c | 26 +++++++++++---------
>  6 files changed, 62 insertions(+), 48 deletions(-)
> 
> diff --git a/repair/agheader.c b/repair/agheader.c
> index 53e47b6..fc5dac9 100644
> --- a/repair/agheader.c
> +++ b/repair/agheader.c
> @@ -472,7 +472,7 @@ verify_set_agheader(xfs_mount_t *mp, xfs_buf_t *sbuf, xfs_sb_t *sb,
>  	int status = XR_OK;
>  	int status_sb = XR_OK;
>  
> -	status = verify_sb(sb, (i == 0));
> +	status = verify_sb(sbuf->b_addr, sb, (i == 0));
>  
>  	if (status != XR_OK)  {
>  		do_warn(_("bad on-disk superblock %d - %s\n"),
> diff --git a/repair/globals.h b/repair/globals.h
> index cbb2ce7..f6e0a22 100644
> --- a/repair/globals.h
> +++ b/repair/globals.h
> @@ -49,7 +49,8 @@
>  #define XR_BAD_SB_UNIT		17	/* bad stripe unit */
>  #define XR_BAD_SB_WIDTH		18	/* bad stripe width */
>  #define XR_BAD_SVN		19	/* bad shared version number */
> -#define XR_BAD_ERR_CODE		20	/* Bad error code */
> +#define XR_BAD_CRC		20	/* Bad CRC */
> +#define XR_BAD_ERR_CODE		21	/* Bad error code */
>  
>  /* XFS filesystem (il)legal values */
>  
> diff --git a/repair/phase1.c b/repair/phase1.c
> index 62de211..ec75ada 100644
> --- a/repair/phase1.c
> +++ b/repair/phase1.c
> @@ -70,13 +70,14 @@ phase1(xfs_mount_t *mp)
>  	ag_bp = alloc_ag_buf(MAX_SECTSIZE);
>  	sb = (xfs_sb_t *) ag_bp;
>  
> -	if (get_sb(sb, 0LL, MAX_SECTSIZE, 0) == XR_EOF)
> +	rval = get_sb(sb, 0LL, MAX_SECTSIZE, 0);
> +	if (rval == XR_EOF)
>  		do_error(_("error reading primary superblock\n"));
>  
>  	/*
>  	 * is this really an sb, verify internal consistency
>  	 */

This comment can probably go away now. Otherwise, looks good...

Reviewed-by: Brian Foster <bfoster@redhat.com>

> -	if ((rval = verify_sb(sb, 1)) != XR_OK)  {
> +	if (rval != XR_OK)  {
>  		do_warn(_("bad primary superblock - %s !!!\n"),
>  			err_string(rval));
>  		if (!find_secondary_sb(sb))
> diff --git a/repair/protos.h b/repair/protos.h
> index 601f2a9..ff42fa7 100644
> --- a/repair/protos.h
> +++ b/repair/protos.h
> @@ -18,7 +18,8 @@
>  
>  void	xfs_init(libxfs_init_t *args);
>  
> -int	verify_sb(xfs_sb_t		*sb,
> +int	verify_sb(char			*sb_buf,
> +		xfs_sb_t		*sb,
>  		int			is_primary_sb);
>  int	verify_set_primary_sb(xfs_sb_t	*root_sb,
>  			int		sb_index,
> diff --git a/repair/sb.c b/repair/sb.c
> index c54d89b..b111aca 100644
> --- a/repair/sb.c
> +++ b/repair/sb.c
> @@ -139,7 +139,7 @@ find_secondary_sb(xfs_sb_t *rsb)
>  			c_bufsb = (char *)sb + i;
>  			libxfs_sb_from_disk(&bufsb, (xfs_dsb_t *)c_bufsb);
>  
> -			if (verify_sb(&bufsb, 0) != XR_OK)
> +			if (verify_sb(c_bufsb, &bufsb, 0) != XR_OK)
>  				continue;
>  
>  			do_warn(_("found candidate secondary superblock...\n"));
> @@ -245,7 +245,7 @@ sb_validate_ino_align(struct xfs_sb *sb)
>   */
>  
>  int
> -verify_sb(xfs_sb_t *sb, int is_primary_sb)
> +verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb)
>  {
>  	__uint32_t	bsize;
>  	int		i;
> @@ -263,8 +263,34 @@ verify_sb(xfs_sb_t *sb, int is_primary_sb)
>  	if (is_primary_sb && sb->sb_inprogress == 1)
>  		return(XR_BAD_INPROGRESS);
>  
> -	/* check to make sure blocksize is legal 2^N, 9 <= N <= 16 */
> +	/*
> +	 * before going *any further*, validate the sector size and if the
> +	 * version says we should have CRCs enabled, validate that.
> +	 */
> +
> +	/* check to make sure sectorsize is legal 2^N, 9 <= N <= 15 */
> +	if (sb->sb_sectsize == 0)
> +		return(XR_BAD_SECT_SIZE_DATA);
> +
> +	bsize = 1;
> +	for (i = 0; bsize < sb->sb_sectsize &&
> +		i < sizeof(sb->sb_sectsize) * NBBY; i++)  {
> +		bsize <<= 1;
> +	}
> +
> +	if (i < XFS_MIN_SECTORSIZE_LOG || i > XFS_MAX_SECTORSIZE_LOG)
> +		return(XR_BAD_SECT_SIZE_DATA);
> +
> +	/* check sb sectorsize field against sb sectlog field */
> +	if (i != sb->sb_sectlog)
> +		return(XR_BAD_SECT_SIZE_DATA);
> +
> +	/* sector size in range - CRC check time */
> +	if (xfs_sb_version_hascrc(sb) &&
> +	    !xfs_verify_cksum(sb_buf, sb->sb_sectsize, XFS_SB_CRC_OFF))
> +		return XR_BAD_CRC;
>  
> +	/* check to make sure blocksize is legal 2^N, 9 <= N <= 16 */
>  	if (sb->sb_blocksize == 0)
>  		return(XR_BAD_BLOCKSIZE);
>  
> @@ -300,26 +326,6 @@ verify_sb(xfs_sb_t *sb, int is_primary_sb)
>  		sb->sb_inopblock != howmany(sb->sb_blocksize,sb->sb_inodesize))
>  		return(XR_BAD_INO_SIZE_DATA);
>  
> -	/* check to make sure sectorsize is legal 2^N, 9 <= N <= 15 */
> -
> -	if (sb->sb_sectsize == 0)
> -		return(XR_BAD_SECT_SIZE_DATA);
> -
> -	bsize = 1;
> -
> -	for (i = 0; bsize < sb->sb_sectsize &&
> -		i < sizeof(sb->sb_sectsize) * NBBY; i++)  {
> -		bsize <<= 1;
> -	}
> -
> -	if (i < XFS_MIN_SECTORSIZE_LOG || i > XFS_MAX_SECTORSIZE_LOG)
> -		return(XR_BAD_SECT_SIZE_DATA);
> -
> -	/* check sb sectorsize field against sb sectlog field */
> -
> -	if (i != sb->sb_sectlog)
> -		return(XR_BAD_SECT_SIZE_DATA);
> -
>  	if (xfs_sb_version_hassector(sb))  {
>  
>  		/* check to make sure log sector is legal 2^N, 9 <= N <= 15 */
> @@ -482,9 +488,11 @@ write_primary_sb(xfs_sb_t *sbp, int size)
>  		do_error(_("couldn't seek to offset 0 in filesystem\n"));
>  	}
>  
> -	
>  	libxfs_sb_to_disk(buf, sbp, XFS_SB_ALL_BITS);
>  
> +	if (xfs_sb_version_hascrc(sbp))
> +		xfs_update_cksum((char *)buf, size, XFS_SB_CRC_OFF);
> +
>  	if (write(x.dfd, buf, size) != size) {
>  		free(buf);
>  		do_error(_("primary superblock write failed!\n"));
> @@ -494,7 +502,7 @@ write_primary_sb(xfs_sb_t *sbp, int size)
>  }
>  
>  /*
> - * get a possible superblock -- don't check for internal consistency
> + * get a possible superblock -- checks for internal consistency
>   */
>  int
>  get_sb(xfs_sb_t *sbp, xfs_off_t off, int size, xfs_agnumber_t agno)
> @@ -529,9 +537,10 @@ get_sb(xfs_sb_t *sbp, xfs_off_t off, int size, xfs_agnumber_t agno)
>  		do_error("%s\n", strerror(error));
>  	}
>  	libxfs_sb_from_disk(sbp, buf);
> -	free(buf);
>  
> -	return (verify_sb(sbp, 0));
> +	rval = verify_sb((char *)buf, sbp, agno == 0);
> +	free(buf);
> +	return rval;
>  }
>  
>  /* returns element on list with highest reference count */
> @@ -745,13 +754,11 @@ verify_set_primary_sb(xfs_sb_t		*rsb,
>  			off = (xfs_off_t)agno * rsb->sb_agblocks << rsb->sb_blocklog;
>  
>  			checked[agno] = 1;
> -
> -			if (get_sb(sb, off, size, agno) == XR_EOF)  {
> -				retval = XR_EOF;
> +			retval = get_sb(sb, off, size, agno);
> +			if (retval == XR_EOF)
>  				goto out_free_list;
> -			}
>  
> -			if (verify_sb(sb, 0) == XR_OK)  {
> +			if (retval == XR_OK) {
>  				/*
>  				 * save away geometry info.
>  				 * don't bother checking the sb
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index 6327076..08b25f0 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -137,6 +137,8 @@ err_string(int err_code)
>  			_("bad stripe width in superblock");
>  		err_message[XR_BAD_SVN] =
>  			_("bad shared version number in superblock");
> +		err_message[XR_BAD_CRC] =
> +			_("bad CRC in superblock");
>  		done = 1;
>  	}
>  
> @@ -529,6 +531,8 @@ main(int argc, char **argv)
>  	xfs_buf_t	*sbp;
>  	xfs_mount_t	xfs_m;
>  	char		*msgbuf;
> +	struct xfs_sb	psb;
> +	int		rval;
>  
>  	progname = basename(argv[0]);
>  	setlocale(LC_ALL, "");
> @@ -558,13 +562,12 @@ main(int argc, char **argv)
>  		exit(1);
>  	}
>  
> -	/* prepare the mount structure */
> -	memset(&xfs_m, 0, sizeof(xfs_mount_t));
> -	libxfs_buftarg_init(&xfs_m, x.ddev, x.logdev, x.rtdev);
> -	sbp = libxfs_readbuf(xfs_m.m_ddev_targp, XFS_SB_DADDR,
> -				1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0,
> -				&xfs_sb_buf_ops);
> -	libxfs_sb_from_disk(&xfs_m.m_sb, XFS_BUF_TO_SBP(sbp));
> +	rval = get_sb(&psb, 0, XFS_MAX_SECTORSIZE, 0);
> +	if (rval != XR_OK) {
> +		do_warn(_("Primary superblock bad after phase 1!\n"
> +			  "Exiting now.\n"));
> +		exit(1);
> +	}
>  
>  	/*
>  	 * if the sector size of the filesystem we are trying to repair is
> @@ -583,7 +586,7 @@ main(int argc, char **argv)
>  			geom.sectsize = BBSIZE;
>  		}
>  
> -		if (xfs_m.m_sb.sb_sectsize < geom.sectsize) {
> +		if (psb.sb_sectsize < geom.sectsize) {
>  			long	old_flags;
>  
>  			old_flags = fcntl(fd, F_GETFL, 0);
> @@ -595,7 +598,10 @@ main(int argc, char **argv)
>  			}
>  		}
>  	}
> -	mp = libxfs_mount(&xfs_m, &xfs_m.m_sb, x.ddev, x.logdev, x.rtdev, 0);
> +
> +	/* prepare the mount structure */
> +	memset(&xfs_m, 0, sizeof(xfs_mount_t));
> +	mp = libxfs_mount(&xfs_m, &psb, x.ddev, x.logdev, x.rtdev, 0);
>  
>  	if (!mp)  {
>  		fprintf(stderr,
> @@ -603,8 +609,6 @@ main(int argc, char **argv)
>  			progname);
>  		exit(1);
>  	}
> -	libxfs_putbuf(sbp);
> -	libxfs_purgebuf(sbp);
>  
>  	/*
>  	 * set XFS-independent status vars from the mount/sb structure
> -- 
> 1.9.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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