On Mon, Apr 20, 2020 at 10:02:21AM -0400, Brian Foster wrote:
> On Mon, Apr 20, 2020 at 01:53:22PM +1000, Dave Chinner wrote:
> > On Fri, Apr 17, 2020 at 11:08:53AM -0400, Brian Foster wrote:
> > > The dquot read/write verifier calls xfs_dqblk_verify() on every
> > > dquot in the buffer. Remove the duplicate call from
> > > xfs_qm_dqflush().
> >
> > Ah, I think there's a bug here - it's not supposed to be a
> > duplicate....
> >
> > > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> > > ---
> > > fs/xfs/xfs_dquot.c | 14 --------------
> > > 1 file changed, 14 deletions(-)
> > >
> > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> > > index af2c8e5ceea0..73032c18a94a 100644
> > > --- a/fs/xfs/xfs_dquot.c
> > > +++ b/fs/xfs/xfs_dquot.c
> > > @@ -1071,7 +1071,6 @@ xfs_qm_dqflush(
> > > struct xfs_buf *bp;
> > > struct xfs_dqblk *dqb;
> > > struct xfs_disk_dquot *ddqp;
> > > - xfs_failaddr_t fa;
> > > int error;
> > >
> > > ASSERT(XFS_DQ_IS_LOCKED(dqp));
> > > @@ -1116,19 +1115,6 @@ xfs_qm_dqflush(
> > > dqb = bp->b_addr + dqp->q_bufoffset;
> > > ddqp = &dqb->dd_diskdq;
> > >
> > > - /*
> > > - * A simple sanity check in case we got a corrupted dquot.
> > > - */
> > > - fa = xfs_dqblk_verify(mp, dqb, be32_to_cpu(ddqp->d_id), 0);
> >
> > So this verifies the on disk dquot ....
> >
> > > - if (fa) {
> > > - xfs_alert(mp, "corrupt dquot ID 0x%x in memory at %pS",
> >
> > ...which issues an "in memory corruption" alert on failure...
> >
> > > - be32_to_cpu(ddqp->d_id), fa);
> > > - xfs_buf_relse(bp);
> > > - xfs_dqfunlock(dqp);
> > > - xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> > > - return -EFSCORRUPTED;
> > > - }
> > > -
> > > /* This is the only portion of data that needs to persist */
> > > memcpy(ddqp, &dqp->q_core, sizeof(struct xfs_disk_dquot));
> >
> > .... and on success we immediately overwrite the on-disk copy with
> > the unchecked in-memory copy of the dquot.
> >
> > IOWs, I think that verification call here should be checking the
> > in-memory dquot core, not the on disk buffer that is about to get
> > trashed. i.e. something like this:
> >
> > - fa = xfs_dqblk_verify(mp, dqb, be32_to_cpu(ddqp->d_id), 0);
> > + fa = xfs_dquot_verify(mp, &dqp->q_core, be32_to_cpu(ddqp->d_id), 0);
> >
>
> Isn't this still essentially duplicated by the write verifier? I don't
> feel strongly about changing it as above vs. removing it, but it does
> still seem unnecessary to me..
It's no different to the xfs_iflush_int() code that runs a heap of
checks on the in-memory inode before it is flushed to the backing
buffer. That uses a combination of open coded checks (for error
injection) and verifier functions (e.g. fork checking), so this
really isn't that unusual.
Realistically, it's better to catch the corruption as early as
possible - if we catch it here we know we corrupted the in-memory
dquot. However, if the write verifier catches it we have no idea
exactly when the corruption occurred, or whether it was a result of
a code problem or an external memory corruption in memory we haven't
modified at all...
IOWs the two checks or intended to catch very different classes of
in-memory corruptions, so they really aren't redundant or
unnecessary at all...
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
