archives/linux
1
0
Fork 0
mirror of https://github.com/torvalds/linux.git synced 2026-03-08 01:04:41 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions

xfs: fix potential pointer access race in xfs_healthmon_get

Browse source 
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: a48373e7d3 ("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>
This commit is contained in:
Darrick J. Wong 2026-02-18 15:25:37 -08:00 • committed by Carlos Maiolino
parent eb8550fb75
commit 94014a23e9
2 changed files with 8 additions and 5 deletions
Download patch file Download diff file Expand all files Collapse all files

11
fs/xfs/xfs_healthmon.c
View file

@ -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);

2
fs/xfs/xfs_mount.h
View file

@ -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)