xfs: use after free on error in xfs_qm_log_quotaoff()

[Dan Carpenter]

This bug is still around.

regards,
dan carpenter

Smatch complains here. I don't think I have reported this one before.
It feels very familiar but I have grepped my outbox.
fs/xfs/xfs_qm_syscalls.c:814 xfs_qm_log_quotaoff()
warn: 'tp' was already freed.
fs/xfs/xfs_qm_syscalls.c
804 /*
805 * We have to make sure that the transaction is secure on disk before we
806 * return and actually stop quota accounting. So, make it synchronous.
807 * We don't care about quotoff's performance.
808 */
809 xfs_trans_set_sync(tp);
810 error = xfs_trans_commit(tp, 0);
^^^^^^^^^^^^^^^^
This function frees tp.
811
813 if (error) {
814 xfs_trans_cancel(tp, 0);
^^^^^^^^^^^^^^^^^
Use after free.
815 /*
816 * No one else is modifying sb_qflags, so this is OK.
817 * We still hold the quotaofflock.
818 */
regards,
dan carpenter

[Brian Foster]

Post by Dan Carpenter
This bug is still around.
regards,
dan carpenter

Smatch complains here. I don't think I have reported this one before.
It feels very familiar but I have grepped my outbox.
fs/xfs/xfs_qm_syscalls.c:814 xfs_qm_log_quotaoff()
warn: 'tp' was already freed.

Heh, looks like this function also has a problem restoring flags. If the
reserve fails we restore flags to 0 rather than the old flags. I'll send
a patch shortly.

Brian

Post by Dan Carpenter

fs/xfs/xfs_qm_syscalls.c
804 /*
805 * We have to make sure that the transaction is secure on disk before we
806 * return and actually stop quota accounting. So, make it synchronous.
807 * We don't care about quotoff's performance.
808 */
809 xfs_trans_set_sync(tp);
810 error = xfs_trans_commit(tp, 0);
^^^^^^^^^^^^^^^^
This function frees tp.
811
813 if (error) {
814 xfs_trans_cancel(tp, 0);
^^^^^^^^^^^^^^^^^
Use after free.
815 /*
816 * No one else is modifying sb_qflags, so this is OK.
817 * We still hold the quotaofflock.
818 */
regards,
dan carpenter

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