[PATCH v2 0/3] xfs: miscellaneous log recovery fixes

[PATCH v2 0/3] xfs: miscellaneous log recovery fixes

[Brian Foster]

Hi all,

Here's v2 of the log recovery fixups for filesystems with undersized
logs and the v4 NULL buffer verifier problem. The most significant
change is that patch 1 is rewritten to take a different approach to
sanity check the log block addresses used during log recovery cycle and
record verification. Specifically, it validates the log block number
used for each buffer on read or write. Also, I've dropped patch 4 for
now since I've not heard anything to suggest it's really necessary. 

Brian

v2:
- Use xlog buffer validation rather than explicit checks.
- Don't push AIL on log recovery error.
- Drop patch 4 (rfc).
- Fix up commit logs.
v1: https://marc.info/?l=linux-xfs&m=150877001230751&w=2

Brian Foster (3):
  xfs: more robust recovery xlog buffer validation
  xfs: fix log block underflow during recovery cycle verification
  xfs: drain the buffer LRU on mount

 fs/xfs/xfs_log.c         | 16 ++++++++++++++++
 fs/xfs/xfs_log_recover.c | 40 +++++++++++++++++++++++++---------------
 2 files changed, 41 insertions(+), 15 deletions(-)

-- 
2.9.5

[PATCH v2 1/3] xfs: more robust recovery xlog buffer validation

[Brian Foster]

mkfs has a historical problem where it can format very small
filesystems with too small of a physical log. Under certain
conditions, log recovery of an associated filesystem can end up
passing garbage parameter values to some of the cycle and log record
verification functions due to bugs in log recovery not dealing with
such filesystems properly. This results in attempts to read from
bogus/underflowed log block addresses.

Since the buffer read may ultimately succeed, log recovery can
proceed with bogus data and otherwise go off the rails and crash.
One example of this is a negative last_blk being passed to
xlog_find_verify_log_record() causing us to skip the loop, pass a
NULL head pointer to xlog_header_check_mount() and crash.

Improve the xlog buffer verification to address this problem. We
already verify xlog buffer length, so update this mechanism to also
sanity check for a valid log relative block address and otherwise
return an error. Pass a fixed, valid log block address from
xlog_get_bp() since the target address will be validated when the
buffer is read. This ensures that any bogus log block address/length
calculations lead to graceful mount failure rather than risking a
crash or worse if recovery proceeds with bogus data.

