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 <pankaj.raghav@linux.dev>
Fixes: a48373e7d35a89f6f ("xfs: start creating infrastructure for health monitoring")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
Reviewed-by: Pankaj Raghav <p.raghav@samsung.com>
Signed-off-by: Carlos Maiolino <cem@kernel.org>

diff --git a/fs/xfs/xfs_healthmon.c b/fs/xfs/xfs_healthmon.c
index e37c18cec37243c58743b77bd66625471abc0c7e..4a06d6632f65e2f2b2f08831bdbbb6488457524e 100644 (file)
--- 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 61c71128d171cbf028166d305436f20b9d5feba9..ddd4028be8d6bac3e4fae48e036b50387917cbe5 100644 (file)
--- 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)