On Mon, Sep 15, 2025 at 4:07 PM Jan Kara jack@suse.cz wrote:
On Mon 15-09-25 15:01:13, Amir Goldstein wrote:
On Mon, Sep 15, 2025 at 12:15 PM Jakub Acs acsjakub@amazon.de wrote:
Calling intotify_show_fdinfo() on fd watching an overlayfs inode, while the overlayfs is being unmounted, can lead to dereferencing NULL ptr.
This issue was found by syzkaller.
Race Condition Diagram:
Thread 1 Thread 2
generic_shutdown_super() shrink_dcache_for_umount sb->s_root = NULL
| | vfs_read() | inotify_fdinfo() | * inode get from mark * | show_mark_fhandle(m, inode) | exportfs_encode_fid(inode, ..) | ovl_encode_fh(inode, ..) | ovl_check_encode_origin(inode) | * deref i_sb->s_root * | | vfsnotify_sb_delete(sb)
Which then leads to:
[ 32.133461] Oops: general protection fault, probably for non-canonical address 0xdffffc0000000006: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN NOPTI [ 32.134438] KASAN: null-ptr-deref in range [0x0000000000000030-0x0000000000000037] [ 32.135032] CPU: 1 UID: 0 PID: 4468 Comm: systemd-coredum Not tainted 6.17.0-rc6 #22 PREEMPT(none)
<snip registers, unreliable trace>
[ 32.143353] Call Trace: [ 32.143732] ovl_encode_fh+0xd5/0x170 [ 32.144031] exportfs_encode_inode_fh+0x12f/0x300 [ 32.144425] show_mark_fhandle+0xbe/0x1f0 [ 32.145805] inotify_fdinfo+0x226/0x2d0 [ 32.146442] inotify_show_fdinfo+0x1c5/0x350 [ 32.147168] seq_show+0x530/0x6f0 [ 32.147449] seq_read_iter+0x503/0x12a0 [ 32.148419] seq_read+0x31f/0x410 [ 32.150714] vfs_read+0x1f0/0x9e0 [ 32.152297] ksys_read+0x125/0x240
IOW ovl_check_encode_origin derefs inode->i_sb->s_root, after it was set to NULL in the unmount path.
Minimize the window of opportunity by adding explicit check.
Fixes: c45beebfde34 ("ovl: support encoding fid from inode with no alias") Signed-off-by: Jakub Acs acsjakub@amazon.de Cc: Miklos Szeredi miklos@szeredi.hu Cc: Amir Goldstein amir73il@gmail.com Cc: linux-unionfs@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org
I'm happy to take suggestions for a better fix - I looked at taking s_umount for reading, but it wasn't clear to me for how long would the fdinfo path need to hold it. Hence the most primitive suggestion in this v1.
I'm also not sure if ENOENT or EBUSY is better?.. or even something else?
fs/overlayfs/export.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c index 83f80fdb1567..424c73188e06 100644 --- a/fs/overlayfs/export.c +++ b/fs/overlayfs/export.c @@ -195,6 +195,8 @@ static int ovl_check_encode_origin(struct inode *inode) if (!ovl_inode_lower(inode)) return 0;
if (!inode->i_sb->s_root)return -ENOENT;For a filesystem method to have to check that its own root is still alive sounds like the wrong way to me. That's one of the things that should be taken for granted by fs code.
I don't think this is an overlayfs specific issue, because other fs would be happy if encode_fh() would be called with NULL sb->s_root.
Actually, I don't see where that would blow up? Generally references to sb->s_root in filesystems outside of mount / remount code are pretty rare. Also most of the code should be unreachable by the time we set sb->s_root to NULL because there are no open files at that moment, no exports etc. But as this report shows, there are occasional surprises (I remember similar issue with ext4 sysfs files handlers using s_root without checking couple years back).
I am not sure that I understand what you are arguing for. I did a very naive grep s_root fs/*/export.c and quickly found:
static int gfs2_encode_fh(struct inode *inode, __u32 *p, int *len, struct inode *parent) { ... if (!parent || inode == d_inode(sb->s_root)) return *len;
So it's not an overlayfs specific issue, just so happens that zysbot likes to test overlayfs.
Are you suggesting that we fix all of those one by one?
Jan,
Can we change the order of generic_shutdown_super() so that fsnotify_sb_delete(sb) is called before setting s_root to NULL?
Or is there a better solution for this race?
Regarding calling fsnotify_sb_delete() before setting s_root to NULL: In 2019 (commit 1edc8eb2e9313 ("fs: call fsnotify_sb_delete after evict_inodes")) we've moved the call after evict_inodes() because otherwise we were just wasting cycles scanning many inodes without watches. So moving it earlier wouldn't be great...
Yes, I noticed that and I figured there were subtleties.
In any case, Jakub, your patch is insufficient because: 1. Checking sb->sb_root without a lock and without READ_ONCE() and a matching WRITE_ONCE() is not safe 2. sb_root can become NULL after the check since you are not holding the s_umount lock
Jakub,
Instead of an unsafe check inside ovl_encode_fh(), I think it is better to use super_trylock_shared() inside show_mark_fhandle() before calling exportfs_encode_fid()?
Feels like the corner case is show_mark_fhandle() and there is no strong incentive to make this code very efficient.
Jan, WDYT?
Thanks, Amir.