[PATCH 0/7] fs, xfs subsystem refcounter conversions

[PATCH 0/7] fs, xfs subsystem refcounter conversions

[Elena Reshetova]

Now when new refcount_t type and API are finally merged
(see include/linux/refcount.h), the following
patches convert various refcounters in the fs/xfs susystem from atomic_t
to refcount_t. By doing this we prevent intentional or accidental
underflows or overflows that can led to use-after-free vulnerabilities.

The below patches are fully independent and can be cherry-picked separately.
Since we convert all kernel subsystems in the same fashion, resulting
in about 300 patches, we have to group them for sending at least in some
fashion to be manageable. Please excuse the long cc list.

Elena Reshetova (7):
  fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to
    refcount_t
  fs, xfs: convert xfs_buf.b_hold and xfs_buf.b_lru_ref from atomic_t to
    refcount_t
  fs, xfs: convert xfs_buf_log_item.bli_refcount from atomic_t to
    refcount_t
  fs, xfs: convert xfs_efi_log_item.efi_refcount from atomic_t to
    refcount_t
  fs, xfs: convert xlog_ticket.t_ref from atomic_t to refcount_t
  fs, xfs: convert xfs_cui_log_item.cui_refcount from atomic_t to
    refcount_t
  fs, xfs: convert xfs_rui_log_item.rui_refcount from atomic_t to
    refcount_t

 fs/xfs/xfs_bmap_item.c     |  4 ++--
 fs/xfs/xfs_bmap_item.h     |  4 +++-
 fs/xfs/xfs_buf.c           | 35 ++++++++++++++++++-----------------
 fs/xfs/xfs_buf.h           |  7 ++++---
 fs/xfs/xfs_buf_item.c      | 16 ++++++++--------
 fs/xfs/xfs_buf_item.h      |  4 +++-
 fs/xfs/xfs_extfree_item.c  |  4 ++--
 fs/xfs/xfs_extfree_item.h  |  4 +++-
 fs/xfs/xfs_log.c           | 10 +++++-----
 fs/xfs/xfs_log_priv.h      |  4 +++-
 fs/xfs/xfs_refcount_item.c |  4 ++--
 fs/xfs/xfs_refcount_item.h |  4 +++-
 fs/xfs/xfs_rmap_item.c     |  4 ++--
 fs/xfs/xfs_rmap_item.h     |  4 +++-
 fs/xfs/xfs_trace.h         | 10 +++++-----
 fs/xfs/xfs_trans_buf.c     | 32 ++++++++++++++++----------------
 16 files changed, 82 insertions(+), 68 deletions(-)

-- 
2.7.4

[PATCH 1/7] fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to refcount_t

[Elena Reshetova]

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 fs/xfs/xfs_bmap_item.c | 4 ++--
 fs/xfs/xfs_bmap_item.h | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 9bf57c7..33104ad 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -199,7 +199,7 @@ xfs_bui_init(
 	buip->bui_format.bui_nextents = XFS_BUI_MAX_FAST_EXTENTS;
 	buip->bui_format.bui_id = (uintptr_t)(void *)buip;
 	atomic_set(&buip->bui_next_extent, 0);
-	atomic_set(&buip->bui_refcount, 2);
+	refcount_set(&buip->bui_refcount, 2);
 
 	return buip;
 }
