From 94014a23e91a3944947048169ccf38b4561cfd0c Mon Sep 17 00:00:00 2001 From: "Darrick J. Wong" Date: Wed, 18 Feb 2026 15:25:37 -0800 Subject: [PATCH] xfs: fix potential pointer access race in xfs_healthmon_get Pankaj Raghav asks about this code in xfs_healthmon_get: hm = mp->m_healthmon; if (hm && !refcount_inc_not_zero(&hm->ref)) hm = NULL; rcu_read_unlock(); return hm; (slightly edited to compress a mailing list thread) "Nit: Should we do a READ_ONCE(mp->m_healthmon) here to avoid any compiler tricks that can result in an undefined behaviour? I am not sure if I am being paranoid here. "So this is my understanding: RCU guarantees that we get a valid object (actual data of m_healthmon) but does not guarantee the compiler will not reread the pointer between checking if hm is !NULL and accessing the pointer as we are doing it lockless. "So just a barrier() call in rcu_read_lock is enough to make sure this doesn't happen and probably adding a READ_ONCE() is not needed?" After some initial confusion I concluded that he's correct. The compiler could very well eliminate the hm variable in favor of walking the pointers twice, turning the code into: if (mp->m_healthmon && !refcount_inc_not_zero(&mp->m_healthmon->ref)) If this happens, then xfs_healthmon_detach can sneak in between the two sides of the && expression and set mp->m_healthmon to NULL, and thereby cause a null pointer dereference crash. Fix this by using the rcu pointer assignment and dereference functions, which ensure that the proper reordering barriers are in place. Practically speaking, gcc seems to allocate an actual variable for hm and only reads mp->m_healthmon once (as intended), but we ought to be more explicit about requiring this. Reported-by: Pankaj Raghav Fixes: a48373e7d35a89f6f ("xfs: start creating infrastructure for health monitoring") Signed-off-by: "Darrick J. Wong" Reviewed-by: Christoph Hellwig Reviewed-by: Carlos Maiolino Reviewed-by: Pankaj Raghav Signed-off-by: Carlos Maiolino --- fs/xfs/xfs_healthmon.c | 11 +++++++---- fs/xfs/xfs_mount.h | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/fs/xfs/xfs_healthmon.c b/fs/xfs/xfs_healthmon.c index e37c18cec372..4a06d6632f65 100644 --- a/fs/xfs/xfs_healthmon.c +++ b/fs/xfs/xfs_healthmon.c @@ -69,7 +69,7 @@ xfs_healthmon_get( struct xfs_healthmon *hm; rcu_read_lock(); - hm = mp->m_healthmon; + hm = rcu_dereference(mp->m_healthmon); if (hm && !refcount_inc_not_zero(&hm->ref)) hm = NULL; rcu_read_unlock(); @@ -110,13 +110,13 @@ xfs_healthmon_attach( struct xfs_healthmon *hm) { spin_lock(&xfs_healthmon_lock); - if (mp->m_healthmon != NULL) { + if (rcu_access_pointer(mp->m_healthmon) != NULL) { spin_unlock(&xfs_healthmon_lock); return -EEXIST; } refcount_inc(&hm->ref); - mp->m_healthmon = hm; + rcu_assign_pointer(mp->m_healthmon, hm); hm->mount_cookie = (uintptr_t)mp->m_super; spin_unlock(&xfs_healthmon_lock); @@ -128,13 +128,16 @@ STATIC void xfs_healthmon_detach( struct xfs_healthmon *hm) { + struct xfs_mount *mp; + spin_lock(&xfs_healthmon_lock); if (hm->mount_cookie == DETACHED_MOUNT_COOKIE) { spin_unlock(&xfs_healthmon_lock); return; } - XFS_M((struct super_block *)hm->mount_cookie)->m_healthmon = NULL; + mp = XFS_M((struct super_block *)hm->mount_cookie); + rcu_assign_pointer(mp->m_healthmon, NULL); hm->mount_cookie = DETACHED_MOUNT_COOKIE; spin_unlock(&xfs_healthmon_lock); diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h index 61c71128d171..ddd4028be8d6 100644 --- a/fs/xfs/xfs_mount.h +++ b/fs/xfs/xfs_mount.h @@ -345,7 +345,7 @@ typedef struct xfs_mount { struct xfs_hooks m_dir_update_hooks; /* Private data referring to a health monitor object. */ - struct xfs_healthmon *m_healthmon; + struct xfs_healthmon __rcu *m_healthmon; } xfs_mount_t; #define M_IGEO(mp) (&(mp)->m_ino_geo)