[RFC PATCH 0/2] xfs: change xfs log item refcount design

[RFC PATCH 0/2] xfs: change xfs log item refcount design

[Jeffin Philip]

This patch series is the first of the series where we attempt to convert
the XFS Log item reference counting design to be more generic
compared to the previous type-specific state to fix the "zero
refcount but in AIL means alive" state. The patch series follows
instructions outlined here[1].

Obviously, the wrappers will be integrated in type-specific functions
later on.

No behavioral change as of yet.

[1]: https://lore.kernel.org/all/aqI6v-_dc2DDSbgB@dread/

Jeffin Philip (2):
  xfs: add lockref and generic helpers for refcounting
  xfs: change xfs_trans_ail_delete return type to bool

 fs/xfs/xfs_log.c        | 38 ++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_trans.h      |  7 +++++++
 fs/xfs/xfs_trans_ail.c  |  5 +++--
 fs/xfs/xfs_trans_priv.h |  2 +-
 4 files changed, 49 insertions(+), 3 deletions(-)

-- 
2.55.0

[RFC PATCH 1/2] xfs: add lockref and generic helpers for refcounting

[Jeffin Philip]

As part of fixing the UAF in xlog_cil_ail_insert() reported by
syzbot, add a generic lockref to xfs_log_item struct and
initialize it in xfs_log_item_init(). In addition, add generic
helpers(get()/put()/get_safe()) as part of the generic refcounting
infrastructure for xfs.

Signed-off-by: Jeffin Philip <jeffinphilip14@gmail.com>
---
 fs/xfs/xfs_log.c   | 38 ++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_trans.h |  7 +++++++
 2 files changed, 45 insertions(+)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index f807f8f4f705..1489f8f20b3e 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1033,12 +1033,50 @@ xfs_log_item_init(
 	item->li_ops = ops;
 	item->li_lv = NULL;
 
+	/*
+	 * Refrain from using lockref_init as BLI refcount should be
+	 * initialized to 0 and lockref_init initializes refcount to 1
+	 */
+	spin_lock_init(&item->li_ref.lock);
+	item->li_ref.count = 0;
 	INIT_LIST_HEAD(&item->li_ail);
 	INIT_LIST_HEAD(&item->li_cil);
 	INIT_LIST_HEAD(&item->li_bio_list);
 	INIT_LIST_HEAD(&item->li_trans);
 }
 
+/*
+ * Only called when the caller knows the object is alive
+ */
+void
+xfs_log_item_get(
+	struct xfs_log_item	*lip)
+{
+	lockref_get(&lip->li_ref);
+}
+
+/*
+ * Drop a log item reference when called. Returns true if last
+ * ref with lock held. Otherwise false.
+ */
+bool
+xfs_log_item_put(
+	struct xfs_log_item	*lip)
+{
+	return lockref_put_or_lock(&lip->li_ref);
+}
+
+/*
+ * Used to lookup if item may be dying. Returns true is the object
+ * is not dead, false otherwise.
+ */
+bool
+xfs_log_item_get_safe(
+	struct xfs_log_item	*lip)
+{
+	return lockref_get_not_dead(&lip->li_ref);
+}
+
 /*
  * Wake up processes waiting for log space after we have moved the log tail.
  */
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index eb83c5dac032..cd469e2e4e4d 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -6,6 +6,8 @@
 #ifndef	__XFS_TRANS_H__
 #define	__XFS_TRANS_H__
 
+#include <linux/lockref.h>
+
 /* kernel only transaction subsystem defines */
 
 struct xlog;
@@ -46,6 +48,8 @@ struct xfs_log_item {
 	struct xfs_log_vec		*li_lv_shadow;	/* standby vector */
 	xfs_csn_t			li_seq;		/* CIL commit seq */
 	uint32_t			li_order_id;	/* CIL commit order */
+
+	struct lockref			li_ref;		/* log item reference */
 };
 
 /*
@@ -110,6 +114,9 @@ xlog_item_is_intent_done(struct xfs_log_item *lip)
 
 void	xfs_log_item_init(struct xfs_mount *mp, struct xfs_log_item *item,
 			  int type, const struct xfs_item_ops *ops);
+void	xfs_log_item_get(struct xfs_log_item *lip);
+bool	xfs_log_item_put(struct xfs_log_item *lip);
+bool	xfs_log_item_get_safe(struct xfs_log_item *lip);
 
 /*
  * Return values for the iop_push() routines.
-- 
2.55.0

[RFC PATCH 2/2] xfs: change xfs_trans_ail_delete return type to bool

[Jeffin Philip]

Change xfs_trans_ail_delete's return type from void to
bool which returns true if the item was removed from
AIL and false in case of shutdown(item not in AIL).
Reflect the change in header file too.

Signed-off-by: Jeffin Philip <jeffinphilip14@gmail.com>
---
 fs/xfs/xfs_trans_ail.c  | 5 +++--
 fs/xfs/xfs_trans_priv.h | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 99a9bf3762b7..b17b677586a0 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -913,7 +913,7 @@ xfs_ail_delete_one(
 	return 0;
 }
 
-void
+bool
 xfs_trans_ail_delete(
 	struct xfs_log_item	*lip,
 	int			shutdown_type)
@@ -931,12 +931,13 @@ xfs_trans_ail_delete(
 					__func__);
 			xlog_force_shutdown(log, shutdown_type);
 		}
-		return;
+		return false;
 	}
 
 	clear_bit(XFS_LI_FAILED, &lip->li_flags);
 	tail_lsn = xfs_ail_delete_one(ailp, lip);
 	xfs_ail_update_finish(ailp, tail_lsn);	/* drops the AIL lock */
+	return true;
 }
 
 int
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index f945f0450b16..b00c803941c7 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -100,7 +100,7 @@ void xfs_trans_ail_insert(struct xfs_ail *ailp, struct xfs_log_item *lip,
 xfs_lsn_t xfs_ail_delete_one(struct xfs_ail *ailp, struct xfs_log_item *lip);
 void xfs_ail_update_finish(struct xfs_ail *ailp, xfs_lsn_t old_lsn)
 			__releases(ailp->ail_lock);
-void xfs_trans_ail_delete(struct xfs_log_item *lip, int shutdown_type);
+bool xfs_trans_ail_delete(struct xfs_log_item *lip, int shutdown_type);
 
 static inline void xfs_ail_push(struct xfs_ail *ailp)
 {
-- 
2.55.0