[PATCH] xfs: fix error handling in xfs_qm_log_quotaoff()

[PATCH] xfs: fix error handling in xfs_qm_log_quotaoff()

[Brian Foster]

The error handling in xfs_qm_log_quotaoff() has a couple problems. If
xfs_trans_commit() fails, we fall through to the error block and call
xfs_trans_cancel(). This is incorrect on commit failure. If
xfs_trans_reserve() fails, we jump to the error block, cancel the tp and
restore the superblock qflags to oldsbqflag. However, oldsbqflag has
been initialized to zero and not yet updated from the original flags so
we set the flags to zero.

Fix up the error handling in xfs_qm_log_quotaoff() to not restore flags
if they haven't been modified and not cancel the tp on commit failure.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_qm_syscalls.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 80f2d77..f2d454e 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -784,13 +784,17 @@ xfs_qm_log_quotaoff(
 {
 	xfs_trans_t	       *tp;
 	int			error;
-	xfs_qoff_logitem_t     *qoffi=NULL;
-	uint			oldsbqflag=0;
+	xfs_qoff_logitem_t     *qoffi;
+	uint			oldsbqflag;
+
+	*qoffstartp = NULL;
 
 	tp = xfs_trans_alloc(mp, XFS_TRANS_QM_QUOTAOFF);
 	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_qm_quotaoff, 0, 0);
-	if (error)
-		goto error0;
+	if (error) {
+		xfs_trans_cancel(tp, 0);
+		return error;
+	}
 
 	qoffi = xfs_trans_get_qoff_item(tp, NULL, flags & XFS_ALL_QUOTA_ACCT);
 	xfs_trans_log_quotaoff_item(tp, qoffi);
@@ -809,19 +813,20 @@ xfs_qm_log_quotaoff(
 	 */
 	xfs_trans_set_sync(tp);
 	error = xfs_trans_commit(tp, 0);
+	if (error)
+		goto error_flags;
 
-error0:
-	if (error) {
-		xfs_trans_cancel(tp, 0);
-		/*
-		 * No one else is modifying sb_qflags, so this is OK.
-		 * We still hold the quotaofflock.
-		 */
-		spin_lock(&mp->m_sb_lock);
-		mp->m_sb.sb_qflags = oldsbqflag;
-		spin_unlock(&mp->m_sb_lock);
-	}
 	*qoffstartp = qoffi;
+	return 0;
+
+error_flags:
+	/*
+	 * No one else is modifying sb_qflags, so this is OK.
+	 * We still hold the quotaofflock.
+	 */
+	spin_lock(&mp->m_sb_lock);
+	mp->m_sb.sb_qflags = oldsbqflag;
+	spin_unlock(&mp->m_sb_lock);
 	return error;
 }
 
-- 
1.8.3.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix error handling in xfs_qm_log_quotaoff()

[Dave Chinner]

