Message ID | 159353172899.2864738.6438709598863248951.stgit@magnolia (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | xfs: remove xfs_disk_quot from incore dquot | expand |
On 6/30/20 8:42 AM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > While loading dquot records off disk, make sure that the quota type > flags are the same between the incore dquot and the ondisk dquot. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> Looks ok Reviewed-by: Allison Collins <allison.henderson@oracle.com> > --- > fs/xfs/xfs_dquot.c | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) > > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > index d5b7f03e93c8..46c8ca83c04d 100644 > --- a/fs/xfs/xfs_dquot.c > +++ b/fs/xfs/xfs_dquot.c > @@ -524,13 +524,27 @@ xfs_dquot_alloc( > } > > /* Copy the in-core quota fields in from the on-disk buffer. */ > -STATIC void > +STATIC int > xfs_dquot_from_disk( > struct xfs_dquot *dqp, > struct xfs_buf *bp) > { > struct xfs_disk_dquot *ddqp = bp->b_addr + dqp->q_bufoffset; > > + /* > + * The only field the verifier didn't check was the quota type flag, so > + * do that here. > + */ > + if ((dqp->dq_flags & XFS_DQ_ALLTYPES) != > + (ddqp->d_flags & XFS_DQ_ALLTYPES) || > + dqp->q_core.d_id != ddqp->d_id) { > + xfs_alert(bp->b_mount, > + "Metadata corruption detected at %pS, quota %u", > + __this_address, be32_to_cpu(dqp->q_core.d_id)); > + xfs_alert(bp->b_mount, "Unmount and run xfs_repair"); > + return -EFSCORRUPTED; > + } > + > /* copy everything from disk dquot to the incore dquot */ > memcpy(&dqp->q_core, ddqp, sizeof(struct xfs_disk_dquot)); > > @@ -544,6 +558,7 @@ xfs_dquot_from_disk( > > /* initialize the dquot speculative prealloc thresholds */ > xfs_dquot_set_prealloc_limits(dqp); > + return 0; > } > > /* Allocate and initialize the dquot buffer for this in-core dquot. */ > @@ -617,9 +632,11 @@ xfs_qm_dqread( > * further. > */ > ASSERT(xfs_buf_islocked(bp)); > - xfs_dquot_from_disk(dqp, bp); > - > + error = xfs_dquot_from_disk(dqp, bp); > xfs_buf_relse(bp); > + if (error) > + goto err; > + > *dqpp = dqp; > return error; > >
On Tuesday 30 June 2020 9:12:09 PM IST Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > While loading dquot records off disk, make sure that the quota type > flags are the same between the incore dquot and the ondisk dquot. > The changes look good to me. Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/xfs_dquot.c | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) > > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > index d5b7f03e93c8..46c8ca83c04d 100644 > --- a/fs/xfs/xfs_dquot.c > +++ b/fs/xfs/xfs_dquot.c > @@ -524,13 +524,27 @@ xfs_dquot_alloc( > } > > /* Copy the in-core quota fields in from the on-disk buffer. */ > -STATIC void > +STATIC int > xfs_dquot_from_disk( > struct xfs_dquot *dqp, > struct xfs_buf *bp) > { > struct xfs_disk_dquot *ddqp = bp->b_addr + dqp->q_bufoffset; > > + /* > + * The only field the verifier didn't check was the quota type flag, so > + * do that here. > + */ > + if ((dqp->dq_flags & XFS_DQ_ALLTYPES) != > + (ddqp->d_flags & XFS_DQ_ALLTYPES) || > + dqp->q_core.d_id != ddqp->d_id) { > + xfs_alert(bp->b_mount, > + "Metadata corruption detected at %pS, quota %u", > + __this_address, be32_to_cpu(dqp->q_core.d_id)); > + xfs_alert(bp->b_mount, "Unmount and run xfs_repair"); > + return -EFSCORRUPTED; > + } > + > /* copy everything from disk dquot to the incore dquot */ > memcpy(&dqp->q_core, ddqp, sizeof(struct xfs_disk_dquot)); > > @@ -544,6 +558,7 @@ xfs_dquot_from_disk( > > /* initialize the dquot speculative prealloc thresholds */ > xfs_dquot_set_prealloc_limits(dqp); > + return 0; > } > > /* Allocate and initialize the dquot buffer for this in-core dquot. */ > @@ -617,9 +632,11 @@ xfs_qm_dqread( > * further. > */ > ASSERT(xfs_buf_islocked(bp)); > - xfs_dquot_from_disk(dqp, bp); > - > + error = xfs_dquot_from_disk(dqp, bp); > xfs_buf_relse(bp); > + if (error) > + goto err; > + > *dqpp = dqp; > return error; > > >
On Tue, Jun 30, 2020 at 08:42:09AM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > While loading dquot records off disk, make sure that the quota type > flags are the same between the incore dquot and the ondisk dquot. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/xfs_dquot.c | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) > > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > index d5b7f03e93c8..46c8ca83c04d 100644 > --- a/fs/xfs/xfs_dquot.c > +++ b/fs/xfs/xfs_dquot.c > @@ -524,13 +524,27 @@ xfs_dquot_alloc( > } > > /* Copy the in-core quota fields in from the on-disk buffer. */ > -STATIC void > +STATIC int > xfs_dquot_from_disk( > struct xfs_dquot *dqp, > struct xfs_buf *bp) > { > struct xfs_disk_dquot *ddqp = bp->b_addr + dqp->q_bufoffset; > > + /* > + * The only field the verifier didn't check was the quota type flag, so > + * do that here. > + */ > + if ((dqp->dq_flags & XFS_DQ_ALLTYPES) != > + (ddqp->d_flags & XFS_DQ_ALLTYPES) || > + dqp->q_core.d_id != ddqp->d_id) { The comment looks a little weird, as this also checks d_id. Also xfs_dquot_verify verifies d_flags against generally bogus value, it just doesn't check that it matches the type we are looking for. Last but not least dqp->dq_flags only contains the type at this point. So what about something like: /* * Ensure we got the type and ID we were looking for. Everything else * we checked by the verifier. */ if ((ddqp->d_flags & XFS_DQ_ALLTYPES) != dqp->dq_flags || ddqp->d_id != dqp->q_core.d_id)
On Wed, Jul 01, 2020 at 09:42:08AM +0100, Christoph Hellwig wrote: > On Tue, Jun 30, 2020 at 08:42:09AM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > While loading dquot records off disk, make sure that the quota type > > flags are the same between the incore dquot and the ondisk dquot. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > fs/xfs/xfs_dquot.c | 23 ++++++++++++++++++++--- > > 1 file changed, 20 insertions(+), 3 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > > index d5b7f03e93c8..46c8ca83c04d 100644 > > --- a/fs/xfs/xfs_dquot.c > > +++ b/fs/xfs/xfs_dquot.c > > @@ -524,13 +524,27 @@ xfs_dquot_alloc( > > } > > > > /* Copy the in-core quota fields in from the on-disk buffer. */ > > -STATIC void > > +STATIC int > > xfs_dquot_from_disk( > > struct xfs_dquot *dqp, > > struct xfs_buf *bp) > > { > > struct xfs_disk_dquot *ddqp = bp->b_addr + dqp->q_bufoffset; > > > > + /* > > + * The only field the verifier didn't check was the quota type flag, so > > + * do that here. > > + */ > > + if ((dqp->dq_flags & XFS_DQ_ALLTYPES) != > > + (ddqp->d_flags & XFS_DQ_ALLTYPES) || > > + dqp->q_core.d_id != ddqp->d_id) { > > The comment looks a little weird, as this also checks d_id. Also > xfs_dquot_verify verifies d_flags against generally bogus value, it > just doesn't check that it matches the type we are looking for. > Last but not least dqp->dq_flags only contains the type at this > point. > > So what about something like: > > /* > * Ensure we got the type and ID we were looking for. Everything else > * we checked by the verifier. > */ > if ((ddqp->d_flags & XFS_DQ_ALLTYPES) != dqp->dq_flags || > ddqp->d_id != dqp->q_core.d_id) Sounds good to me. I'll make that change. --D >
On Tue, Jun 30, 2020 at 08:42:09AM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > While loading dquot records off disk, make sure that the quota type > flags are the same between the incore dquot and the ondisk dquot. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/xfs_dquot.c | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) > > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > index d5b7f03e93c8..46c8ca83c04d 100644 > --- a/fs/xfs/xfs_dquot.c > +++ b/fs/xfs/xfs_dquot.c > @@ -524,13 +524,27 @@ xfs_dquot_alloc( > } > > /* Copy the in-core quota fields in from the on-disk buffer. */ > -STATIC void > +STATIC int > xfs_dquot_from_disk( > struct xfs_dquot *dqp, > struct xfs_buf *bp) > { > struct xfs_disk_dquot *ddqp = bp->b_addr + dqp->q_bufoffset; > > + /* > + * The only field the verifier didn't check was the quota type flag, so > + * do that here. > + */ > + if ((dqp->dq_flags & XFS_DQ_ALLTYPES) != > + (ddqp->d_flags & XFS_DQ_ALLTYPES) || > + dqp->q_core.d_id != ddqp->d_id) { > + xfs_alert(bp->b_mount, > + "Metadata corruption detected at %pS, quota %u", > + __this_address, be32_to_cpu(dqp->q_core.d_id)); Probably should indicate which quota type is invalid, too. Also, looking at xfs_buf_corruption_error(), it also uses xfs_alert_tag(mp, XFS_PTAG_VERIFIER_ERROR, .... Should that be used here, too? Cheers, Dave.
On Thu, Jul 02, 2020 at 08:41:12AM +1000, Dave Chinner wrote: > On Tue, Jun 30, 2020 at 08:42:09AM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > While loading dquot records off disk, make sure that the quota type > > flags are the same between the incore dquot and the ondisk dquot. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > fs/xfs/xfs_dquot.c | 23 ++++++++++++++++++++--- > > 1 file changed, 20 insertions(+), 3 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > > index d5b7f03e93c8..46c8ca83c04d 100644 > > --- a/fs/xfs/xfs_dquot.c > > +++ b/fs/xfs/xfs_dquot.c > > @@ -524,13 +524,27 @@ xfs_dquot_alloc( > > } > > > > /* Copy the in-core quota fields in from the on-disk buffer. */ > > -STATIC void > > +STATIC int > > xfs_dquot_from_disk( > > struct xfs_dquot *dqp, > > struct xfs_buf *bp) > > { > > struct xfs_disk_dquot *ddqp = bp->b_addr + dqp->q_bufoffset; > > > > + /* > > + * The only field the verifier didn't check was the quota type flag, so > > + * do that here. > > + */ > > + if ((dqp->dq_flags & XFS_DQ_ALLTYPES) != > > + (ddqp->d_flags & XFS_DQ_ALLTYPES) || > > + dqp->q_core.d_id != ddqp->d_id) { > > + xfs_alert(bp->b_mount, > > + "Metadata corruption detected at %pS, quota %u", > > + __this_address, be32_to_cpu(dqp->q_core.d_id)); > > Probably should indicate which quota type is invalid, too. Also, > looking at xfs_buf_corruption_error(), it also uses > > xfs_alert_tag(mp, XFS_PTAG_VERIFIER_ERROR, .... > > Should that be used here, too? Yeah. Will fix. --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
On Wed, Jul 01, 2020 at 11:25:08AM -0700, Darrick J. Wong wrote: > > /* > > * Ensure we got the type and ID we were looking for. Everything else > > * we checked by the verifier. > > */ > > if ((ddqp->d_flags & XFS_DQ_ALLTYPES) != dqp->dq_flags || > > ddqp->d_id != dqp->q_core.d_id) > > Sounds good to me. I'll make that change. We also don't need the mask on the on-disk flags, as it never contains anything but the type, so this can be further simplified.
On Thu, Jul 02, 2020 at 07:30:21AM +0100, Christoph Hellwig wrote: > On Wed, Jul 01, 2020 at 11:25:08AM -0700, Darrick J. Wong wrote: > > > /* > > > * Ensure we got the type and ID we were looking for. Everything else > > > * we checked by the verifier. > > > */ > > > if ((ddqp->d_flags & XFS_DQ_ALLTYPES) != dqp->dq_flags || > > > ddqp->d_id != dqp->q_core.d_id) > > > > Sounds good to me. I'll make that change. > > We also don't need the mask on the on-disk flags, as it never contains > anything but the type, so this can be further simplified. d_flags will contain more than the type Real Soon Now; I was planning to send out the y2038 feature patchset (at least for another RFC) right after landing this series. --D
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c index d5b7f03e93c8..46c8ca83c04d 100644 --- a/fs/xfs/xfs_dquot.c +++ b/fs/xfs/xfs_dquot.c @@ -524,13 +524,27 @@ xfs_dquot_alloc( } /* Copy the in-core quota fields in from the on-disk buffer. */ -STATIC void +STATIC int xfs_dquot_from_disk( struct xfs_dquot *dqp, struct xfs_buf *bp) { struct xfs_disk_dquot *ddqp = bp->b_addr + dqp->q_bufoffset; + /* + * The only field the verifier didn't check was the quota type flag, so + * do that here. + */ + if ((dqp->dq_flags & XFS_DQ_ALLTYPES) != + (ddqp->d_flags & XFS_DQ_ALLTYPES) || + dqp->q_core.d_id != ddqp->d_id) { + xfs_alert(bp->b_mount, + "Metadata corruption detected at %pS, quota %u", + __this_address, be32_to_cpu(dqp->q_core.d_id)); + xfs_alert(bp->b_mount, "Unmount and run xfs_repair"); + return -EFSCORRUPTED; + } + /* copy everything from disk dquot to the incore dquot */ memcpy(&dqp->q_core, ddqp, sizeof(struct xfs_disk_dquot)); @@ -544,6 +558,7 @@ xfs_dquot_from_disk( /* initialize the dquot speculative prealloc thresholds */ xfs_dquot_set_prealloc_limits(dqp); + return 0; } /* Allocate and initialize the dquot buffer for this in-core dquot. */ @@ -617,9 +632,11 @@ xfs_qm_dqread( * further. */ ASSERT(xfs_buf_islocked(bp)); - xfs_dquot_from_disk(dqp, bp); - + error = xfs_dquot_from_disk(dqp, bp); xfs_buf_relse(bp); + if (error) + goto err; + *dqpp = dqp; return error;