Reported-by: Zorro Lang <zlang@redhat.com>
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_log_recover.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index ee34899..54494ab 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -85,17 +85,21 @@ struct xfs_buf_cancel {
  */
 
 /*
- * Verify the given count of basic blocks is valid number of blocks
- * to specify for an operation involving the given XFS log buffer.
- * Returns nonzero if the count is valid, 0 otherwise.
+ * Verify the log-relative block number and length in basic blocks are valid for
+ * an operation involving the given XFS log buffer. Returns true if the fields
+ * are valid, false otherwise.
  */
-
-static inline int
-xlog_buf_bbcount_valid(
+static inline bool
+xlog_verify_bp(
 	struct xlog	*log,
+	xfs_daddr_t	blk_no,
 	int		bbcount)
 {
-	return bbcount > 0 && bbcount <= log->l_logBBsize;
+	if (blk_no < 0 || blk_no >= log->l_logBBsize)
+		return false;
+	if (bbcount <= 0 || bbcount > log->l_logBBsize)
+		return false;
+	return true;
 }
 
 /*
@@ -110,7 +114,11 @@ xlog_get_bp(
 {
 	struct xfs_buf	*bp;
 
-	if (!xlog_buf_bbcount_valid(log, nbblks)) {
+	/*
+	 * Pass log block 0 since we don't have an addr yet, buffer will be
+	 * verified on read.
+	 */
+	if (!xlog_verify_bp(log, 0, nbblks)) {
 		xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer",
 			nbblks);
 		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp);
@@ -180,9 +188,10 @@ xlog_bread_noalign(
 {
 	int		error;
 
-	if (!xlog_buf_bbcount_valid(log, nbblks)) {
-		xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer",
-			nbblks);
+	if (!xlog_verify_bp(log, blk_no, nbblks)) {
+		xfs_warn(log->l_mp,
+			 "Invalid log block/length (0x%llx, 0x%x) for buffer",
+			 blk_no, nbblks);
 		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp);
 		return -EFSCORRUPTED;
 	}
@@ -265,9 +274,10 @@ xlog_bwrite(
 {
 	int		error;
 
-	if (!xlog_buf_bbcount_valid(log, nbblks)) {
-		xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer",
-			nbblks);
+	if (!xlog_verify_bp(log, blk_no, nbblks)) {
+		xfs_warn(log->l_mp,
+			 "Invalid log block/length (0x%llx, 0x%x) for buffer",
+			 blk_no, nbblks);
 		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp);
 		return -EFSCORRUPTED;
 	}
-- 
2.9.5

[PATCH v2 2/3] xfs: fix log block underflow during recovery cycle verification

[Brian Foster]

It is possible for mkfs to format very small filesystems with too
small of an internal log with respect to the various minimum size
and block count requirements. If this occurs when the log happens to
be smaller than the scan window used for cycle verification and the
scan wraps the end of the log, the start_blk calculation in
xlog_find_head() underflows and leads to an attempt to scan an
invalid range of log blocks. This results in log recovery failure
and a failed mount.

Since there may be filesystems out in the wild with this kind of
geometry, we cannot simply refuse to mount. Instead, cap the scan
window for cycle verification to the size of the physical log. This
ensures that the cycle verification proceeds as expected when the
scan wraps the end of the log.

Reported-by: Zorro Lang <zlang@redhat.com>
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_log_recover.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 54494ab..98350c9 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -763,7 +763,7 @@ xlog_find_head(
 	 * in the in-core log.  The following number can be made tighter if
 	 * we actually look at the block size of the filesystem.
 	 */
-	num_scan_bblks = XLOG_TOTAL_REC_SHIFT(log);
+	num_scan_bblks = min_t(int, log_bbnum, XLOG_TOTAL_REC_SHIFT(log));
 	if (head_blk >= num_scan_bblks) {
 		/*
 		 * We are guaranteed that the entire check can be performed
-- 
2.9.5

[PATCH v2 3/3] xfs: drain the buffer LRU on mount

[Brian Foster]

Log recovery of v4 filesystems does not use buffer verifiers because
log recovery historically can result in transient buffer corruption
when target buffers might be ahead of the log after a crash. v5
filesystems work around this problem with metadata LSN ordering.

While this log recovery verifier behavior is necessary on v4 supers,
it can result in leaving buffers around in the LRU without verifiers
attached for a significant amount of time. This leads to use of
unverified buffers while the filesystem is in active use, long after
recovery has completed.

To address this problem, drain all buffers from the LRU as a final
step of the log mount sequence. Note that this is done
unconditionally to provide a consistently clean cache footprint,
regardless of superblock version or log state. As a side effect,
this ensures that all cache resident, unverified buffers are
reclaimed after log recovery and therefore must be recreated with
verifiers on subsequent use.

Reported-by: Darrick Wong <darrick.wong@oracle.com>
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_log.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index dc95a49..ab59e78 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -744,6 +744,7 @@ xfs_log_mount_finish(
 {
 	int	error = 0;
 	bool	readonly = (mp->m_flags & XFS_MOUNT_RDONLY);
+	bool	recovered = mp->m_log->l_flags & XLOG_RECOVERY_NEEDED;
 
 	if (mp->m_flags & XFS_MOUNT_NORECOVERY) {
 		ASSERT(mp->m_flags & XFS_MOUNT_RDONLY);
@@ -780,6 +781,21 @@ xfs_log_mount_finish(
 	mp->m_super->s_flags &= ~MS_ACTIVE;
 	evict_inodes(mp->m_super);
 
+	/*
+	 * Drain the buffer LRU after log recovery. This is required for v4
+	 * filesystems to avoid leaving around buffers with NULL verifier ops,
+	 * but we do it unconditionally to make sure we're always in a clean
+	 * cache state after mount.
+	 *
+	 * Don't push in the error case because the AIL may have pending intents
+	 * that aren't removed until recovery is cancelled.
+	 */
+	if (!error && recovered) {
+		xfs_log_force(mp, XFS_LOG_SYNC);
+		xfs_ail_push_all_sync(mp->m_ail);
+	}
+	xfs_wait_buftarg(mp->m_ddev_targp);
+
 	if (readonly)
 		mp->m_flags |= XFS_MOUNT_RDONLY;
 
-- 
2.9.5

Re: [PATCH v2 1/3] xfs: more robust recovery xlog buffer validation

[Darrick J. Wong]

On Wed, Oct 25, 2017 at 02:57:03PM -0400, Brian Foster wrote:
> mkfs has a historical problem where it can format very small
> filesystems with too small of a physical log. Under certain
> conditions, log recovery of an associated filesystem can end up
> passing garbage parameter values to some of the cycle and log record
> verification functions due to bugs in log recovery not dealing with
> such filesystems properly. This results in attempts to read from
> bogus/underflowed log block addresses.
> 
> Since the buffer read may ultimately succeed, log recovery can
> proceed with bogus data and otherwise go off the rails and crash.
> One example of this is a negative last_blk being passed to
> xlog_find_verify_log_record() causing us to skip the loop, pass a
> NULL head pointer to xlog_header_check_mount() and crash.
> 
> Improve the xlog buffer verification to address this problem. We
> already verify xlog buffer length, so update this mechanism to also
> sanity check for a valid log relative block address and otherwise
> return an error. Pass a fixed, valid log block address from
> xlog_get_bp() since the target address will be validated when the
> buffer is read. This ensures that any bogus log block address/length
> calculations lead to graceful mount failure rather than risking a
> crash or worse if recovery proceeds with bogus data.
> 
> Reported-by: Zorro Lang <zlang@redhat.com>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_log_recover.c | 38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index ee34899..54494ab 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -85,17 +85,21 @@ struct xfs_buf_cancel {
>   */
>  
>  /*
> - * Verify the given count of basic blocks is valid number of blocks
> - * to specify for an operation involving the given XFS log buffer.
> - * Returns nonzero if the count is valid, 0 otherwise.
> + * Verify the log-relative block number and length in basic blocks are valid for
> + * an operation involving the given XFS log buffer. Returns true if the fields
> + * are valid, false otherwise.
>   */
> -
> -static inline int
> -xlog_buf_bbcount_valid(
> +static inline bool
> +xlog_verify_bp(
>  	struct xlog	*log,
> +	xfs_daddr_t	blk_no,
>  	int		bbcount)
>  {
> -	return bbcount > 0 && bbcount <= log->l_logBBsize;
> +	if (blk_no < 0 || blk_no >= log->l_logBBsize)
> +		return false;
> +	if (bbcount <= 0 || bbcount > log->l_logBBsize)
> +		return false;

Shouldn't this be (blk_no + bbcount) > log->l_logBBsize, since the
blk_no/bbcount parameters identify an extent within the log?

--D

> +	return true;
>  }
>  
>  /*
> @@ -110,7 +114,11 @@ xlog_get_bp(
>  {
>  	struct xfs_buf	*bp;
>  
> -	if (!xlog_buf_bbcount_valid(log, nbblks)) {
> +	/*
> +	 * Pass log block 0 since we don't have an addr yet, buffer will be
> +	 * verified on read.
> +	 */
> +	if (!xlog_verify_bp(log, 0, nbblks)) {
>  		xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer",
>  			nbblks);
>  		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp);
> @@ -180,9 +188,10 @@ xlog_bread_noalign(
>  {
>  	int		error;
>  
> -	if (!xlog_buf_bbcount_valid(log, nbblks)) {
> -		xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer",
> -			nbblks);
> +	if (!xlog_verify_bp(log, blk_no, nbblks)) {
> +		xfs_warn(log->l_mp,
> +			 "Invalid log block/length (0x%llx, 0x%x) for buffer",
> +			 blk_no, nbblks);
>  		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp);
>  		return -EFSCORRUPTED;
>  	}
> @@ -265,9 +274,10 @@ xlog_bwrite(
>  {
>  	int		error;
>  
> -	if (!xlog_buf_bbcount_valid(log, nbblks)) {
> -		xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer",
> -			nbblks);
> +	if (!xlog_verify_bp(log, blk_no, nbblks)) {
> +		xfs_warn(log->l_mp,
> +			 "Invalid log block/length (0x%llx, 0x%x) for buffer",
> +			 blk_no, nbblks);
>  		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp);
>  		return -EFSCORRUPTED;
>  	}
> -- 
> 2.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] xfs: more robust recovery xlog buffer validation

[Brian Foster]

On Wed, Oct 25, 2017 at 03:12:08PM -0700, Darrick J. Wong wrote:
> On Wed, Oct 25, 2017 at 02:57:03PM -0400, Brian Foster wrote:
> > mkfs has a historical problem where it can format very small
> > filesystems with too small of a physical log. Under certain
> > conditions, log recovery of an associated filesystem can end up
> > passing garbage parameter values to some of the cycle and log record
> > verification functions due to bugs in log recovery not dealing with
> > such filesystems properly. This results in attempts to read from
> > bogus/underflowed log block addresses.
> > 
> > Since the buffer read may ultimately succeed, log recovery can
> > proceed with bogus data and otherwise go off the rails and crash.
> > One example of this is a negative last_blk being passed to
> > xlog_find_verify_log_record() causing us to skip the loop, pass a
> > NULL head pointer to xlog_header_check_mount() and crash.
> > 
> > Improve the xlog buffer verification to address this problem. We
> > already verify xlog buffer length, so update this mechanism to also
> > sanity check for a valid log relative block address and otherwise
> > return an error. Pass a fixed, valid log block address from
> > xlog_get_bp() since the target address will be validated when the
> > buffer is read. This ensures that any bogus log block address/length
> > calculations lead to graceful mount failure rather than risking a
> > crash or worse if recovery proceeds with bogus data.
> > 
> > Reported-by: Zorro Lang <zlang@redhat.com>
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/xfs_log_recover.c | 38 ++++++++++++++++++++++++--------------
> >  1 file changed, 24 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index ee34899..54494ab 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -85,17 +85,21 @@ struct xfs_buf_cancel {
> >   */
> >  
> >  /*
> > - * Verify the given count of basic blocks is valid number of blocks
> > - * to specify for an operation involving the given XFS log buffer.
> > - * Returns nonzero if the count is valid, 0 otherwise.
> > + * Verify the log-relative block number and length in basic blocks are valid for
> > + * an operation involving the given XFS log buffer. Returns true if the fields
> > + * are valid, false otherwise.
> >   */
> > -
> > -static inline int
> > -xlog_buf_bbcount_valid(
> > +static inline bool
> > +xlog_verify_bp(
> >  	struct xlog	*log,
> > +	xfs_daddr_t	blk_no,
> >  	int		bbcount)
> >  {
> > -	return bbcount > 0 && bbcount <= log->l_logBBsize;
> > +	if (blk_no < 0 || blk_no >= log->l_logBBsize)
> > +		return false;
> > +	if (bbcount <= 0 || bbcount > log->l_logBBsize)
> > +		return false;
> 
> Shouldn't this be (blk_no + bbcount) > log->l_logBBsize, since the
> blk_no/bbcount parameters identify an extent within the log?
> 

Yes, I suppose we can do that since we pass blk_no = 0 from the get_bp()
case. The new invocations pass the blockcount of the requested I/O
operation (as opposed to the buffer size, which is probably what I was
thinking), so that seems reasonable to me.

Brian

> --D
> 
> > +	return true;
> >  }
> >  
> >  /*
> > @@ -110,7 +114,11 @@ xlog_get_bp(
> >  {
> >  	struct xfs_buf	*bp;
> >  
> > -	if (!xlog_buf_bbcount_valid(log, nbblks)) {
> > +	/*
> > +	 * Pass log block 0 since we don't have an addr yet, buffer will be
> > +	 * verified on read.
> > +	 */
> > +	if (!xlog_verify_bp(log, 0, nbblks)) {
> >  		xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer",
> >  			nbblks);
> >  		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp);
> > @@ -180,9 +188,10 @@ xlog_bread_noalign(
> >  {
> >  	int		error;
> >  
> > -	if (!xlog_buf_bbcount_valid(log, nbblks)) {
> > -		xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer",
> > -			nbblks);
> > +	if (!xlog_verify_bp(log, blk_no, nbblks)) {
> > +		xfs_warn(log->l_mp,
> > +			 "Invalid log block/length (0x%llx, 0x%x) for buffer",
> > +			 blk_no, nbblks);
> >  		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp);
> >  		return -EFSCORRUPTED;
> >  	}
> > @@ -265,9 +274,10 @@ xlog_bwrite(
> >  {
> >  	int		error;
> >  
> > -	if (!xlog_buf_bbcount_valid(log, nbblks)) {
> > -		xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer",
> > -			nbblks);
> > +	if (!xlog_verify_bp(log, blk_no, nbblks)) {
> > +		xfs_warn(log->l_mp,
> > +			 "Invalid log block/length (0x%llx, 0x%x) for buffer",
> > +			 blk_no, nbblks);
> >  		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp);
> >  		return -EFSCORRUPTED;
> >  	}
> > -- 
> > 2.9.5
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] xfs: more robust recovery xlog buffer validation

[Brian Foster]

mkfs has a historical problem where it can format very small
filesystems with too small of a physical log. Under certain
conditions, log recovery of an associated filesystem can end up
passing garbage parameter values to some of the cycle and log record
verification functions due to bugs in log recovery not dealing with
such filesystems properly. This results in attempts to read from
bogus/underflowed log block addresses.

Since the buffer read may ultimately succeed, log recovery can
proceed with bogus data and otherwise go off the rails and crash.
One example of this is a negative last_blk being passed to
xlog_find_verify_log_record() causing us to skip the loop, pass a
NULL head pointer to xlog_header_check_mount() and crash.

Improve the xlog buffer verification to address this problem. We
already verify xlog buffer length, so update this mechanism to also
sanity check for a valid log relative block address and otherwise
return an error. Pass a fixed, valid log block address from
xlog_get_bp() since the target address will be validated when the
buffer is read. This ensures that any bogus log block address/length
calculations lead to graceful mount failure rather than risking a
crash or worse if recovery proceeds with bogus data.

Reported-by: Zorro Lang <zlang@redhat.com>
Signed-off-by: Brian Foster <bfoster@redhat.com>
---

v3:
- Verify log buf I/O range is within log boundary.

 fs/xfs/xfs_log_recover.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index ee34899..1074b5f 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -85,17 +85,21 @@ struct xfs_buf_cancel {
  */
 
 /*
- * Verify the given count of basic blocks is valid number of blocks
- * to specify for an operation involving the given XFS log buffer.
- * Returns nonzero if the count is valid, 0 otherwise.
+ * Verify the log-relative block number and length in basic blocks are valid for
+ * an operation involving the given XFS log buffer. Returns true if the fields
+ * are valid, false otherwise.
  */
-
-static inline int
-xlog_buf_bbcount_valid(
+static inline bool
+xlog_verify_bp(
 	struct xlog	*log,
+	xfs_daddr_t	blk_no,
 	int		bbcount)
 {
-	return bbcount > 0 && bbcount <= log->l_logBBsize;
+	if (blk_no < 0 || blk_no >= log->l_logBBsize)
+		return false;
+	if (bbcount <= 0 || (blk_no + bbcount) > log->l_logBBsize)
+		return false;
+	return true;
 }
 
 /*
@@ -110,7 +114,11 @@ xlog_get_bp(
 {
 	struct xfs_buf	*bp;
 
-	if (!xlog_buf_bbcount_valid(log, nbblks)) {
+	/*
+	 * Pass log block 0 since we don't have an addr yet, buffer will be
+	 * verified on read.
+	 */
+	if (!xlog_verify_bp(log, 0, nbblks)) {
 		xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer",
 			nbblks);
 		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp);
@@ -180,9 +188,10 @@ xlog_bread_noalign(
 {
 	int		error;
 
-	if (!xlog_buf_bbcount_valid(log, nbblks)) {
-		xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer",
-			nbblks);
+	if (!xlog_verify_bp(log, blk_no, nbblks)) {
+		xfs_warn(log->l_mp,
+			 "Invalid log block/length (0x%llx, 0x%x) for buffer",
+			 blk_no, nbblks);
 		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp);
 		return -EFSCORRUPTED;
 	}
@@ -265,9 +274,10 @@ xlog_bwrite(
 {
 	int		error;
 
-	if (!xlog_buf_bbcount_valid(log, nbblks)) {
-		xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer",
-			nbblks);
+	if (!xlog_verify_bp(log, blk_no, nbblks)) {
+		xfs_warn(log->l_mp,
+			 "Invalid log block/length (0x%llx, 0x%x) for buffer",
+			 blk_no, nbblks);
 		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp);
 		return -EFSCORRUPTED;
 	}
-- 
2.9.5

Re: [PATCH v3] xfs: more robust recovery xlog buffer validation

[Darrick J. Wong]

On Thu, Oct 26, 2017 at 09:27:44AM -0400, Brian Foster wrote:
> mkfs has a historical problem where it can format very small
> filesystems with too small of a physical log. Under certain
> conditions, log recovery of an associated filesystem can end up
> passing garbage parameter values to some of the cycle and log record
> verification functions due to bugs in log recovery not dealing with
> such filesystems properly. This results in attempts to read from
> bogus/underflowed log block addresses.
> 
> Since the buffer read may ultimately succeed, log recovery can
> proceed with bogus data and otherwise go off the rails and crash.
> One example of this is a negative last_blk being passed to
> xlog_find_verify_log_record() causing us to skip the loop, pass a
> NULL head pointer to xlog_header_check_mount() and crash.
> 
> Improve the xlog buffer verification to address this problem. We
> already verify xlog buffer length, so update this mechanism to also
> sanity check for a valid log relative block address and otherwise
> return an error. Pass a fixed, valid log block address from
> xlog_get_bp() since the target address will be validated when the
> buffer is read. This ensures that any bogus log block address/length
> calculations lead to graceful mount failure rather than risking a
> crash or worse if recovery proceeds with bogus data.
> 
> Reported-by: Zorro Lang <zlang@redhat.com>
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> ---
> 
> v3:
> - Verify log buf I/O range is within log boundary.
> 
>  fs/xfs/xfs_log_recover.c | 38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index ee34899..1074b5f 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -85,17 +85,21 @@ struct xfs_buf_cancel {
>   */
>  
>  /*
> - * Verify the given count of basic blocks is valid number of blocks
> - * to specify for an operation involving the given XFS log buffer.
> - * Returns nonzero if the count is valid, 0 otherwise.
> + * Verify the log-relative block number and length in basic blocks are valid for
> + * an operation involving the given XFS log buffer. Returns true if the fields
> + * are valid, false otherwise.
>   */
> -
> -static inline int
> -xlog_buf_bbcount_valid(
> +static inline bool
> +xlog_verify_bp(
>  	struct xlog	*log,
> +	xfs_daddr_t	blk_no,
>  	int		bbcount)
>  {
> -	return bbcount > 0 && bbcount <= log->l_logBBsize;
> +	if (blk_no < 0 || blk_no >= log->l_logBBsize)
> +		return false;
> +	if (bbcount <= 0 || (blk_no + bbcount) > log->l_logBBsize)
> +		return false;
> +	return true;
>  }
>  
>  /*
> @@ -110,7 +114,11 @@ xlog_get_bp(
>  {
>  	struct xfs_buf	*bp;
>  
> -	if (!xlog_buf_bbcount_valid(log, nbblks)) {
> +	/*
> +	 * Pass log block 0 since we don't have an addr yet, buffer will be
> +	 * verified on read.
> +	 */
> +	if (!xlog_verify_bp(log, 0, nbblks)) {
>  		xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer",
>  			nbblks);
>  		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp);
> @@ -180,9 +188,10 @@ xlog_bread_noalign(
>  {
>  	int		error;
>  
> -	if (!xlog_buf_bbcount_valid(log, nbblks)) {
> -		xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer",
> -			nbblks);
> +	if (!xlog_verify_bp(log, blk_no, nbblks)) {
> +		xfs_warn(log->l_mp,
> +			 "Invalid log block/length (0x%llx, 0x%x) for buffer",
> +			 blk_no, nbblks);
>  		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp);
>  		return -EFSCORRUPTED;
>  	}
> @@ -265,9 +274,10 @@ xlog_bwrite(
>  {
>  	int		error;
>  
> -	if (!xlog_buf_bbcount_valid(log, nbblks)) {
> -		xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer",
> -			nbblks);
> +	if (!xlog_verify_bp(log, blk_no, nbblks)) {
> +		xfs_warn(log->l_mp,
> +			 "Invalid log block/length (0x%llx, 0x%x) for buffer",
> +			 blk_no, nbblks);
>  		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_HIGH, log->l_mp);
>  		return -EFSCORRUPTED;
>  	}
> -- 
> 2.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] xfs: drain the buffer LRU on mount

[Darrick J. Wong]

On Wed, Oct 25, 2017 at 02:57:05PM -0400, Brian Foster wrote:
> Log recovery of v4 filesystems does not use buffer verifiers because
> log recovery historically can result in transient buffer corruption
> when target buffers might be ahead of the log after a crash. v5
> filesystems work around this problem with metadata LSN ordering.
> 
> While this log recovery verifier behavior is necessary on v4 supers,
> it can result in leaving buffers around in the LRU without verifiers
> attached for a significant amount of time. This leads to use of
> unverified buffers while the filesystem is in active use, long after
> recovery has completed.
> 
> To address this problem, drain all buffers from the LRU as a final
> step of the log mount sequence. Note that this is done
> unconditionally to provide a consistently clean cache footprint,
> regardless of superblock version or log state. As a side effect,
> this ensures that all cache resident, unverified buffers are
> reclaimed after log recovery and therefore must be recreated with
> verifiers on subsequent use.
> 
> Reported-by: Darrick Wong <darrick.wong@oracle.com>
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> ---
>  fs/xfs/xfs_log.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index dc95a49..ab59e78 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -744,6 +744,7 @@ xfs_log_mount_finish(
>  {
>  	int	error = 0;
>  	bool	readonly = (mp->m_flags & XFS_MOUNT_RDONLY);
> +	bool	recovered = mp->m_log->l_flags & XLOG_RECOVERY_NEEDED;
>  
>  	if (mp->m_flags & XFS_MOUNT_NORECOVERY) {
>  		ASSERT(mp->m_flags & XFS_MOUNT_RDONLY);
> @@ -780,6 +781,21 @@ xfs_log_mount_finish(
>  	mp->m_super->s_flags &= ~MS_ACTIVE;
>  	evict_inodes(mp->m_super);
>  
> +	/*
> +	 * Drain the buffer LRU after log recovery. This is required for v4
> +	 * filesystems to avoid leaving around buffers with NULL verifier ops,
> +	 * but we do it unconditionally to make sure we're always in a clean
> +	 * cache state after mount.
> +	 *
> +	 * Don't push in the error case because the AIL may have pending intents
> +	 * that aren't removed until recovery is cancelled.
> +	 */
> +	if (!error && recovered) {
> +		xfs_log_force(mp, XFS_LOG_SYNC);
> +		xfs_ail_push_all_sync(mp->m_ail);
> +	}
> +	xfs_wait_buftarg(mp->m_ddev_targp);
> +
>  	if (readonly)
>  		mp->m_flags |= XFS_MOUNT_RDONLY;
>  
> -- 
> 2.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html