xfs: fix error handling in xfs_qm_log_quotaoff()
PATCH 1/1 xfs: fix error handling in xfs_qm_log_quotaoff()
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.
--- fs/xfs/xfs_qm_syscalls.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-)
fs/xfs/xfs_qm_syscalls.c +20 -15
@@ -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; }
Signature · 6 lines
1.8.3.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 44 lines · Brian Foster
The error handling in
xfs_qm_log_quotaoff()has a couple problems. Ifxfs_trans_commit()fails, we fall through to the error block and callxfs_trans_cancel(). This is incorrect on commit failure. Ifxfs_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 <[email protected]>Signed-off-by Brian Foster <[email protected]>--- fs/xfs/xfs_qm_syscalls.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-)fs/xfs/xfs_qm_syscalls.c +8
-4@@ -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);
fs/xfs/xfs_qm_syscalls.c xfs_qm_log_quotaoff( · line 817
Quoted 5 lines
fs/xfs/xfs_qm_syscalls.c +2
-0@@ -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.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 42 lines · Dave Chinner
Quoted 41 lines · Brian Foster
The error handling in
xfs_qm_log_quotaoff()has a couple problems. Ifxfs_trans_commit()fails, we fall through to the error block and callxfs_trans_cancel(). This is incorrect on commit failure. Ifxfs_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 <[email protected]>Signed-off-by Brian Foster <[email protected]>--- fs/xfs/xfs_qm_syscalls.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-)fs/xfs/xfs_qm_syscalls.c +8
-4@@ -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; + }
Quoted 17 lines
fs/xfs/xfs_qm_syscalls.c xfs_qm_log_quotaoff( · line 817
Quoted 8 lines
fs/xfs/xfs_qm_syscalls.c +2
-0qoffi =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
Quoted 11 lines
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs