[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>

[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>
Dave Chinner · Lachlan McIlroy
1 parent 6307091fe6
Showing 8 changed files with 66 additions and 19 deletions Side-by-side Diff
fs/xfs/xfs_bmap.c
fs/xfs/xfs_inode.c
fs/xfs/xfs_log.c
fs/xfs/xfs_log.h
fs/xfs/xfs_log_priv.h
fs/xfs/xfs_trans.c
fs/xfs/xfs_utils.c
fs/xfs/xfs_vnodeops.c
@@ -4292,9 +4292,15 @@
 	 * 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;
@@ -1782,8 +1782,14 @@
 		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);
@@ -100,12 +100,11 @@
  
  
 /* 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 @@
 		 */
 		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 @@
 		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 @@
 		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 @@
  */
  
 /*
- * 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 @@
 		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_tic_reset_res(tic);
  
 	return tic;
-}	/* xlog_ticket_get */
+}
  
  
 /******************************************************************************
@@ -134,6 +134,7 @@
 #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,
@@ -176,6 +177,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
  
@@ -245,6 +245,7 @@
 	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  */
@@ -290,7 +290,7 @@
 	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;
@@ -1258,6 +1258,13 @@
 		return (error);
  
 	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.
@@ -172,6 +172,12 @@
 			*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);
 		/*
@@ -1019,6 +1019,12 @@
 		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).
 	 */
 	if (ip->i_df.if_bytes)
...	...	@@ -4292,9 +4292,15 @@
4292	4292	* We have a new transaction, so we should return committed=1,
4293	4293	* even though we're returning an error.
4294	4294	*/
4295		- if (error) {
	4295	+ if (error)
4296	4296	return error;
4297		- }
	4297	+
	4298	+ /*
	4299	+ * transaction commit worked ok so we can drop the extra ticket
	4300	+ * reference that we gained in xfs_trans_dup()
	4301	+ */
	4302	+ xfs_log_ticket_put(ntp->t_ticket);
	4303	+