On Thu, Oct 16, 2014 at 09:39:33AM -0400, Brian Foster wrote:
> The error handling in xfs_qm_log_quotaoff() has a couple problems. If
> xfs_trans_commit() fails, we fall through to the error block and call
> xfs_trans_cancel(). This is incorrect on commit failure. If
> xfs_trans_reserve() fails, we jump to the error block, cancel the tp and
> restore the superblock qflags to oldsbqflag. However, oldsbqflag has
> been initialized to zero and not yet updated from the original flags so
> we set the flags to zero.
> 
> Fix up the error handling in xfs_qm_log_quotaoff() to not restore flags
> if they haven't been modified and not cancel the tp on commit failure.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_qm_syscalls.c | 35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index 80f2d77..f2d454e 100644
> --- a/fs/xfs/xfs_qm_syscalls.c
> +++ b/fs/xfs/xfs_qm_syscalls.c
> @@ -784,13 +784,17 @@ xfs_qm_log_quotaoff(
>  {
>  	xfs_trans_t	       *tp;
>  	int			error;
> -	xfs_qoff_logitem_t     *qoffi=NULL;
> -	uint			oldsbqflag=0;
> +	xfs_qoff_logitem_t     *qoffi;
> +	uint			oldsbqflag;
> +
> +	*qoffstartp = NULL;
>  
>  	tp = xfs_trans_alloc(mp, XFS_TRANS_QM_QUOTAOFF);
>  	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_qm_quotaoff, 0, 0);
> -	if (error)
> -		goto error0;
> +	if (error) {
> +		xfs_trans_cancel(tp, 0);
> +		return error;
> +	}
>  
>  	qoffi = xfs_trans_get_qoff_item(tp, NULL, flags & XFS_ALL_QUOTA_ACCT);
>  	xfs_trans_log_quotaoff_item(tp, qoffi);
> @@ -809,19 +813,20 @@ xfs_qm_log_quotaoff(
>  	 */
>  	xfs_trans_set_sync(tp);
>  	error = xfs_trans_commit(tp, 0);
> +	if (error)
> +		goto error_flags;


If the commit fails, the filesystem will be shut down and the state
of the quota flags is completely irrelevant at this point so there's
no reason to restore them. Indeed, is restoring them even the right
thing to do? The commit *may* have made it to disk, but a
subsequent error during completion handling resulted in the commit
failing....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix error handling in xfs_qm_log_quotaoff()

[Brian Foster]

On Mon, Oct 20, 2014 at 11:31:20AM +1100, Dave Chinner wrote:
> On Thu, Oct 16, 2014 at 09:39:33AM -0400, Brian Foster wrote:
> > The error handling in xfs_qm_log_quotaoff() has a couple problems. If
> > xfs_trans_commit() fails, we fall through to the error block and call
> > xfs_trans_cancel(). This is incorrect on commit failure. If
> > xfs_trans_reserve() fails, we jump to the error block, cancel the tp and
> > restore the superblock qflags to oldsbqflag. However, oldsbqflag has
> > been initialized to zero and not yet updated from the original flags so
> > we set the flags to zero.
> > 
> > Fix up the error handling in xfs_qm_log_quotaoff() to not restore flags
> > if they haven't been modified and not cancel the tp on commit failure.
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/xfs_qm_syscalls.c | 35 ++++++++++++++++++++---------------
> >  1 file changed, 20 insertions(+), 15 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> > index 80f2d77..f2d454e 100644
> > --- a/fs/xfs/xfs_qm_syscalls.c
> > +++ b/fs/xfs/xfs_qm_syscalls.c
> > @@ -784,13 +784,17 @@ xfs_qm_log_quotaoff(
> >  {
> >  	xfs_trans_t	       *tp;
> >  	int			error;
> > -	xfs_qoff_logitem_t     *qoffi=NULL;
> > -	uint			oldsbqflag=0;
> > +	xfs_qoff_logitem_t     *qoffi;
> > +	uint			oldsbqflag;
> > +
> > +	*qoffstartp = NULL;
> >  
> >  	tp = xfs_trans_alloc(mp, XFS_TRANS_QM_QUOTAOFF);
> >  	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_qm_quotaoff, 0, 0);
> > -	if (error)
> > -		goto error0;
> > +	if (error) {
> > +		xfs_trans_cancel(tp, 0);
> > +		return error;
> > +	}
> >  
> >  	qoffi = xfs_trans_get_qoff_item(tp, NULL, flags & XFS_ALL_QUOTA_ACCT);
> >  	xfs_trans_log_quotaoff_item(tp, qoffi);
> > @@ -809,19 +813,20 @@ xfs_qm_log_quotaoff(
> >  	 */
> >  	xfs_trans_set_sync(tp);
> >  	error = xfs_trans_commit(tp, 0);
> > +	if (error)
> > +		goto error_flags;
> 
> 
> If the commit fails, the filesystem will be shut down and the state
> of the quota flags is completely irrelevant at this point so there's
> no reason to restore them. Indeed, is restoring them even the right
> thing to do? The commit *may* have made it to disk, but a
> subsequent error during completion handling resulted in the commit
> failing....
> 

Good point. We don't seem to try and undo this kind of state change
anywhere else that I can see either. At most, we release references or
free memory or other such things that aren't tied to the transaction in
any way.

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs