[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250917113627.GA51799@dev-dsk-acsjakub-1b-6f9934e2.eu-west-1.amazon.com>
Date: Wed, 17 Sep 2025 11:36:27 +0000
From: Jakub Acs <acsjakub@...zon.de>
To: Amir Goldstein <amir73il@...il.com>
CC: Jan Kara <jack@...e.cz>, <linux-unionfs@...r.kernel.org>, Miklos Szeredi
<miklos@...redi.hu>, <linux-kernel@...r.kernel.org>,
<stable@...r.kernel.org>, Christian Brauner <brauner@...nel.org>,
linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] ovl: check before dereferencing s_root field
On Wed, Sep 17, 2025 at 01:07:45PM +0200, Amir Goldstein wrote:
> Might something naive as this be enough?
>
> Thanks,
> Amir.
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 60046ae23d514..8c9d0d6bb0045 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1999,10 +1999,12 @@ struct dentry *d_make_root(struct inode *root_inode)
>
> if (root_inode) {
> res = d_alloc_anon(root_inode->i_sb);
> - if (res)
> + if (res) {
> + root_inode->i_opflags |= IOP_ROOT;
> d_instantiate(res, root_inode);
> - else
> + } else {
> iput(root_inode);
> + }
> }
> return res;
> }
> diff --git a/fs/gfs2/export.c b/fs/gfs2/export.c
> index 3334c394ce9cb..809a09c6a89e0 100644
> --- a/fs/gfs2/export.c
> +++ b/fs/gfs2/export.c
> @@ -46,7 +46,7 @@ static int gfs2_encode_fh(struct inode *inode, __u32
> *p, int *len,
> fh[3] = cpu_to_be32(ip->i_no_addr & 0xFFFFFFFF);
> *len = GFS2_SMALL_FH_SIZE;
>
> - if (!parent || inode == d_inode(sb->s_root))
> + if (!parent || is_root_inode(inode))
> return *len;
>
> ip = GFS2_I(parent);
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 83f80fdb15674..7827c63354ad5 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -199,7 +199,7 @@ static int ovl_check_encode_origin(struct inode *inode)
> * Root is never indexed, so if there's an upper layer, encode upper for
> * root.
> */
> - if (inode == d_inode(inode->i_sb->s_root))
> + if (is_root_inode(inode))
> return 0;
>
> /*
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index ec867f112fd5f..ed84379aa06ca 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -665,6 +665,7 @@ is_uncached_acl(struct posix_acl *acl)
> #define IOP_DEFAULT_READLINK 0x0010
> #define IOP_MGTIME 0x0020
> #define IOP_CACHED_LINK 0x0040
> +#define IOP_ROOT 0x0080
> /*
> * Keep mostly read-only and often accessed (especially for
> @@ -2713,6 +2714,11 @@ static inline bool is_mgtime(const struct inode *inode)
> return inode->i_opflags & IOP_MGTIME;
> }
>
> +static inline bool is_root_inode(const struct inode *inode)
> +{
> + return inode->i_opflags & IOP_ROOT;
> +}
> +
> extern struct dentry *mount_bdev(struct file_system_type *fs_type,
> int flags, const char *dev_name, void *data,
> int (*fill_super)(struct super_block *, void *, int));
>
This would prevent the null-ptr-deref, but the encoding procedure would
continue (for non-root inode), potentially reaching other code paths
that assume fs is still mounted - could that maybe be a problem?
I had considered similar direction initially, too, but then decided I'm
unable to verify the paths and that it's safer to just fail if we detect
no root (or cannot take the lock).
Am I thinking wrong?
Jakub
Amazon Web Services Development Center Germany GmbH
Tamara-Danz-Str. 13
10243 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B
Sitz: Berlin
Ust-ID: DE 365 538 597
Powered by blists - more mailing lists