4298	4304	if ((error = xfs_trans_reserve(ntp, 0, logres, 0, XFS_TRANS_PERM_LOG_RES,
4299	4305	logcount)))
4300	4306	return error;
...	...	@@ -1782,8 +1782,14 @@
1782	1782	xfs_trans_ijoin(ntp, ip, XFS_ILOCK_EXCL \| XFS_IOLOCK_EXCL);
1783	1783	xfs_trans_ihold(ntp, ip);
1784	1784
1785		- if (!error)
1786		- error = xfs_trans_reserve(ntp, 0,
	1785	+ if (error)
	1786	+ return error;
	1787	+ /*
	1788	+ * transaction commit worked ok so we can drop the extra ticket
	1789	+ * reference that we gained in xfs_trans_dup()
	1790	+ */
	1791	+ xfs_log_ticket_put(ntp->t_ticket);
	1792	+ error = xfs_trans_reserve(ntp, 0,
1787	1793	XFS_ITRUNCATE_LOG_RES(mp), 0,
1788	1794	XFS_TRANS_PERM_LOG_RES,
1789	1795	XFS_ITRUNCATE_LOG_COUNT);
...	...	@@ -100,12 +100,11 @@
100	100
101	101
102	102	/* local ticket functions */
103		-STATIC xlog_ticket_t xlog_ticket_get(xlog_t log,
	103	+STATIC xlog_ticket_t xlog_ticket_alloc(xlog_t log,
104	104	int unit_bytes,
105	105	int count,
106	106	char clientid,
107	107	uint flags);
108		-STATIC void xlog_ticket_put(xlog_t log, xlog_ticket_t ticket);
109	108
110	109	#if defined(DEBUG)
111	110	STATIC void xlog_verify_dest_ptr(xlog_t *log, __psint_t ptr);
...	...	@@ -360,7 +359,7 @@
360	359	*/
361	360	xlog_trace_loggrant(log, ticket, "xfs_log_done: (non-permanent)");
362	361	xlog_ungrant_log_space(log, ticket);
363		- xlog_ticket_put(log, ticket);
	362	+ xfs_log_ticket_put(ticket);
364	363	} else {
365	364	xlog_trace_loggrant(log, ticket, "xfs_log_done: (permanent)");
366	365	xlog_regrant_reserve_log_space(log, ticket);
...	...	@@ -514,7 +513,7 @@
514	513	retval = xlog_regrant_write_log_space(log, internal_ticket);
515	514	} else {
516	515	/* may sleep if need to allocate more tickets */
517		- internal_ticket = xlog_ticket_get(log, unit_bytes, cnt,
	516	+ internal_ticket = xlog_ticket_alloc(log, unit_bytes, cnt,
518	517	client, flags);
519	518	if (!internal_ticket)
520	519	return XFS_ERROR(ENOMEM);
...	...	@@ -749,7 +748,7 @@
749	748	if (tic) {
750	749	xlog_trace_loggrant(log, tic, "unmount rec");
751	750	xlog_ungrant_log_space(log, tic);
752		- xlog_ticket_put(log, tic);
	751	+ xfs_log_ticket_put(tic);
753	752	}
754	753	} else {
755	754	/*
756	755
757	756
758	757
759	758
...	...	@@ -3222,22 +3221,33 @@
3222	3221	*/
3223	3222
3224	3223	/*
3225		- * Free a used ticket.
	3224	+ * Free a used ticket when it's refcount falls to zero.
3226	3225	*/
3227		-STATIC void
3228		-xlog_ticket_put(xlog_t *log,
3229		- xlog_ticket_t *ticket)
	3226	+void
	3227	+xfs_log_ticket_put(
	3228	+ xlog_ticket_t *ticket)
3230	3229	{
3231		- sv_destroy(&ticket->t_wait);
3232		- kmem_zone_free(xfs_log_ticket_zone, ticket);
3233		-} /* xlog_ticket_put */
	3230	+ ASSERT(atomic_read(&ticket->t_ref) > 0);
	3231	+ if (atomic_dec_and_test(&ticket->t_ref)) {
	3232	+ sv_destroy(&ticket->t_wait);
	3233	+ kmem_zone_free(xfs_log_ticket_zone, ticket);
	3234	+ }
	3235	+}
3234	3236
	3237	+xlog_ticket_t *
	3238	+xfs_log_ticket_get(
	3239	+ xlog_ticket_t *ticket)
	3240	+{
	3241	+ ASSERT(atomic_read(&ticket->t_ref) > 0);
	3242	+ atomic_inc(&ticket->t_ref);
	3243	+ return ticket;
	3244	+}
3235	3245
3236	3246	/*
3237	3247	* Allocate and initialise a new log ticket.
3238	3248	*/
3239	3249	STATIC xlog_ticket_t *
3240		-xlog_ticket_get(xlog_t *log,
	3250	+xlog_ticket_alloc(xlog_t *log,
3241	3251	int unit_bytes,
3242	3252	int cnt,
3243	3253	char client,
...	...	@@ -3308,6 +3318,7 @@
3308	3318	unit_bytes += 2*BBSIZE;
3309	3319	}
3310	3320
	3321	+ atomic_set(&tic->t_ref, 1);
3311	3322	tic->t_unit_res = unit_bytes;
3312	3323	tic->t_curr_res = unit_bytes;
3313	3324	tic->t_cnt = cnt;
...	...	@@ -3323,7 +3334,7 @@
3323	3334	xlog_tic_reset_res(tic);
3324	3335
3325	3336	return tic;
3326		-} /* xlog_ticket_get */
	3337	+}
3327	3338
3328	3339
3329	3340	/******************************************************************************
...	...	@@ -134,6 +134,7 @@
134	134	#ifdef __KERNEL__
135	135	/* Log manager interfaces */
136	136	struct xfs_mount;
	137	+struct xlog_ticket;
137	138	xfs_lsn_t xfs_log_done(struct xfs_mount *mp,
138	139	xfs_log_ticket_t ticket,
139	140	void **iclog,
...	...	@@ -176,6 +177,9 @@
176	177	int xfs_log_need_covered(struct xfs_mount *mp);
177	178
178	179	void xlog_iodone(struct xfs_buf *);
	180	+
	181	+struct xlog_ticket * xfs_log_ticket_get(struct xlog_ticket *ticket);
	182	+void xfs_log_ticket_put(struct xlog_ticket *ticket);
179	183
180	184	#endif
181	185
...	...	@@ -245,6 +245,7 @@
245	245	struct xlog_ticket t_next; / :4\|8 */
246	246	struct xlog_ticket t_prev; / :4\|8 */
247	247	xlog_tid_t t_tid; /* transaction identifier : 4 */
	248	+ atomic_t t_ref; /* ticket reference count : 4 */
248	249	int t_curr_res; /* current reservation in bytes : 4 */
249	250	int t_unit_res; /* unit reservation in bytes : 4 */
250	251	char t_ocnt; /* original count : 1 */
...	...	@@ -290,7 +290,7 @@
290	290	ASSERT(tp->t_ticket != NULL);
291	291
292	292	ntp->t_flags = XFS_TRANS_PERM_LOG_RES \| (tp->t_flags & XFS_TRANS_RESERVE);
293		- ntp->t_ticket = tp->t_ticket;
	293	+ ntp->t_ticket = xfs_log_ticket_get(tp->t_ticket);
294	294	ntp->t_blk_res = tp->t_blk_res - tp->t_blk_res_used;
295	295	tp->t_blk_res = tp->t_blk_res_used;
296	296	ntp->t_rtx_res = tp->t_rtx_res - tp->t_rtx_res_used;
...	...	@@ -1258,6 +1258,13 @@
1258	1258	return (error);
1259	1259
1260	1260	trans = *tpp;
	1261	+
	1262	+ /*
	1263	+ * transaction commit worked ok so we can drop the extra ticket
	1264	+ * reference that we gained in xfs_trans_dup()
	1265	+ */
	1266	+ xfs_log_ticket_put(trans->t_ticket);
	1267	+
1261	1268
1262	1269	/*
1263	1270	* Reserve space in the log for th next transaction.
...	...	@@ -172,6 +172,12 @@
172	172	*ipp = NULL;
173	173	return code;
174	174	}
	175	+
	176	+ /*
	177	+ * transaction commit worked ok so we can drop the extra ticket
	178	+ * reference that we gained in xfs_trans_dup()
	179	+ */
	180	+ xfs_log_ticket_put(tp->t_ticket);
175	181	code = xfs_trans_reserve(tp, 0, log_res, 0,
176	182	XFS_TRANS_PERM_LOG_RES, log_count);
177	183	/*
...	...	@@ -1019,6 +1019,12 @@
1019	1019	goto error0;
1020	1020	}
1021	1021	/*
	1022	+ * transaction commit worked ok so we can drop the extra ticket
	1023	+ * reference that we gained in xfs_trans_dup()
	1024	+ */
	1025	+ xfs_log_ticket_put(tp->t_ticket);
	1026	+
	1027	+ /*
1022	1028	* Remove the memory for extent descriptions (just bookkeeping).
1023	1029	*/
1024	1030	if (ip->i_df.if_bytes)