Re: [RFC PATCH] ovl: keep merged and impure readdir caches separate
Amir Goldstein <[email protected]> Mon, 11 May 2026 22:54:26 +0200
| Newsgroups | org.kernel.vger.linux-unionfs,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <CAOQ4uxh-1J2OBjnDVbrrrjy1puSOW0kABvghN85ih6dtKUBTyA@mail.gmail.com> |
Hi Nirmoy, Thanks for the patch. On Mon, May 11, 2026 at 8:21 AM Nirmoy Das <[email protected]> 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.
0001-ovl-fix-race-between-copy-up-and-open-of-a-directory.patch
(application/x-patch, 2.4 KB)
From 94d75948ceb70d5dc49e9187a742b9a2e42c33c0 Mon Sep 17 00:00:00 2001 From: Amir Goldstein <[email protected]> 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: [email protected] Closes: https://syzkaller.appspot.com/bug?extid=a16fb0cce329a320661c Signed-off-by: Amir Goldstein <[email protected]> --- 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