On Mon, Jul 27, 2015 at 11:13:06AM +1000, Dave Chinner wrote:
> On Tue, Jul 21, 2015 at 01:59:34PM -0400, Brian Foster wrote:
> > On Tue, Jul 21, 2015 at 01:44:57PM -0400, Brian Foster wrote:
> > > Each log record header contains an h_size value which represents the
> > > size of the iclog buffers when the record was logged and an h_len value
> > > which identifies the length of the particular log record. Log recovery
> > > uses both fields to determine the size of the log buffers to use for
> > > recovery and to correctly process each log record.
> > >
> > > Neither field is completely validated during recovery, however. While
> > > on-disk corruptions might be detected by CRC verification, we are still
> > > susceptible to errors such as excessively sized buffer allocations and
> > > overruns of the log record header and data buffers.
> > >
> > > Update the xlog_valid_rec_header() function to validate the record
> > > h_size and h_len fields against a new max_size parameter. The maximum
> > > size value is passed as a parameter because the value differs depending
> > > on whether we are trying to identify the iclog size or actually
> > > processing a log record. In the former case, validate that neither field
> > > exceeds the maximum supported iclog size of XFS. Once the iclog size is
> > > identified and log record buffers allocated, validate all records to be
> > > processed against the iclog size to ensure that buffer overrun cannot
> > > occur.
> > >
> > > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> > > ---
> > >
> > > Here's a stab at addressing the log record field validation issues Dave
> > > calls out here:
> > >
> > > http://oss.sgi.com/pipermail/xfs/2015-July/042557.html
> > >
> > > This is still undergoing some testing, but I've not hit any problems so
> > > far...
> > >
> >
> > ... aaaaannd sure enough I managed to hit an assert blow up within a
> > couple minutes of sending this out. ;)
> >
> > kernel: XFS: Assertion failed: 0, file: fs/xfs/xfs_log_recover.c, line:
> > 2005
> >
> > I'm not yet sure whether it is related. It seems somewhat strange if it
> > is. I'll see if it's reproducible with and without these patches.
> > Thoughts appreciated on this change in the meantime...
>
> Any update on this assert failure?
>
I haven't been able to reproduce so far. That said, my log recovery
testing thus far has been limited by the umount hang addressed by the
EFI patches I posted towards the end of last week. FWIW, that series
alone underwent a decent amount of recovery testing to show that it at
least addressed the hang without reproducing any such assert failures.
I'll have to put these all together in a series and beat on that for a
while.
>
> > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > > index 01dd228..98c420a 100644
> > > --- a/fs/xfs/xfs_log_recover.c
> > > +++ b/fs/xfs/xfs_log_recover.c
> > > @@ -4078,13 +4078,22 @@ xlog_unpack_data(
> > > return 0;
> > > }
> > >
> > > +/*
> > > + * Sanity check a log record header. The caller provides the maximum
> > > iclog size
> > > + * and record length since validity depends on the context. For example,
> > > the
> > > + * first record is used to allocate buffers and thus is validated
> > > against the
> > > + * maximum supported iclog size. Subsequent records must be validated
> > > against
> > > + * the identified iclog size to avoid overflow of the record buffers.
> > > + */
> > > STATIC int
> > > xlog_valid_rec_header(
> > > struct xlog *log,
> > > struct xlog_rec_header *rhead,
> > > - xfs_daddr_t blkno)
> > > + xfs_daddr_t blkno,
> > > + int max_size)
> > > {
> > > int hlen;
> > > + int hsize;
> > >
> > > if (unlikely(rhead->h_magicno != cpu_to_be32(XLOG_HEADER_MAGIC_NUM))) {
> > > XFS_ERROR_REPORT("xlog_valid_rec_header(1)",
> > > @@ -4099,14 +4108,21 @@ xlog_valid_rec_header(
> > > return -EIO;
> > > }
> > >
> > > + hsize = be32_to_cpu(rhead->h_size);
>
> be32_to_cpu() returns an unsigned value, so the hsize/hlen variables
> can de declared as u32/uint32_t, and then
>
> > > + if (unlikely(hsize <= 0 || hsize > max_size)) {
>
> The comparison for < 0 can go away.
>
Ok.
> Also, I don't think that unlikely() provides any value here - it
> makes the code harder to read, it's not a performance sensitive
> path, and, IIRC, gcc already weights branches that end in a return
> statement as "unlikely".
>
Alright. I'll fix up the rest of the function as such.
> > > + xfs_warn(log->l_mp, "%s: invalid iclog size (%d).", __func__,
> > > + hsize);
> > > + return -EFSCORRUPTED;
> > > + }
> > > +
> > > /* LR body must have data or it wouldn't have been written */
> > > hlen = be32_to_cpu(rhead->h_len);
> > > - if (unlikely( hlen <= 0 || hlen > INT_MAX )) {
> > > + if (unlikely(hlen <= 0 || hlen > max_size)) {
> > > XFS_ERROR_REPORT("xlog_valid_rec_header(2)",
> > > XFS_ERRLEVEL_LOW, log->l_mp);
> > > return -EFSCORRUPTED;
> > > }
> > > - if (unlikely( blkno > log->l_logBBsize || blkno > INT_MAX )) {
> > > + if (unlikely(blkno > log->l_logBBsize || blkno > INT_MAX)) {
> > > XFS_ERROR_REPORT("xlog_valid_rec_header(3)",
> > > XFS_ERRLEVEL_LOW, log->l_mp);
> > > return -EFSCORRUPTED;
> > > @@ -4159,7 +4175,8 @@ xlog_do_recovery_pass(
> > > goto bread_err1;
> > >
> > > rhead = (xlog_rec_header_t *)offset;
> > > - error = xlog_valid_rec_header(log, rhead, tail_blk);
> > > + error = xlog_valid_rec_header(log, rhead, tail_blk,
> > > + XLOG_MAX_RECORD_BSIZE);
>
> For v1 logs this is XLOG_BIG_RECORD_BSIZE, not
> XLOG_MAX_RECORD_BSIZE.
>
This is all within an 'if (xfs_sb_version_haslogv2(...))' block. h_size
(which the subsequent xlog_valid_rec_header() calls use) is fixed to
XLOG_BIG_RECORD_BSIZE in the else block.
Brian
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
|