diff mbox series

[03/18] xfs: validate ondisk/incore dquot flags

Message ID 159353172899.2864738.6438709598863248951.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs: remove xfs_disk_quot from incore dquot | expand

Commit Message

Darrick J. Wong June 30, 2020, 3:42 p.m. UTC 
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(-)

Comments

Allison Henderson June 30, 2020, 9:35 p.m. UTC | #1 
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;
>   
>
Chandan Babu R July 1, 2020, 8:33 a.m. UTC | #2 
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;
>  
> 
>
Christoph Hellwig July 1, 2020, 8:42 a.m. UTC | #3 
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)
Darrick J. Wong July 1, 2020, 6:25 p.m. UTC | #4 
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

>
Dave Chinner July 1, 2020, 10:41 p.m. UTC | #5 
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.
Darrick J. Wong July 1, 2020, 11:16 p.m. UTC | #6 
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
Christoph Hellwig July 2, 2020, 6:30 a.m. UTC | #7 
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.
Darrick J. Wong July 2, 2020, 3:13 p.m. UTC | #8 
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 mbox series

Patch

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;