[XFS] Fix double free of log tickets

When an I/O error occurs during an intermediate commit on a rolling
transaction, xfs_trans_commit() will free the transaction structure
and the related ticket. However, the duplicate transaction that
gets used as the transaction continues still contains a pointer
to the ticket. Hence when the duplicate transaction is cancelled
and freed, we free the ticket a second time.

Add reference counting to the ticket so that we hold an extra
reference to the ticket over the transaction commit. We drop the
extra reference once we have checked that the transaction commit
did not return an error, thus avoiding a double free on commit
error.

Credit to Nick Piggin for tripping over the problem.

SGI-PV: 989741

Signed-off-by: Dave Chinner <david@fromorbit.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Lachlan McIlroy <lachlan@sgi.com>

diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index db289050692f4fe4a95c791d427151b92c92fbdf..c3912213645c6a5e17637f2b9f57441de8c8d9c9 100644 (file)
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -4292,9 +4292,15 @@ xfs_bmap_finish(
         * We have a new transaction, so we should return committed=1,
         * even though we're returning an error.
         */
-       if (error) {
+       if (error)
                return error;
-       }
+
+       /*
+        * transaction commit worked ok so we can drop the extra ticket
+        * reference that we gained in xfs_trans_dup()
+        */
+       xfs_log_ticket_put(ntp->t_ticket);
+
        if ((error = xfs_trans_reserve(ntp, 0, logres, 0, XFS_TRANS_PERM_LOG_RES,
                        logcount)))
                return error;

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index cd522827f99e3c7f9fb6185bd3034170f659c13c..b9771004706228d96a467e0708ec2f741c481b38 100644 (file)
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1782,8 +1782,14 @@ xfs_itruncate_finish(
                xfs_trans_ijoin(ntp, ip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
                xfs_trans_ihold(ntp, ip);
 
-               if (!error)
-                       error = xfs_trans_reserve(ntp, 0,
+               if (error)
+                       return error;
+               /*
+                * transaction commit worked ok so we can drop the extra ticket
+                * reference that we gained in xfs_trans_dup()
+                */
+               xfs_log_ticket_put(ntp->t_ticket);
+               error = xfs_trans_reserve(ntp, 0,
                                        XFS_ITRUNCATE_LOG_RES(mp), 0,
                                        XFS_TRANS_PERM_LOG_RES,
                                        XFS_ITRUNCATE_LOG_COUNT);

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 92c20a8d9e69c80388c64c58b3b64d69d9a6ae74..4bf44aef644c5d1cfdf84faa68e0d5e673de7707 100644 (file)
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -100,12 +100,11 @@ STATIC void xlog_ungrant_log_space(xlog_t  *log,
 
 
 /* local ticket functions */
-STATIC xlog_ticket_t   *xlog_ticket_get(xlog_t *log,
+STATIC xlog_ticket_t   *xlog_ticket_alloc(xlog_t *log,
                                         int    unit_bytes,
                                         int    count,
                                         char   clientid,
                                         uint   flags);
-STATIC void            xlog_ticket_put(xlog_t *log, xlog_ticket_t *ticket);
 
 #if defined(DEBUG)
 STATIC void    xlog_verify_dest_ptr(xlog_t *log, __psint_t ptr);
@@ -360,7 +359,7 @@ xfs_log_done(xfs_mount_t    *mp,
                 */
                xlog_trace_loggrant(log, ticket, "xfs_log_done: (non-permanent)");
                xlog_ungrant_log_space(log, ticket);
-               xlog_ticket_put(log, ticket);
+               xfs_log_ticket_put(ticket);
        } else {
                xlog_trace_loggrant(log, ticket, "xfs_log_done: (permanent)");
                xlog_regrant_reserve_log_space(log, ticket);
@@ -514,7 +513,7 @@ xfs_log_reserve(xfs_mount_t  *mp,
                retval = xlog_regrant_write_log_space(log, internal_ticket);
        } else {
                /* may sleep if need to allocate more tickets */
-               internal_ticket = xlog_ticket_get(log, unit_bytes, cnt,
+               internal_ticket = xlog_ticket_alloc(log, unit_bytes, cnt,
                                                  client, flags);
                if (!internal_ticket)
                        return XFS_ERROR(ENOMEM);
@@ -749,7 +748,7 @@ xfs_log_unmount_write(xfs_mount_t *mp)
                if (tic) {
                        xlog_trace_loggrant(log, tic, "unmount rec");
                        xlog_ungrant_log_space(log, tic);
-                       xlog_ticket_put(log, tic);
+                       xfs_log_ticket_put(tic);
                }
        } else {
                /*
@@ -3222,22 +3221,33 @@ xlog_state_want_sync(xlog_t *log, xlog_in_core_t *iclog)
  */
 
 /*
- * Free a used ticket.
+ * Free a used ticket when it's refcount falls to zero.
  */
-STATIC void
-xlog_ticket_put(xlog_t         *log,
-               xlog_ticket_t   *ticket)
+void
+xfs_log_ticket_put(
+       xlog_ticket_t   *ticket)
 {
-       sv_destroy(&ticket->t_wait);
-       kmem_zone_free(xfs_log_ticket_zone, ticket);
-}      /* xlog_ticket_put */
+       ASSERT(atomic_read(&ticket->t_ref) > 0);
+       if (atomic_dec_and_test(&ticket->t_ref)) {
+               sv_destroy(&ticket->t_wait);
+               kmem_zone_free(xfs_log_ticket_zone, ticket);
+       }
+}
 
+xlog_ticket_t *
+xfs_log_ticket_get(
+       xlog_ticket_t   *ticket)
+{
+       ASSERT(atomic_read(&ticket->t_ref) > 0);
+       atomic_inc(&ticket->t_ref);
+       return ticket;
+}
 
 /*
  * Allocate and initialise a new log ticket.
  */
 STATIC xlog_ticket_t *
-xlog_ticket_get(xlog_t         *log,
+xlog_ticket_alloc(xlog_t               *log,
                int             unit_bytes,
                int             cnt,
                char            client,
@@ -3308,6 +3318,7 @@ xlog_ticket_get(xlog_t            *log,
                unit_bytes += 2*BBSIZE;
         }
 
+       atomic_set(&tic->t_ref, 1);
        tic->t_unit_res         = unit_bytes;
        tic->t_curr_res         = unit_bytes;
        tic->t_cnt              = cnt;
@@ -3323,7 +3334,7 @@ xlog_ticket_get(xlog_t            *log,
        xlog_tic_reset_res(tic);
 
        return tic;
-}      /* xlog_ticket_get */
+}
 
 
 /******************************************************************************

diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
index d47b91f10822b98400a6d11961a8a12d4e930d76..8a3e84e900a34e5153a453eae896bb795576a57b 100644 (file)
--- a/fs/xfs/xfs_log.h
+++ b/fs/xfs/xfs_log.h
@@ -134,6 +134,7 @@ typedef struct xfs_log_callback {
 #ifdef __KERNEL__
 /* Log manager interfaces */
 struct xfs_mount;
+struct xlog_ticket;
 xfs_lsn_t xfs_log_done(struct xfs_mount *mp,
                       xfs_log_ticket_t ticket,
                       void             **iclog,
@@ -177,6 +178,9 @@ int   xfs_log_need_covered(struct xfs_mount *mp);
 
 void     xlog_iodone(struct xfs_buf *);
 
+struct xlog_ticket * xfs_log_ticket_get(struct xlog_ticket *ticket);
+void     xfs_log_ticket_put(struct xlog_ticket *ticket);
+
 #endif
 
 

diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index de7ef6ca9206d797ea7b9007b2e4a30e2178dffc..b39a1980e82d1faa4eef0784afa7a0420e769947 100644 (file)
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -245,6 +245,7 @@ typedef struct xlog_ticket {
        struct xlog_ticket *t_next;      /*                              :4|8 */
        struct xlog_ticket *t_prev;      /*                              :4|8 */
        xlog_tid_t         t_tid;        /* transaction identifier       : 4  */
+       atomic_t           t_ref;        /* ticket reference count       : 4  */
        int                t_curr_res;   /* current reservation in bytes : 4  */
        int                t_unit_res;   /* unit reservation in bytes    : 4  */
        char               t_ocnt;       /* original count               : 1  */

diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index ad137efc87020561f208cbbd3a382f98fcff24f7..8570b826fedd85e2375831aebc754f18e3685ab1 100644 (file)
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -290,7 +290,7 @@ xfs_trans_dup(
        ASSERT(tp->t_ticket != NULL);
 
        ntp->t_flags = XFS_TRANS_PERM_LOG_RES | (tp->t_flags & XFS_TRANS_RESERVE);
-       ntp->t_ticket = tp->t_ticket;
+       ntp->t_ticket = xfs_log_ticket_get(tp->t_ticket);
        ntp->t_blk_res = tp->t_blk_res - tp->t_blk_res_used;
        tp->t_blk_res = tp->t_blk_res_used;
        ntp->t_rtx_res = tp->t_rtx_res - tp->t_rtx_res_used;
@@ -1259,6 +1259,13 @@ xfs_trans_roll(
 
        trans = *tpp;
 
+       /*
+        * transaction commit worked ok so we can drop the extra ticket
+        * reference that we gained in xfs_trans_dup()
+        */
+       xfs_log_ticket_put(trans->t_ticket);
+
+
        /*
         * Reserve space in the log for th next transaction.
         * This also pushes items in the "AIL", the list of logged items,

diff --git a/fs/xfs/xfs_utils.c b/fs/xfs/xfs_utils.c
index 35d4d414bcc273ca0981fad6ed4dddbbf8a9ea42..771144932ab42ef08b21e68d1d7bc3c3db5d29df 100644 (file)
--- a/fs/xfs/xfs_utils.c
+++ b/fs/xfs/xfs_utils.c
@@ -172,6 +172,12 @@ xfs_dir_ialloc(
                        *ipp = NULL;
                        return code;
                }
+
+               /*
+                * transaction commit worked ok so we can drop the extra ticket
+                * reference that we gained in xfs_trans_dup()
+                */
+               xfs_log_ticket_put(tp->t_ticket);
                code = xfs_trans_reserve(tp, 0, log_res, 0,
                                         XFS_TRANS_PERM_LOG_RES, log_count);
                /*

diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
index c45ea278ef414b79c292e180ff8b6a8233435010..0574aadc4d3cfe287d8a88e23e82b11814225831 100644 (file)
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -1018,6 +1018,12 @@ xfs_inactive_symlink_rmt(
                ASSERT(XFS_FORCED_SHUTDOWN(mp));
                goto error0;
        }
+       /*
+        * transaction commit worked ok so we can drop the extra ticket
+        * reference that we gained in xfs_trans_dup()
+        */
+       xfs_log_ticket_put(tp->t_ticket);
+
        /*
         * Remove the memory for extent descriptions (just bookkeeping).
         */