@@ -215,7 +215,7 @@ void
 xfs_bui_release(
 	struct xfs_bui_log_item	*buip)
 {
-	if (atomic_dec_and_test(&buip->bui_refcount)) {
+	if (refcount_dec_and_test(&buip->bui_refcount)) {
 		xfs_trans_ail_remove(&buip->bui_item, SHUTDOWN_LOG_IO_ERROR);
 		xfs_bui_item_free(buip);
 	}
diff --git a/fs/xfs/xfs_bmap_item.h b/fs/xfs/xfs_bmap_item.h
index c867daa..988a6ae 100644
--- a/fs/xfs/xfs_bmap_item.h
+++ b/fs/xfs/xfs_bmap_item.h
@@ -20,6 +20,8 @@
 #ifndef	__XFS_BMAP_ITEM_H__
 #define	__XFS_BMAP_ITEM_H__
 
+#include <linux/refcount.h>
+
 /*
  * There are (currently) two pairs of bmap btree redo item types: map & unmap.
  * The common abbreviations for these are BUI (bmap update intent) and BUD
@@ -61,7 +63,7 @@ struct kmem_zone;
  */
 struct xfs_bui_log_item {
 	struct xfs_log_item		bui_item;
-	atomic_t			bui_refcount;
+	refcount_t			bui_refcount;
 	atomic_t			bui_next_extent;
 	unsigned long			bui_flags;	/* misc flags */
 	struct xfs_bui_log_format	bui_format;
-- 
2.7.4

[PATCH 2/7] fs, xfs: convert xfs_buf.b_hold and xfs_buf.b_lru_ref from atomic_t to refcount_t

[Elena Reshetova]

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 fs/xfs/xfs_buf.c   | 35 ++++++++++++++++++-----------------
 fs/xfs/xfs_buf.h   |  7 ++++---
 fs/xfs/xfs_trace.h |  8 ++++----
 3 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 8c7d01b..21a09c1 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -151,12 +151,12 @@ xfs_buf_stale(
 	xfs_buf_ioacct_dec(bp);
 
 	spin_lock(&bp->b_lock);
-	atomic_set(&bp->b_lru_ref, 0);
+	refcount_set(&bp->b_lru_ref, 0);
 	if (!(bp->b_state & XFS_BSTATE_DISPOSE) &&
 	    (list_lru_del(&bp->b_target->bt_lru, &bp->b_lru)))
-		atomic_dec(&bp->b_hold);
+		refcount_dec(&bp->b_hold);
 
-	ASSERT(atomic_read(&bp->b_hold) >= 1);
+	ASSERT(refcount_read(&bp->b_hold) >= 1);
 	spin_unlock(&bp->b_lock);
 }
 
@@ -214,8 +214,8 @@ _xfs_buf_alloc(
 	 */
 	flags &= ~(XBF_UNMAPPED | XBF_TRYLOCK | XBF_ASYNC | XBF_READ_AHEAD);
 
-	atomic_set(&bp->b_hold, 1);
-	atomic_set(&bp->b_lru_ref, 1);
+	refcount_set(&bp->b_hold, 1);
+	refcount_set(&bp->b_lru_ref, 1);
 	init_completion(&bp->b_iowait);
 	INIT_LIST_HEAD(&bp->b_lru);
 	INIT_LIST_HEAD(&bp->b_list);
@@ -581,7 +581,7 @@ _xfs_buf_find(
 	bp = rhashtable_lookup_fast(&pag->pag_buf_hash, &cmap,
 				    xfs_buf_hash_params);
 	if (bp) {
-		atomic_inc(&bp->b_hold);
+		refcount_inc(&bp->b_hold);
 		goto found;
 	}
 
@@ -940,7 +940,7 @@ xfs_buf_hold(
 	xfs_buf_t		*bp)
 {
 	trace_xfs_buf_hold(bp, _RET_IP_);
-	atomic_inc(&bp->b_hold);
+	refcount_inc(&bp->b_hold);
 }
 
 /*
@@ -959,16 +959,16 @@ xfs_buf_rele(
 
 	if (!pag) {
 		ASSERT(list_empty(&bp->b_lru));
-		if (atomic_dec_and_test(&bp->b_hold)) {
+		if (refcount_dec_and_test(&bp->b_hold)) {
 			xfs_buf_ioacct_dec(bp);
 			xfs_buf_free(bp);
 		}
 		return;
 	}
 
-	ASSERT(atomic_read(&bp->b_hold) > 0);
+	ASSERT(refcount_read(&bp->b_hold) > 0);
 
-	release = atomic_dec_and_lock(&bp->b_hold, &pag->pag_buf_lock);
+	release = refcount_dec_and_lock(&bp->b_hold, &pag->pag_buf_lock);
 	spin_lock(&bp->b_lock);
 	if (!release) {
 		/*
@@ -977,14 +977,14 @@ xfs_buf_rele(
 		 * haven't acquired the pag lock, but the use of _XBF_IN_FLIGHT
 		 * ensures the decrement occurs only once per-buf.
 		 */
-		if ((atomic_read(&bp->b_hold) == 1) && !list_empty(&bp->b_lru))
+		if ((refcount_read(&bp->b_hold) == 1) && !list_empty(&bp->b_lru))
 			xfs_buf_ioacct_dec(bp);
 		goto out_unlock;
 	}
 
 	/* the last reference has been dropped ... */
 	xfs_buf_ioacct_dec(bp);
-	if (!(bp->b_flags & XBF_STALE) && atomic_read(&bp->b_lru_ref)) {
+	if (!(bp->b_flags & XBF_STALE) && refcount_read(&bp->b_lru_ref)) {
 		/*
 		 * If the buffer is added to the LRU take a new reference to the
 		 * buffer for the LRU and clear the (now stale) dispose list
@@ -992,7 +992,7 @@ xfs_buf_rele(
 		 */
 		if (list_lru_add(&bp->b_target->bt_lru, &bp->b_lru)) {
 			bp->b_state &= ~XFS_BSTATE_DISPOSE;
-			atomic_inc(&bp->b_hold);
+			refcount_inc(&bp->b_hold);
 		}
 		spin_unlock(&pag->pag_buf_lock);
 	} else {
@@ -1598,7 +1598,7 @@ xfs_buftarg_wait_rele(
 	struct xfs_buf		*bp = container_of(item, struct xfs_buf, b_lru);
 	struct list_head	*dispose = arg;
 
-	if (atomic_read(&bp->b_hold) > 1) {
+	if (refcount_read(&bp->b_hold) > 1) {
 		/* need to wait, so skip it this pass */
 		trace_xfs_buf_wait_buftarg(bp, _RET_IP_);
 		return LRU_SKIP;
@@ -1610,7 +1610,7 @@ xfs_buftarg_wait_rele(
 	 * clear the LRU reference count so the buffer doesn't get
 	 * ignored in xfs_buf_rele().
 	 */
-	atomic_set(&bp->b_lru_ref, 0);
+	refcount_set(&bp->b_lru_ref, 0);
 	bp->b_state |= XFS_BSTATE_DISPOSE;
 	list_lru_isolate_move(lru, item, dispose);
 	spin_unlock(&bp->b_lock);
@@ -1684,10 +1684,11 @@ xfs_buftarg_isolate(
 	 * zero. If the value is already zero, we need to reclaim the
 	 * buffer, otherwise it gets another trip through the LRU.
 	 */
-	if (!atomic_add_unless(&bp->b_lru_ref, -1, 0)) {
+	if (!refcount_read(&bp->b_lru_ref)) {
 		spin_unlock(&bp->b_lock);
 		return LRU_ROTATE;
 	}
+	refcount_dec_and_test(&bp->b_lru_ref);
 
 	bp->b_state |= XFS_BSTATE_DISPOSE;
 	list_lru_isolate_move(lru, item, dispose);
@@ -1854,7 +1855,7 @@ xfs_buf_delwri_queue(
 	 */
 	bp->b_flags |= _XBF_DELWRI_Q;
 	if (list_empty(&bp->b_list)) {
-		atomic_inc(&bp->b_hold);
+		refcount_inc(&bp->b_hold);
 		list_add_tail(&bp->b_list, list);
 	}
 
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 3c867e5..7373246 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -27,6 +27,7 @@
 #include <linux/buffer_head.h>
 #include <linux/uio.h>
 #include <linux/list_lru.h>
+#include <linux/refcount.h>
 
 /*
  *	Base types
@@ -153,8 +154,8 @@ typedef struct xfs_buf {
 	struct rhash_head	b_rhash_head;	/* pag buffer hash node */
 	xfs_daddr_t		b_bn;		/* block number of buffer */
 	int			b_length;	/* size of buffer in BBs */
-	atomic_t		b_hold;		/* reference count */
-	atomic_t		b_lru_ref;	/* lru reclaim ref count */
+	refcount_t		b_hold;		/* reference count */
+	refcount_t		b_lru_ref;	/* lru reclaim ref count */
 	xfs_buf_flags_t		b_flags;	/* status flags */
 	struct semaphore	b_sema;		/* semaphore for lockables */
 
@@ -353,7 +354,7 @@ extern void xfs_buf_terminate(void);
 
 static inline void xfs_buf_set_ref(struct xfs_buf *bp, int lru_ref)
 {
-	atomic_set(&bp->b_lru_ref, lru_ref);
+	refcount_set(&bp->b_lru_ref, lru_ref);
 }
 
 static inline int xfs_buf_ispinned(struct xfs_buf *bp)
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 383ac22..8fc98d5 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -326,7 +326,7 @@ DECLARE_EVENT_CLASS(xfs_buf_class,
 		__entry->dev = bp->b_target->bt_dev;
 		__entry->bno = bp->b_bn;
 		__entry->nblks = bp->b_length;
-		__entry->hold = atomic_read(&bp->b_hold);
+		__entry->hold = refcount_read(&bp->b_hold);
 		__entry->pincount = atomic_read(&bp->b_pin_count);
 		__entry->lockval = bp->b_sema.count;
 		__entry->flags = bp->b_flags;
@@ -395,7 +395,7 @@ DECLARE_EVENT_CLASS(xfs_buf_flags_class,
 		__entry->bno = bp->b_bn;
 		__entry->buffer_length = BBTOB(bp->b_length);
 		__entry->flags = flags;
-		__entry->hold = atomic_read(&bp->b_hold);
+		__entry->hold = refcount_read(&bp->b_hold);
 		__entry->pincount = atomic_read(&bp->b_pin_count);
 		__entry->lockval = bp->b_sema.count;
 		__entry->caller_ip = caller_ip;
@@ -438,7 +438,7 @@ TRACE_EVENT(xfs_buf_ioerror,
 		__entry->dev = bp->b_target->bt_dev;
 		__entry->bno = bp->b_bn;
 		__entry->buffer_length = BBTOB(bp->b_length);
-		__entry->hold = atomic_read(&bp->b_hold);
+		__entry->hold = refcount_read(&bp->b_hold);
 		__entry->pincount = atomic_read(&bp->b_pin_count);
 		__entry->lockval = bp->b_sema.count;
 		__entry->error = error;
@@ -483,7 +483,7 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
 		__entry->buf_bno = bip->bli_buf->b_bn;
 		__entry->buf_len = BBTOB(bip->bli_buf->b_length);
 		__entry->buf_flags = bip->bli_buf->b_flags;
-		__entry->buf_hold = atomic_read(&bip->bli_buf->b_hold);
+		__entry->buf_hold = refcount_read(&bip->bli_buf->b_hold);
 		__entry->buf_pincount = atomic_read(&bip->bli_buf->b_pin_count);
 		__entry->buf_lockval = bip->bli_buf->b_sema.count;
 		__entry->li_desc = bip->bli_item.li_desc;
-- 
2.7.4

[PATCH 3/7] fs, xfs: convert xfs_buf_log_item.bli_refcount from atomic_t to refcount_t

[Elena Reshetova]

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 fs/xfs/xfs_buf_item.c  | 16 ++++++++--------
 fs/xfs/xfs_buf_item.h  |  4 +++-
 fs/xfs/xfs_trace.h     |  2 +-
 fs/xfs/xfs_trans_buf.c | 32 ++++++++++++++++----------------
 4 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 0306168..f6063ad 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -137,7 +137,7 @@ xfs_buf_item_size(
 	struct xfs_buf_log_item	*bip = BUF_ITEM(lip);
 	int			i;
 
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 	if (bip->bli_flags & XFS_BLI_STALE) {
 		/*
 		 * The buffer is stale, so all we need to log
@@ -316,7 +316,7 @@ xfs_buf_item_format(
 	uint			offset = 0;
 	int			i;
 
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 	ASSERT((bip->bli_flags & XFS_BLI_LOGGED) ||
 	       (bip->bli_flags & XFS_BLI_STALE));
 	ASSERT((bip->bli_flags & XFS_BLI_STALE) ||
@@ -383,14 +383,14 @@ xfs_buf_item_pin(
 {
 	struct xfs_buf_log_item	*bip = BUF_ITEM(lip);
 
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 	ASSERT((bip->bli_flags & XFS_BLI_LOGGED) ||
 	       (bip->bli_flags & XFS_BLI_ORDERED) ||
 	       (bip->bli_flags & XFS_BLI_STALE));
 
 	trace_xfs_buf_item_pin(bip);
 
-	atomic_inc(&bip->bli_refcount);
+	refcount_inc(&bip->bli_refcount);
 	atomic_inc(&bip->bli_buf->b_pin_count);
 }
 
@@ -419,11 +419,11 @@ xfs_buf_item_unpin(
 	int		freed;
 
 	ASSERT(bp->b_fspriv == bip);
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 
 	trace_xfs_buf_item_unpin(bip);
 
-	freed = atomic_dec_and_test(&bip->bli_refcount);
+	freed = refcount_dec_and_test(&bip->bli_refcount);
 
 	if (atomic_dec_and_test(&bp->b_pin_count))
 		wake_up_all(&bp->b_waiters);
@@ -605,7 +605,7 @@ xfs_buf_item_unlock(
 		trace_xfs_buf_item_unlock_stale(bip);
 		ASSERT(bip->__bli_format.blf_flags & XFS_BLF_CANCEL);
 		if (!aborted) {
-			atomic_dec(&bip->bli_refcount);
+			refcount_dec(&bip->bli_refcount);
 			return;
 		}
 	}
@@ -642,7 +642,7 @@ xfs_buf_item_unlock(
 	 * it in this case, because an aborted transaction has already shut the
 	 * filesystem down and this is the last chance we will have to do so.
 	 */
-	if (atomic_dec_and_test(&bip->bli_refcount)) {
+	if (refcount_dec_and_test(&bip->bli_refcount)) {
 		if (clean)
 			xfs_buf_item_relse(bp);
 		else if (aborted) {
diff --git a/fs/xfs/xfs_buf_item.h b/fs/xfs/xfs_buf_item.h
index f7eba99..7bbdef7 100644
--- a/fs/xfs/xfs_buf_item.h
+++ b/fs/xfs/xfs_buf_item.h
@@ -18,6 +18,8 @@
 #ifndef	__XFS_BUF_ITEM_H__
 #define	__XFS_BUF_ITEM_H__
 
+#include <linux/refcount.h>
+
 /* kernel only definitions */
 
 /* buf log item flags */
@@ -55,7 +57,7 @@ typedef struct xfs_buf_log_item {
 	struct xfs_buf		*bli_buf;	/* real buffer pointer */
 	unsigned int		bli_flags;	/* misc flags */
 	unsigned int		bli_recur;	/* lock recursion count */
-	atomic_t		bli_refcount;	/* cnt of tp refs */
+	refcount_t		bli_refcount;	/* cnt of tp refs */
 	int			bli_format_count;	/* count of headers */
 	struct xfs_buf_log_format *bli_formats;	/* array of in-log header ptrs */
 	struct xfs_buf_log_format __bli_format;	/* embedded in-log header */
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 8fc98d5..4afd160 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -479,7 +479,7 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
 		__entry->dev = bip->bli_buf->b_target->bt_dev;
 		__entry->bli_flags = bip->bli_flags;
 		__entry->bli_recur = bip->bli_recur;
-		__entry->bli_refcount = atomic_read(&bip->bli_refcount);
+		__entry->bli_refcount = refcount_read(&bip->bli_refcount);
 		__entry->buf_bno = bip->bli_buf->b_bn;
 		__entry->buf_len = BBTOB(bip->bli_buf->b_length);
 		__entry->buf_flags = bip->bli_buf->b_flags;
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index 8ee29ca..fa7f213 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -97,7 +97,7 @@ _xfs_trans_bjoin(
 	/*
 	 * Take a reference for this transaction on the buf item.
 	 */
-	atomic_inc(&bip->bli_refcount);
+	refcount_inc(&bip->bli_refcount);
 
 	/*
 	 * Get a log_item_desc to point at the new item.
@@ -161,7 +161,7 @@ xfs_trans_get_buf_map(
 		ASSERT(bp->b_transp == tp);
 		bip = bp->b_fspriv;
 		ASSERT(bip != NULL);
-		ASSERT(atomic_read(&bip->bli_refcount) > 0);
+		ASSERT(refcount_read(&bip->bli_refcount) > 0);
 		bip->bli_recur++;
 		trace_xfs_trans_get_buf_recur(bip);
 		return bp;
@@ -212,7 +212,7 @@ xfs_trans_getsb(xfs_trans_t	*tp,
 	if (bp->b_transp == tp) {
 		bip = bp->b_fspriv;
 		ASSERT(bip != NULL);
-		ASSERT(atomic_read(&bip->bli_refcount) > 0);
+		ASSERT(refcount_read(&bip->bli_refcount) > 0);
 		bip->bli_recur++;
 		trace_xfs_trans_getsb_recur(bip);
 		return bp;
@@ -282,7 +282,7 @@ xfs_trans_read_buf_map(
 		bip = bp->b_fspriv;
 		bip->bli_recur++;
 
-		ASSERT(atomic_read(&bip->bli_refcount) > 0);
+		ASSERT(refcount_read(&bip->bli_refcount) > 0);
 		trace_xfs_trans_read_buf_recur(bip);
 		*bpp = bp;
 		return 0;
@@ -371,7 +371,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
 	ASSERT(bip->bli_item.li_type == XFS_LI_BUF);
 	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
 	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 
 	trace_xfs_trans_brelse(bip);
 
@@ -419,7 +419,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
 	/*
 	 * Drop our reference to the buf log item.
 	 */
-	atomic_dec(&bip->bli_refcount);
+	refcount_dec(&bip->bli_refcount);
 
 	/*
 	 * If the buf item is not tracking data in the log, then
@@ -432,7 +432,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
 /***
 		ASSERT(bp->b_pincount == 0);
 ***/
-		ASSERT(atomic_read(&bip->bli_refcount) == 0);
+		ASSERT(refcount_read(&bip->bli_refcount) == 0);
 		ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
 		ASSERT(!(bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF));
 		xfs_buf_item_relse(bp);
@@ -458,7 +458,7 @@ xfs_trans_bhold(xfs_trans_t	*tp,
 	ASSERT(bip != NULL);
 	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
 	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 
 	bip->bli_flags |= XFS_BLI_HOLD;
 	trace_xfs_trans_bhold(bip);
@@ -478,7 +478,7 @@ xfs_trans_bhold_release(xfs_trans_t	*tp,
 	ASSERT(bip != NULL);
 	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
 	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 	ASSERT(bip->bli_flags & XFS_BLI_HOLD);
 
 	bip->bli_flags &= ~XFS_BLI_HOLD;
@@ -520,7 +520,7 @@ xfs_trans_log_buf(xfs_trans_t	*tp,
 	 */
 	bp->b_flags |= XBF_DONE;
 
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 	bp->b_iodone = xfs_buf_iodone_callbacks;
 	bip->bli_item.li_cb = xfs_buf_iodone;
 
@@ -591,7 +591,7 @@ xfs_trans_binval(
 
 	ASSERT(bp->b_transp == tp);
 	ASSERT(bip != NULL);
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 
 	trace_xfs_trans_binval(bip);
 
@@ -645,7 +645,7 @@ xfs_trans_inode_buf(
 
 	ASSERT(bp->b_transp == tp);
 	ASSERT(bip != NULL);
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 
 	bip->bli_flags |= XFS_BLI_INODE_BUF;
 	xfs_trans_buf_set_type(tp, bp, XFS_BLFT_DINO_BUF);
@@ -669,7 +669,7 @@ xfs_trans_stale_inode_buf(
 
 	ASSERT(bp->b_transp == tp);
 	ASSERT(bip != NULL);
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 
 	bip->bli_flags |= XFS_BLI_STALE_INODE;
 	bip->bli_item.li_cb = xfs_buf_iodone;
@@ -694,7 +694,7 @@ xfs_trans_inode_alloc_buf(
 
 	ASSERT(bp->b_transp == tp);
 	ASSERT(bip != NULL);
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 
 	bip->bli_flags |= XFS_BLI_INODE_ALLOC_BUF;
 	xfs_trans_buf_set_type(tp, bp, XFS_BLFT_DINO_BUF);
@@ -717,7 +717,7 @@ xfs_trans_ordered_buf(
 
 	ASSERT(bp->b_transp == tp);
 	ASSERT(bip != NULL);
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 
 	bip->bli_flags |= XFS_BLI_ORDERED;
 	trace_xfs_buf_item_ordered(bip);
@@ -740,7 +740,7 @@ xfs_trans_buf_set_type(
 
 	ASSERT(bp->b_transp == tp);
 	ASSERT(bip != NULL);
-	ASSERT(atomic_read(&bip->bli_refcount) > 0);
+	ASSERT(refcount_read(&bip->bli_refcount) > 0);
 
 	xfs_blft_to_flags(&bip->__bli_format, type);
 }
-- 
2.7.4

[PATCH 4/7] fs, xfs: convert xfs_efi_log_item.efi_refcount from atomic_t to refcount_t

[Elena Reshetova]

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 fs/xfs/xfs_extfree_item.c | 4 ++--
 fs/xfs/xfs_extfree_item.h | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index d7bc149..4e0acf0 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -220,7 +220,7 @@ xfs_efi_init(
 	efip->efi_format.efi_nextents = nextents;
 	efip->efi_format.efi_id = (uintptr_t)(void *)efip;
 	atomic_set(&efip->efi_next_extent, 0);
-	atomic_set(&efip->efi_refcount, 2);
+	refcount_set(&efip->efi_refcount, 2);
 
 	return efip;
 }
@@ -290,7 +290,7 @@ void
 xfs_efi_release(
 	struct xfs_efi_log_item	*efip)
 {
-	if (atomic_dec_and_test(&efip->efi_refcount)) {
+	if (refcount_dec_and_test(&efip->efi_refcount)) {
 		xfs_trans_ail_remove(&efip->efi_item, SHUTDOWN_LOG_IO_ERROR);
 		xfs_efi_item_free(efip);
 	}
diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
index a32c794..e6da63d 100644
--- a/fs/xfs/xfs_extfree_item.h
+++ b/fs/xfs/xfs_extfree_item.h
@@ -18,6 +18,8 @@
 #ifndef	__XFS_EXTFREE_ITEM_H__
 #define	__XFS_EXTFREE_ITEM_H__
 
+#include <linux/refcount.h>
+
 /* kernel only EFI/EFD definitions */
 
 struct xfs_mount;
@@ -64,7 +66,7 @@ struct kmem_zone;
  */
 typedef struct xfs_efi_log_item {
 	xfs_log_item_t		efi_item;
-	atomic_t		efi_refcount;
+	refcount_t		efi_refcount;
 	atomic_t		efi_next_extent;
 	unsigned long		efi_flags;	/* misc flags */
 	xfs_efi_log_format_t	efi_format;
-- 
2.7.4

[PATCH 5/7] fs, xfs: convert xlog_ticket.t_ref from atomic_t to refcount_t

[Elena Reshetova]

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 fs/xfs/xfs_log.c      | 10 +++++-----
 fs/xfs/xfs_log_priv.h |  4 +++-
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index b1469f0..c127fa0 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3500,8 +3500,8 @@ void
 xfs_log_ticket_put(
 	xlog_ticket_t	*ticket)
 {
-	ASSERT(atomic_read(&ticket->t_ref) > 0);
-	if (atomic_dec_and_test(&ticket->t_ref))
+	ASSERT(refcount_read(&ticket->t_ref) > 0);
+	if (refcount_dec_and_test(&ticket->t_ref))
 		kmem_zone_free(xfs_log_ticket_zone, ticket);
 }
 
@@ -3509,8 +3509,8 @@ xlog_ticket_t *
 xfs_log_ticket_get(
 	xlog_ticket_t	*ticket)
 {
-	ASSERT(atomic_read(&ticket->t_ref) > 0);
-	atomic_inc(&ticket->t_ref);
+	ASSERT(refcount_read(&ticket->t_ref) > 0);
+	refcount_inc(&ticket->t_ref);
 	return ticket;
 }
 
@@ -3632,7 +3632,7 @@ xlog_ticket_alloc(
 
 	unit_res = xfs_log_calc_unit_res(log->l_mp, unit_bytes);
 
-	atomic_set(&tic->t_ref, 1);
+	refcount_set(&tic->t_ref, 1);
 	tic->t_task		= current;
 	INIT_LIST_HEAD(&tic->t_queue);
 	tic->t_unit_res		= unit_res;
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index c2604a5..279afce 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -18,6 +18,8 @@
 #ifndef	__XFS_LOG_PRIV_H__
 #define __XFS_LOG_PRIV_H__
 
+#include <linux/refcount.h>
+
 struct xfs_buf;
 struct xlog;
 struct xlog_ticket;
@@ -168,7 +170,7 @@ typedef struct xlog_ticket {
 	struct list_head   t_queue;	 /* reserve/write queue */
 	struct task_struct *t_task;	 /* task that owns this ticket */
 	xlog_tid_t	   t_tid;	 /* transaction identifier	 : 4  */
-	atomic_t	   t_ref;	 /* ticket reference count       : 4  */
+	refcount_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  */
-- 
2.7.4

[PATCH 6/7] fs, xfs: convert xfs_cui_log_item.cui_refcount from atomic_t to refcount_t

[Elena Reshetova]

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 fs/xfs/xfs_refcount_item.c | 4 ++--
 fs/xfs/xfs_refcount_item.h | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 6e4c744..61bc570 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -205,7 +205,7 @@ xfs_cui_init(
 	cuip->cui_format.cui_nextents = nextents;
 	cuip->cui_format.cui_id = (uintptr_t)(void *)cuip;
 	atomic_set(&cuip->cui_next_extent, 0);
-	atomic_set(&cuip->cui_refcount, 2);
+	refcount_set(&cuip->cui_refcount, 2);
 
 	return cuip;
 }
@@ -221,7 +221,7 @@ void
 xfs_cui_release(
 	struct xfs_cui_log_item	*cuip)
 {
-	if (atomic_dec_and_test(&cuip->cui_refcount)) {
+	if (refcount_dec_and_test(&cuip->cui_refcount)) {
 		xfs_trans_ail_remove(&cuip->cui_item, SHUTDOWN_LOG_IO_ERROR);
 		xfs_cui_item_free(cuip);
 	}
diff --git a/fs/xfs/xfs_refcount_item.h b/fs/xfs/xfs_refcount_item.h
index 5b74ddd..7f23ff8 100644
--- a/fs/xfs/xfs_refcount_item.h
+++ b/fs/xfs/xfs_refcount_item.h
@@ -20,6 +20,8 @@
 #ifndef	__XFS_REFCOUNT_ITEM_H__
 #define	__XFS_REFCOUNT_ITEM_H__
 
+#include <linux/refcount.h>
+
 /*
  * There are (currently) two pairs of refcount btree redo item types:
  * increase and decrease.  The log items for these are CUI (refcount
@@ -63,7 +65,7 @@ struct kmem_zone;
  */
 struct xfs_cui_log_item {
 	struct xfs_log_item		cui_item;
-	atomic_t			cui_refcount;
+	refcount_t			cui_refcount;
 	atomic_t			cui_next_extent;
 	unsigned long			cui_flags;	/* misc flags */
 	struct xfs_cui_log_format	cui_format;
-- 
2.7.4

[PATCH 7/7] fs, xfs: convert xfs_rui_log_item.rui_refcount from atomic_t to refcount_t

[Elena Reshetova]

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 fs/xfs/xfs_rmap_item.c | 4 ++--
 fs/xfs/xfs_rmap_item.h | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 73c8278..5e1664a 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -204,7 +204,7 @@ xfs_rui_init(
 	ruip->rui_format.rui_nextents = nextents;
 	ruip->rui_format.rui_id = (uintptr_t)(void *)ruip;
 	atomic_set(&ruip->rui_next_extent, 0);
-	atomic_set(&ruip->rui_refcount, 2);
+	refcount_set(&ruip->rui_refcount, 2);
 
 	return ruip;
 }
@@ -243,7 +243,7 @@ void
 xfs_rui_release(
 	struct xfs_rui_log_item	*ruip)
 {
-	if (atomic_dec_and_test(&ruip->rui_refcount)) {
+	if (refcount_dec_and_test(&ruip->rui_refcount)) {
 		xfs_trans_ail_remove(&ruip->rui_item, SHUTDOWN_LOG_IO_ERROR);
 		xfs_rui_item_free(ruip);
 	}
diff --git a/fs/xfs/xfs_rmap_item.h b/fs/xfs/xfs_rmap_item.h
index 340c968..2529a35 100644
--- a/fs/xfs/xfs_rmap_item.h
+++ b/fs/xfs/xfs_rmap_item.h
@@ -20,6 +20,8 @@
 #ifndef	__XFS_RMAP_ITEM_H__
 #define	__XFS_RMAP_ITEM_H__
 
+#include <linux/refcount.h>
+
 /*
  * There are (currently) three pairs of rmap btree redo item types: map, unmap,
  * and convert.  The common abbreviations for these are RUI (rmap update
@@ -64,7 +66,7 @@ struct kmem_zone;
  */
 struct xfs_rui_log_item {
 	struct xfs_log_item		rui_item;
-	atomic_t			rui_refcount;
+	refcount_t			rui_refcount;
 	atomic_t			rui_next_extent;
 	unsigned long			rui_flags;	/* misc flags */
 	struct xfs_rui_log_format	rui_format;
-- 
2.7.4

Re: [PATCH 3/7] fs, xfs: convert xfs_buf_log_item.bli_refcount from atomic_t to refcount_t

[Peter Zijlstra]

On Tue, Feb 21, 2017 at 05:49:03PM +0200, Elena Reshetova wrote:
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.

Changelog forgets to mention if this was runtime tested..


> @@ -371,7 +371,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
>  	ASSERT(bip->bli_item.li_type == XFS_LI_BUF);
>  	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
>  	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
> -	ASSERT(atomic_read(&bip->bli_refcount) > 0);
> +	ASSERT(refcount_read(&bip->bli_refcount) > 0);
>  
>  	trace_xfs_trans_brelse(bip);
>  
> @@ -419,7 +419,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
>  	/*
>  	 * Drop our reference to the buf log item.
>  	 */
> -	atomic_dec(&bip->bli_refcount);
> +	refcount_dec(&bip->bli_refcount);
>  
>  	/*
>  	 * If the buf item is not tracking data in the log, then
> @@ -432,7 +432,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
>  /***
>  		ASSERT(bp->b_pincount == 0);
>  ***/
> -		ASSERT(atomic_read(&bip->bli_refcount) == 0);
> +		ASSERT(refcount_read(&bip->bli_refcount) == 0);
>  		ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
>  		ASSERT(!(bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF));
>  		xfs_buf_item_relse(bp);


This for example looks dodgy.

That seems to suggest the atomic_dec() there can actually hit 0, which
_will_ generate a WARN.

Re: [PATCH 2/7] fs, xfs: convert xfs_buf.b_hold and xfs_buf.b_lru_ref from atomic_t to refcount_t

[Peter Zijlstra]

On Tue, Feb 21, 2017 at 05:49:02PM +0200, Elena Reshetova wrote:
> @@ -1684,10 +1684,11 @@ xfs_buftarg_isolate(
>  	 * zero. If the value is already zero, we need to reclaim the
>  	 * buffer, otherwise it gets another trip through the LRU.
>  	 */
> -	if (!atomic_add_unless(&bp->b_lru_ref, -1, 0)) {
> +	if (!refcount_read(&bp->b_lru_ref)) {
>  		spin_unlock(&bp->b_lock);
>  		return LRU_ROTATE;
>  	}
> +	refcount_dec_and_test(&bp->b_lru_ref);
>  
>  	bp->b_state |= XFS_BSTATE_DISPOSE;
>  	list_lru_isolate_move(lru, item, dispose);

This should never have passed testing.. refcount_dec_and_test() has a
__must_check.

Furthermore the above seems to suggest thingies can live with a 0
refcount, so a straight conversion cannot work.

RE: [PATCH 3/7] fs, xfs: convert xfs_buf_log_item.bli_refcount from atomic_t to refcount_t

[Reshetova, Elena]

> On Tue, Feb 21, 2017 at 05:49:03PM +0200, Elena Reshetova wrote:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> 
> Changelog forgets to mention if this was runtime tested..

It was boot-tested in the whole refcount_t changes pile, which is not very useful for fs anyway. 
What's why we are sending this through maintainers to get through their tests. 
I am sure that testing would be better than what we can do. 

> 
> 
> > @@ -371,7 +371,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> >  	ASSERT(bip->bli_item.li_type == XFS_LI_BUF);
> >  	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
> >  	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
> > -	ASSERT(atomic_read(&bip->bli_refcount) > 0);
> > +	ASSERT(refcount_read(&bip->bli_refcount) > 0);
> >
> >  	trace_xfs_trans_brelse(bip);
> >
> > @@ -419,7 +419,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> >  	/*
> >  	 * Drop our reference to the buf log item.
> >  	 */
> > -	atomic_dec(&bip->bli_refcount);
> > +	refcount_dec(&bip->bli_refcount);
> >
> >  	/*
> >  	 * If the buf item is not tracking data in the log, then
> > @@ -432,7 +432,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> >  /***
> >  		ASSERT(bp->b_pincount == 0);
> >  ***/
> > -		ASSERT(atomic_read(&bip->bli_refcount) == 0);
> > +		ASSERT(refcount_read(&bip->bli_refcount) == 0);
> >  		ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
> >  		ASSERT(!(bip->bli_flags &
> XFS_BLI_INODE_ALLOC_BUF));
> >  		xfs_buf_item_relse(bp);
> 
> 
> This for example looks dodgy.
> 
> That seems to suggest the atomic_dec() there can actually hit 0, which
> _will_ generate a WARN.

True, but in some of this cases WARN might be ok, I think? As soon as functionality is not changed and object is not reused (by doing refcount_inc on it) anywhere later on. 

Re: [PATCH 3/7] fs, xfs: convert xfs_buf_log_item.bli_refcount from atomic_t to refcount_t

[Peter Zijlstra]

On Tue, Feb 21, 2017 at 04:06:30PM +0000, Reshetova, Elena wrote:
> > On Tue, Feb 21, 2017 at 05:49:03PM +0200, Elena Reshetova wrote:
> > > refcount_t type and corresponding API should be
> > > used instead of atomic_t when the variable is used as
> > > a reference counter. This allows to avoid accidental
> > > refcounter overflows that might lead to use-after-free
> > > situations.
> > 
> > Changelog forgets to mention if this was runtime tested..
> 
> It was boot-tested in the whole refcount_t changes pile, which is not very useful for fs anyway. 
> What's why we are sending this through maintainers to get through their tests. 
> I am sure that testing would be better than what we can do. 

For FS changes, I would recommend at the very least running xfstests on
an instance (which unlike the name suggests is good for most block
filesystems Linux has).

> > > @@ -432,7 +432,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > >  /***
> > >  		ASSERT(bp->b_pincount == 0);
> > >  ***/
> > > -		ASSERT(atomic_read(&bip->bli_refcount) == 0);
> > > +		ASSERT(refcount_read(&bip->bli_refcount) == 0);
> > >  		ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
> > >  		ASSERT(!(bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF));
> > >  		xfs_buf_item_relse(bp);
> > 
> > 
> > This for example looks dodgy.
> > 
> > That seems to suggest the atomic_dec() there can actually hit 0, which
> > _will_ generate a WARN.
> 
> True, but in some of this cases WARN might be ok, I think? As soon as
> functionality is not changed and object is not reused (by doing
> refcount_inc on it) anywhere later on. 

No, your conversion should not generate spurious WARN()s.

And also no, atomic_dec() must not hit 0. If you've been _that_ careful
with your reference counting and you absolutely _know_ this is the very
last one, write something like:

  WARN_ON(!refcount_dec_if_one());


Please, stop sending out conversions that haven't been tested. And take
the time to actually look at your own patches. If I can spot fail just
looking through them, so can you (or any of the other many people in
your SoB chain).

Take a little more time and a little extra care.

Re: [PATCH 3/7] fs, xfs: convert xfs_buf_log_item.bli_refcount from atomic_t to refcount_t

[Peter Zijlstra]

On Tue, Feb 21, 2017 at 05:27:58PM +0100, Peter Zijlstra wrote:
> > True, but in some of this cases WARN might be ok, I think? As soon as
> > functionality is not changed and object is not reused (by doing
> > refcount_inc on it) anywhere later on. 
> 
> No, your conversion should not generate spurious WARN()s.
> 
> And also no, atomic_dec() must not hit 0. If you've been _that_ careful

refcount_dec() obviously, atomic_dec() doesn't care one way or the
other.

> with your reference counting and you absolutely _know_ this is the very
> last one, write something like:
> 
>   WARN_ON(!refcount_dec_if_one());
> 
> 
> Please, stop sending out conversions that haven't been tested. And take
> the time to actually look at your own patches. If I can spot fail just
> looking through them, so can you (or any of the other many people in
> your SoB chain).
> 
> Take a little more time and a little extra care.

Re: [PATCH 1/7] fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to refcount_t

[Darrick J. Wong]

On Tue, Feb 21, 2017 at 05:49:01PM +0200, Elena Reshetova wrote:
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.
> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: David Windsor <dwindsor@gmail.com>
> ---
>  fs/xfs/xfs_bmap_item.c | 4 ++--
>  fs/xfs/xfs_bmap_item.h | 4 +++-
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index 9bf57c7..33104ad 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -199,7 +199,7 @@ xfs_bui_init(
>  	buip->bui_format.bui_nextents = XFS_BUI_MAX_FAST_EXTENTS;
>  	buip->bui_format.bui_id = (uintptr_t)(void *)buip;
>  	atomic_set(&buip->bui_next_extent, 0);
> -	atomic_set(&buip->bui_refcount, 2);
> +	refcount_set(&buip->bui_refcount, 2);
>  
>  	return buip;
>  }
> @@ -215,7 +215,7 @@ void
>  xfs_bui_release(
>  	struct xfs_bui_log_item	*buip)
>  {
> -	if (atomic_dec_and_test(&buip->bui_refcount)) {
> +	if (refcount_dec_and_test(&buip->bui_refcount)) {
>  		xfs_trans_ail_remove(&buip->bui_item, SHUTDOWN_LOG_IO_ERROR);
>  		xfs_bui_item_free(buip);
>  	}
> diff --git a/fs/xfs/xfs_bmap_item.h b/fs/xfs/xfs_bmap_item.h
> index c867daa..988a6ae 100644
> --- a/fs/xfs/xfs_bmap_item.h
> +++ b/fs/xfs/xfs_bmap_item.h
> @@ -20,6 +20,8 @@
>  #ifndef	__XFS_BMAP_ITEM_H__
>  #define	__XFS_BMAP_ITEM_H__
>  
> +#include <linux/refcount.h>

Seeing as you're sprinkling refcount_t all over XFS, this ought to go in
xfs_linux.h with all the other Linux-specific #includes.

--D

> +
>  /*
>   * There are (currently) two pairs of bmap btree redo item types: map & unmap.
>   * The common abbreviations for these are BUI (bmap update intent) and BUD
> @@ -61,7 +63,7 @@ struct kmem_zone;
>   */
>  struct xfs_bui_log_item {
>  	struct xfs_log_item		bui_item;
> -	atomic_t			bui_refcount;
> +	refcount_t			bui_refcount;
>  	atomic_t			bui_next_extent;
>  	unsigned long			bui_flags;	/* misc flags */
>  	struct xfs_bui_log_format	bui_format;
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/7] fs, xfs: convert xfs_buf_log_item.bli_refcount from atomic_t to refcount_t

[Darrick J. Wong]

On Tue, Feb 21, 2017 at 04:06:30PM +0000, Reshetova, Elena wrote:
> > On Tue, Feb 21, 2017 at 05:49:03PM +0200, Elena Reshetova wrote:
> > > refcount_t type and corresponding API should be
> > > used instead of atomic_t when the variable is used as
> > > a reference counter. This allows to avoid accidental
> > > refcounter overflows that might lead to use-after-free
> > > situations.
> > 
> > Changelog forgets to mention if this was runtime tested..
> 
> It was boot-tested in the whole refcount_t changes pile, which is not very useful for fs anyway. 
> What's why we are sending this through maintainers to get through their tests. 
> I am sure that testing would be better than what we can do. 

If you're going to go around making this many changes to XFS (or any
other filesystem), please run the changes through xfstests first.
Many fs projects (not just XFS) record their test cases there.

I think the kernel 0day build service is supposed to do that
automatically...

--D

> 
> > 
> > 
> > > @@ -371,7 +371,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > >  	ASSERT(bip->bli_item.li_type == XFS_LI_BUF);
> > >  	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
> > >  	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
> > > -	ASSERT(atomic_read(&bip->bli_refcount) > 0);
> > > +	ASSERT(refcount_read(&bip->bli_refcount) > 0);
> > >
> > >  	trace_xfs_trans_brelse(bip);
> > >
> > > @@ -419,7 +419,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > >  	/*
> > >  	 * Drop our reference to the buf log item.
> > >  	 */
> > > -	atomic_dec(&bip->bli_refcount);
> > > +	refcount_dec(&bip->bli_refcount);
> > >
> > >  	/*
> > >  	 * If the buf item is not tracking data in the log, then
> > > @@ -432,7 +432,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > >  /***
> > >  		ASSERT(bp->b_pincount == 0);
> > >  ***/
> > > -		ASSERT(atomic_read(&bip->bli_refcount) == 0);
> > > +		ASSERT(refcount_read(&bip->bli_refcount) == 0);
> > >  		ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
> > >  		ASSERT(!(bip->bli_flags &
> > XFS_BLI_INODE_ALLOC_BUF));
> > >  		xfs_buf_item_relse(bp);
> > 
> > 
> > This for example looks dodgy.
> > 
> > That seems to suggest the atomic_dec() there can actually hit 0, which
> > _will_ generate a WARN.
> 
> True, but in some of this cases WARN might be ok, I think? As soon as functionality is not changed and object is not reused (by doing refcount_inc on it) anywhere later on. 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/7] fs, xfs: convert xfs_buf_log_item.bli_refcount from atomic_t to refcount_t

[Brian Foster]

On Tue, Feb 21, 2017 at 09:06:20AM -0800, Darrick J. Wong wrote:
> On Tue, Feb 21, 2017 at 04:06:30PM +0000, Reshetova, Elena wrote:
> > > On Tue, Feb 21, 2017 at 05:49:03PM +0200, Elena Reshetova wrote:
> > > > refcount_t type and corresponding API should be
> > > > used instead of atomic_t when the variable is used as
> > > > a reference counter. This allows to avoid accidental
> > > > refcounter overflows that might lead to use-after-free
> > > > situations.
> > > 
> > > Changelog forgets to mention if this was runtime tested..
> > 
> > It was boot-tested in the whole refcount_t changes pile, which is not very useful for fs anyway. 
> > What's why we are sending this through maintainers to get through their tests. 
> > I am sure that testing would be better than what we can do. 
> 
> If you're going to go around making this many changes to XFS (or any
> other filesystem), please run the changes through xfstests first.
> Many fs projects (not just XFS) record their test cases there.
> 
> I think the kernel 0day build service is supposed to do that
> automatically...
> 

Be sure to use CONFIG_XFS_DEBUG and/or CONFIG_XFS_WARN to capture any
potential assert failures as well.

Brian

> --D
> 
> > 
> > > 
> > > 
> > > > @@ -371,7 +371,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > > >  	ASSERT(bip->bli_item.li_type == XFS_LI_BUF);
> > > >  	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
> > > >  	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
> > > > -	ASSERT(atomic_read(&bip->bli_refcount) > 0);
> > > > +	ASSERT(refcount_read(&bip->bli_refcount) > 0);
> > > >
> > > >  	trace_xfs_trans_brelse(bip);
> > > >
> > > > @@ -419,7 +419,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > > >  	/*
> > > >  	 * Drop our reference to the buf log item.
> > > >  	 */
> > > > -	atomic_dec(&bip->bli_refcount);
> > > > +	refcount_dec(&bip->bli_refcount);
> > > >
> > > >  	/*
> > > >  	 * If the buf item is not tracking data in the log, then
> > > > @@ -432,7 +432,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > > >  /***
> > > >  		ASSERT(bp->b_pincount == 0);
> > > >  ***/
> > > > -		ASSERT(atomic_read(&bip->bli_refcount) == 0);
> > > > +		ASSERT(refcount_read(&bip->bli_refcount) == 0);
> > > >  		ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
> > > >  		ASSERT(!(bip->bli_flags &
> > > XFS_BLI_INODE_ALLOC_BUF));
> > > >  		xfs_buf_item_relse(bp);
> > > 
> > > 
> > > This for example looks dodgy.
> > > 
> > > That seems to suggest the atomic_dec() there can actually hit 0, which
> > > _will_ generate a WARN.
> > 
> > True, but in some of this cases WARN might be ok, I think? As soon as functionality is not changed and object is not reused (by doing refcount_inc on it) anywhere later on. 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/7] fs, xfs: convert xfs_buf.b_hold and xfs_buf.b_lru_ref from atomic_t to refcount_t

[Dave Chinner]

On Tue, Feb 21, 2017 at 05:04:08PM +0100, Peter Zijlstra wrote:
> On Tue, Feb 21, 2017 at 05:49:02PM +0200, Elena Reshetova wrote:
> > @@ -1684,10 +1684,11 @@ xfs_buftarg_isolate(
> >  	 * zero. If the value is already zero, we need to reclaim the
> >  	 * buffer, otherwise it gets another trip through the LRU.
> >  	 */
> > -	if (!atomic_add_unless(&bp->b_lru_ref, -1, 0)) {
> > +	if (!refcount_read(&bp->b_lru_ref)) {
> >  		spin_unlock(&bp->b_lock);
> >  		return LRU_ROTATE;
> >  	}
> > +	refcount_dec_and_test(&bp->b_lru_ref);
> >  
> >  	bp->b_state |= XFS_BSTATE_DISPOSE;
> >  	list_lru_isolate_move(lru, item, dispose);
> 
> This should never have passed testing.. refcount_dec_and_test() has a
> __must_check.
> 
> Furthermore the above seems to suggest thingies can live with a 0
> refcount, so a straight conversion cannot work.

Yes, 0 is a valid value - the buffer lru reference is *not an object
lifecycle reference count*. A value of zero means reclaim needs to
take action if it sees that value - it does not mean that the object
is not referenced by anyone (that's b_hold). i.e.  b_lru_ref is an
"active reference weighting" used to provide a heirarchical reclaim
bias toward less important metadata objects, and has no bearing on
the actual active users of the object.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/7] fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to refcount_t

[Dave Chinner]

On Tue, Feb 21, 2017 at 05:49:01PM +0200, Elena Reshetova wrote:
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.

I'm missing something: how do you overflow a log item object
reference count?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

RE: [PATCH 2/7] fs, xfs: convert xfs_buf.b_hold and xfs_buf.b_lru_ref from atomic_t to refcount_t

[Reshetova, Elena]

> On Tue, Feb 21, 2017 at 05:04:08PM +0100, Peter Zijlstra wrote:
> > On Tue, Feb 21, 2017 at 05:49:02PM +0200, Elena Reshetova wrote:
> > > @@ -1684,10 +1684,11 @@ xfs_buftarg_isolate(
> > >  	 * zero. If the value is already zero, we need to reclaim the
> > >  	 * buffer, otherwise it gets another trip through the LRU.
> > >  	 */
> > > -	if (!atomic_add_unless(&bp->b_lru_ref, -1, 0)) {
> > > +	if (!refcount_read(&bp->b_lru_ref)) {
> > >  		spin_unlock(&bp->b_lock);
> > >  		return LRU_ROTATE;
> > >  	}
> > > +	refcount_dec_and_test(&bp->b_lru_ref);
> > >
> > >  	bp->b_state |= XFS_BSTATE_DISPOSE;
> > >  	list_lru_isolate_move(lru, item, dispose);
> >
> > This should never have passed testing.. refcount_dec_and_test() has a
> > __must_check.
> >
> > Furthermore the above seems to suggest thingies can live with a 0
> > refcount, so a straight conversion cannot work.
> 
> Yes, 0 is a valid value - the buffer lru reference is *not an object
> lifecycle reference count*. A value of zero means reclaim needs to
> take action if it sees that value - it does not mean that the object
> is not referenced by anyone (that's b_hold). i.e.  b_lru_ref is an
> "active reference weighting" used to provide a heirarchical reclaim
> bias toward less important metadata objects, and has no bearing on
> the actual active users of the object.


OK, so all of this suggests that we should not conver b_lru_ref to the refcount_t then. 
I will remove this conversion from this commit and only leave b_hold.

Thank you!

Best Regards,
Elena.

RE: [PATCH 1/7] fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to refcount_t

[Reshetova, Elena]

> On Tue, Feb 21, 2017 at 05:49:01PM +0200, Elena Reshetova wrote:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> >
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: David Windsor <dwindsor@gmail.com>
> > ---
> >  fs/xfs/xfs_bmap_item.c | 4 ++--
> >  fs/xfs/xfs_bmap_item.h | 4 +++-
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> > index 9bf57c7..33104ad 100644
> > --- a/fs/xfs/xfs_bmap_item.c
> > +++ b/fs/xfs/xfs_bmap_item.c
> > @@ -199,7 +199,7 @@ xfs_bui_init(
> >  	buip->bui_format.bui_nextents =
> XFS_BUI_MAX_FAST_EXTENTS;
> >  	buip->bui_format.bui_id = (uintptr_t)(void *)buip;
> >  	atomic_set(&buip->bui_next_extent, 0);
> > -	atomic_set(&buip->bui_refcount, 2);
> > +	refcount_set(&buip->bui_refcount, 2);
> >
> >  	return buip;
> >  }
> > @@ -215,7 +215,7 @@ void
> >  xfs_bui_release(
> >  	struct xfs_bui_log_item	*buip)
> >  {
> > -	if (atomic_dec_and_test(&buip->bui_refcount)) {
> > +	if (refcount_dec_and_test(&buip->bui_refcount)) {
> >  		xfs_trans_ail_remove(&buip->bui_item,
> SHUTDOWN_LOG_IO_ERROR);
> >  		xfs_bui_item_free(buip);
> >  	}
> > diff --git a/fs/xfs/xfs_bmap_item.h b/fs/xfs/xfs_bmap_item.h
> > index c867daa..988a6ae 100644
> > --- a/fs/xfs/xfs_bmap_item.h
> > +++ b/fs/xfs/xfs_bmap_item.h
> > @@ -20,6 +20,8 @@
> >  #ifndef	__XFS_BMAP_ITEM_H__
> >  #define	__XFS_BMAP_ITEM_H__
> >
> > +#include <linux/refcount.h>
> 
> Seeing as you're sprinkling refcount_t all over XFS, this ought to go in
> xfs_linux.h with all the other Linux-specific #includes.
> 
> --D

Sure, I will move the include there and leave only one. 
Will send a next version also after fixing the other non-reference counters case we discussed. 

Best Regards,
Elena.
> 
> > +
> >  /*
> >   * There are (currently) two pairs of bmap btree redo item types: map &
> unmap.
> >   * The common abbreviations for these are BUI (bmap update intent) and
> BUD
> > @@ -61,7 +63,7 @@ struct kmem_zone;
> >   */
> >  struct xfs_bui_log_item {
> >  	struct xfs_log_item		bui_item;
> > -	atomic_t			bui_refcount;
> > +	refcount_t			bui_refcount;
> >  	atomic_t			bui_next_extent;
> >  	unsigned long			bui_flags;	/* misc
> flags */
> >  	struct xfs_bui_log_format	bui_format;
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/7] fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to refcount_t

[Reshetova, Elena]

> On Tue, Feb 21, 2017 at 05:49:01PM +0200, Elena Reshetova wrote:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> 
> I'm missing something: how do you overflow a log item object
> reference count?

We are currently converting all reference counters present in kernel to a safer refcount_t type. 
Agreed, in some cases it might be easier or harder to actually create/trigger an overflow, but since it can be caused even by a bug in the legitimate code (current version or its future iterative), it is good idea to do "safe defaults" and stop worrying about the problem. 

Do you have any reasons why it should not be converted? 

Best Regards,
Elena.
> 
> Cheers,
> 
> Dave.
> --
> Dave Chinner
> david@fromorbit.com

RE: [PATCH 3/7] fs, xfs: convert xfs_buf_log_item.bli_refcount from atomic_t to refcount_t

[Reshetova, Elena]

> On Tue, Feb 21, 2017 at 09:06:20AM -0800, Darrick J. Wong wrote:
> > On Tue, Feb 21, 2017 at 04:06:30PM +0000, Reshetova, Elena wrote:
> > > > On Tue, Feb 21, 2017 at 05:49:03PM +0200, Elena Reshetova wrote:
> > > > > refcount_t type and corresponding API should be
> > > > > used instead of atomic_t when the variable is used as
> > > > > a reference counter. This allows to avoid accidental
> > > > > refcounter overflows that might lead to use-after-free
> > > > > situations.
> > > >
> > > > Changelog forgets to mention if this was runtime tested..
> > >
> > > It was boot-tested in the whole refcount_t changes pile, which is not very
> useful for fs anyway.
> > > What's why we are sending this through maintainers to get through their
> tests.
> > > I am sure that testing would be better than what we can do.
> >
> > If you're going to go around making this many changes to XFS (or any
> > other filesystem), please run the changes through xfstests first.
> > Many fs projects (not just XFS) record their test cases there.
> >
> > I think the kernel 0day build service is supposed to do that
> > automatically...
> >
> 
> Be sure to use CONFIG_XFS_DEBUG and/or CONFIG_XFS_WARN to capture
> any
> potential assert failures as well.

Thanks for pointing to this! I have been actually asking before on how to runtime test things more with all our patches before submission, but got reply around: "submit to maintainers, they know how to do it". 
I think we need to look into 0day automated testing, otherwise since we make changes to many FSes, testing this manually would be little fun...

Best Regards,
Elena
> 
> Brian
> 
> > --D
> >
> > >
> > > >
> > > >
> > > > > @@ -371,7 +371,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > > > >  	ASSERT(bip->bli_item.li_type == XFS_LI_BUF);
> > > > >  	ASSERT(!(bip->bli_flags & XFS_BLI_STALE));
> > > > >  	ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
> > > > > -	ASSERT(atomic_read(&bip->bli_refcount) > 0);
> > > > > +	ASSERT(refcount_read(&bip->bli_refcount) > 0);
> > > > >
> > > > >  	trace_xfs_trans_brelse(bip);
> > > > >
> > > > > @@ -419,7 +419,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > > > >  	/*
> > > > >  	 * Drop our reference to the buf log item.
> > > > >  	 */
> > > > > -	atomic_dec(&bip->bli_refcount);
> > > > > +	refcount_dec(&bip->bli_refcount);
> > > > >
> > > > >  	/*
> > > > >  	 * If the buf item is not tracking data in the log, then
> > > > > @@ -432,7 +432,7 @@ xfs_trans_brelse(xfs_trans_t	*tp,
> > > > >  /***
> > > > >  		ASSERT(bp->b_pincount == 0);
> > > > >  ***/
> > > > > -		ASSERT(atomic_read(&bip->bli_refcount) == 0);
> > > > > +		ASSERT(refcount_read(&bip->bli_refcount) == 0);
> > > > >  		ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
> > > > >  		ASSERT(!(bip->bli_flags &
> > > > XFS_BLI_INODE_ALLOC_BUF));
> > > > >  		xfs_buf_item_relse(bp);
> > > >
> > > >
> > > > This for example looks dodgy.
> > > >
> > > > That seems to suggest the atomic_dec() there can actually hit 0, which
> > > > _will_ generate a WARN.
> > >
> > > True, but in some of this cases WARN might be ok, I think? As soon as
> functionality is not changed and object is not reused (by doing refcount_inc on
> it) anywhere later on.
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to refcount_t

[Dave Chinner]

On Wed, Feb 22, 2017 at 11:20:31AM +0000, Reshetova, Elena wrote:
> > On Tue, Feb 21, 2017 at 05:49:01PM +0200, Elena Reshetova wrote:
> > > refcount_t type and corresponding API should be
> > > used instead of atomic_t when the variable is used as
> > > a reference counter. This allows to avoid accidental
> > > refcounter overflows that might lead to use-after-free
> > > situations.
> > 
> > I'm missing something: how do you overflow a log item object
> > reference count?
> 
> We are currently converting all reference counters present in kernel to a safer refcount_t type. 

Yes, I see that you are taking anything that you *think* is an
object lifetime reference counter and changing it.

> Agreed, in some cases it might be easier or harder to actually create/trigger an overflow, but since it can be caused even by a bug in the legitimate code (current version or its future iterative), it is good idea to do "safe defaults" and stop worrying about the problem. 
> 
> Do you have any reasons why it should not be converted? 

It's core dirty metadata object code.  Any change to code in this
area needs to be gone over with a fine tooth comb, because bugs can
result in filesystem and/or journal corruption issues that may not
be noticed until a system crashes and log recovery fails and the
user loses their entire filesystem....

Hence the repeated comments about needing to actually test the code
you are changing.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

RE: [PATCH 1/7] fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to refcount_t

[Reshetova, Elena]

> On Wed, Feb 22, 2017 at 11:20:31AM +0000, Reshetova, Elena wrote:
> > > On Tue, Feb 21, 2017 at 05:49:01PM +0200, Elena Reshetova wrote:
> > > > refcount_t type and corresponding API should be
> > > > used instead of atomic_t when the variable is used as
> > > > a reference counter. This allows to avoid accidental
> > > > refcounter overflows that might lead to use-after-free
> > > > situations.
> > >
> > > I'm missing something: how do you overflow a log item object
> > > reference count?
> >
> > We are currently converting all reference counters present in kernel to a
> safer refcount_t type.
> 
> Yes, I see that you are taking anything that you *think* is an
> object lifetime reference counter and changing it.
> 
> > Agreed, in some cases it might be easier or harder to actually create/trigger
> an overflow, but since it can be caused even by a bug in the legitimate code
> (current version or its future iterative), it is good idea to do "safe defaults" and
> stop worrying about the problem.
> >
> > Do you have any reasons why it should not be converted?
> 
> It's core dirty metadata object code.  Any change to code in this
> area needs to be gone over with a fine tooth comb, because bugs can
> result in filesystem and/or journal corruption issues that may not
> be noticed until a system crashes and log recovery fails and the
> user loses their entire filesystem....
> 
> Hence the repeated comments about needing to actually test the code
> you are changing.

Sure, we are now in the process of testing this run-time as was suggested using xfstests. 
I will only repost this series after we done with testing and fix issues.
 
Best Regards,
Elena.


> 
> Cheers,
> 
> Dave.
> --
> Dave Chinner
> david@fromorbit.com