Re: [RFC PATCH] ovl: keep merged and impure readdir caches separate

[Amir Goldstein]

Hi Nirmoy,

Thanks for the patch.

On Mon, May 11, 2026 at 8:21 AM Nirmoy Das <nirmoyd@xxxxxxxxxx> wrote:
>
> Overlayfs uses one inode cache slot for two readdir cache users with
> different lifetime rules. Merged directory iteration pins the cache from
> open directory files with cache->refcount. Impure real-directory iteration
> uses the inode cache as an unrefcounted lookup table.
>
> Those caches cannot be reused interchangeably. If merged iteration finds
> an impure cache in the inode slot, it can pin and seek through a cache
> that was not built for merged iteration. If impure iteration finds a merged
> cache, it can walk an object whose lifetime is controlled by open directory
> files. Either direction can leave ovl_iterate() using stale cache entries.
>
> Add ovl_dir_cache_drop() to detach the inode cache before freeing it. Keep
> refcounted merged caches alive until ovl_cache_put(), stop publishing new
> merged caches through the inode slot,

This is unacceptable - invalidating merged dir cache to paper over
another root bug, which was not really fixed.

> and let impure iteration reuse only
> unrefcounted caches.

All those guards are nice, but how does a directory inode change from
being merged to impure or vice versa?
This should never happen.

It took a lot of arguing with Sonnet about wrong leads to find the real bug.

Real bug was introduced by:
b79e05aaa1667 ("ovl: no direct iteration for dir with origin xattr")

It changed the test from:

od->is_real = !OVL_TYPE_MERGE(type);
od->is_upper = OVL_TYPE_UPPER(type);

to:

od->is_real = !ovl_test_flag(OVL_WHITEOUTS, d_inode(dir));
od->is_upper = OVL_TYPE_UPPER(type);

But there is a race window in copy up of a directory where
upper is set and published before the OVL_WHITEOUTS flag
is set.

an opendir() observing this state inside the race window will
wrongly start to iterate_real and another opendir later will
observe the flag and start iterate_merged - boom!

Attached is what I think is a correct fix.
WDYT?

Thanks,
Amir.
From 94d75948ceb70d5dc49e9187a742b9a2e42c33c0 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Mon, 11 May 2026 22:25:18 +0200
Subject: [PATCH] ovl: fix race between copy-up and open of a directory

Commit b79e05aaa166 ("ovl: no direct iteration for dir with origin xattr")
replaced the structural OVL_TYPE_MERGE check in od->is_real with the
OVL_WHITEOUTS flag, but set that flag after ovl_inode_update() published
the upper dentry via smp_wmb().  A concurrent open() that observes
__upperdentry via the smp_rmb pair may not yet see OVL_WHITEOUTS and
gets od->is_real=true on what is now a merged directory.

That stale od->is_real causes a subsequent getdents() to call
ovl_cache_get_impure(), which frees the merged refcounted cache stored
by another fd, resulting in a use-after-free.

Move the flag set into ovl_inode_update() before the smp_wmb() to close
the race window.

Fixes: b79e05aaa166 ("ovl: no direct iteration for dir with origin xattr")
Reported-by: syzbot+a16fb0cce329a320661c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=a16fb0cce329a320661c
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c | 2 --
 fs/overlayfs/util.c    | 9 +++++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 13cb60b52bd6e..f612ceab30af9 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -835,8 +835,6 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	if (!c->metacopy)
 		ovl_set_upperdata(inode);
 	ovl_inode_update(inode, temp);
-	if (S_ISDIR(inode->i_mode))
-		ovl_set_flag(OVL_WHITEOUTS, inode);
 out:
 	ovl_end_write(c->dentry);
 
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 7b86a6bac6449..7cd3201ae7497 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -595,6 +595,15 @@ void ovl_inode_update(struct inode *inode, struct dentry *upperdentry)
 
 	WARN_ON(OVL_I(inode)->__upperdentry);
 
+	/*
+	 * For a directory being copied up, set OVL_WHITEOUTS before publishing
+	 * the upper dentry, so a concurrent open() that observes __upperdentry
+	 * via the smp_wmb/rmb pair is guaranteed to also observe OVL_WHITEOUTS
+	 * and ovl_dir_is_real() returns false.
+	 */
+	if (S_ISDIR(inode->i_mode))
+		ovl_set_flag(OVL_WHITEOUTS, inode);
+
 	/*
 	 * Make sure upperdentry is consistent before making it visible
 	 */
-- 
2.54.0