* [PATCH v2 0/3] xfs: miscellaneous log recovery fixes @ 2017-10-25 18:57 Brian Foster 2017-10-25 18:57 ` [PATCH v2 1/3] xfs: more robust recovery xlog buffer validation Brian Foster ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Brian Foster @ 2017-10-25 18:57 UTC (permalink / raw) To: linux-xfs 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/3] xfs: more robust recovery xlog buffer validation 2017-10-25 18:57 [PATCH v2 0/3] xfs: miscellaneous log recovery fixes Brian Foster @ 2017-10-25 18:57 ` Brian Foster 2017-10-25 22:12 ` Darrick J. Wong 2017-10-26 13:27 ` [PATCH v3] " Brian Foster 2017-10-25 18:57 ` [PATCH v2 2/3] xfs: fix log block underflow during recovery cycle verification Brian Foster 2017-10-25 18:57 ` [PATCH v2 3/3] xfs: drain the buffer LRU on mount Brian Foster 2 siblings, 2 replies; 9+ messages in thread From: Brian Foster @ 2017-10-25 18:57 UTC (permalink / raw) To: linux-xfs 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] xfs: more robust recovery xlog buffer validation 2017-10-25 18:57 ` [PATCH v2 1/3] xfs: more robust recovery xlog buffer validation Brian Foster @ 2017-10-25 22:12 ` Darrick J. Wong 2017-10-26 10:21 ` Brian Foster 2017-10-26 13:27 ` [PATCH v3] " Brian Foster 1 sibling, 1 reply; 9+ messages in thread From: Darrick J. Wong @ 2017-10-25 22:12 UTC (permalink / raw) To: Brian Foster; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] xfs: more robust recovery xlog buffer validation 2017-10-25 22:12 ` Darrick J. Wong @ 2017-10-26 10:21 ` Brian Foster 0 siblings, 0 replies; 9+ messages in thread From: Brian Foster @ 2017-10-26 10:21 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] xfs: more robust recovery xlog buffer validation 2017-10-25 18:57 ` [PATCH v2 1/3] xfs: more robust recovery xlog buffer validation Brian Foster 2017-10-25 22:12 ` Darrick J. Wong @ 2017-10-26 13:27 ` Brian Foster 2017-10-26 15:59 ` Darrick J. Wong 1 sibling, 1 reply; 9+ messages in thread From: Brian Foster @ 2017-10-26 13:27 UTC (permalink / raw) To: linux-xfs 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] xfs: more robust recovery xlog buffer validation 2017-10-26 13:27 ` [PATCH v3] " Brian Foster @ 2017-10-26 15:59 ` Darrick J. Wong 0 siblings, 0 replies; 9+ messages in thread From: Darrick J. Wong @ 2017-10-26 15:59 UTC (permalink / raw) To: Brian Foster; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/3] xfs: fix log block underflow during recovery cycle verification 2017-10-25 18:57 [PATCH v2 0/3] xfs: miscellaneous log recovery fixes Brian Foster 2017-10-25 18:57 ` [PATCH v2 1/3] xfs: more robust recovery xlog buffer validation Brian Foster @ 2017-10-25 18:57 ` Brian Foster 2017-10-25 18:57 ` [PATCH v2 3/3] xfs: drain the buffer LRU on mount Brian Foster 2 siblings, 0 replies; 9+ messages in thread From: Brian Foster @ 2017-10-25 18:57 UTC (permalink / raw) To: linux-xfs 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] xfs: drain the buffer LRU on mount 2017-10-25 18:57 [PATCH v2 0/3] xfs: miscellaneous log recovery fixes Brian Foster 2017-10-25 18:57 ` [PATCH v2 1/3] xfs: more robust recovery xlog buffer validation Brian Foster 2017-10-25 18:57 ` [PATCH v2 2/3] xfs: fix log block underflow during recovery cycle verification Brian Foster @ 2017-10-25 18:57 ` Brian Foster 2017-10-26 16:31 ` Darrick J. Wong 2 siblings, 1 reply; 9+ messages in thread From: Brian Foster @ 2017-10-25 18:57 UTC (permalink / raw) To: linux-xfs 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/3] xfs: drain the buffer LRU on mount 2017-10-25 18:57 ` [PATCH v2 3/3] xfs: drain the buffer LRU on mount Brian Foster @ 2017-10-26 16:31 ` Darrick J. Wong 0 siblings, 0 replies; 9+ messages in thread From: Darrick J. Wong @ 2017-10-26 16:31 UTC (permalink / raw) To: Brian Foster; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-10-26 16:31 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-25 18:57 [PATCH v2 0/3] xfs: miscellaneous log recovery fixes Brian Foster 2017-10-25 18:57 ` [PATCH v2 1/3] xfs: more robust recovery xlog buffer validation Brian Foster 2017-10-25 22:12 ` Darrick J. Wong 2017-10-26 10:21 ` Brian Foster 2017-10-26 13:27 ` [PATCH v3] " Brian Foster 2017-10-26 15:59 ` Darrick J. Wong 2017-10-25 18:57 ` [PATCH v2 2/3] xfs: fix log block underflow during recovery cycle verification Brian Foster 2017-10-25 18:57 ` [PATCH v2 3/3] xfs: drain the buffer LRU on mount Brian Foster 2017-10-26 16:31 ` Darrick J. Wong
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.