[PATCH v3 0/3] overlayfs: debugging check for valid superblock

[PATCH v3 0/3] overlayfs: debugging check for valid superblock

[Andrea Righi]

OVL_FS() is used to get a struct ovl_fs from a sturct super_block, but
we don't have any check to determine if the superblock is valid or not.

This can lead to unexpected behaviors or bugs that are pretty hard to
track down.

Add an explicit WARN_ON_ONCE() check to OVL_FS() to make sure it's
always used with a valid overlayfs superblock.

To avoid enabling this additional pendatic check everywhere, introduce
the new config option CONFIG_OVERLAY_FS_DEBUG, that can be used in the
future also for other additional debugging checks.

Maybe a nicer solution could be to return an error from OVL_FS() when
it's used with an invalid superblock and propagate the error in the rest
of overlayfs code, but for now having at least the possibility to
trigger a warning can help to catch potential bugs in advance.

Changelog (v2 -> v3):
 - do not hide is_ovl_fs_sb() under CONFIG_OVERLAY_FS_DEBUG
 - split consistent use of OVL_FS() and superblock validation in two
   separate patches

Changelog (v1 -> v2):
 - replace BUG_ON() with WARN_ON_ONCE()
 - introduce CONFIG_OVERLAY_FS_DEBUG

Andrea Righi (3):
      ovl: Kconfig: introduce CONFIG_OVERLAY_FS_DEBUG
      ovl: make consistent use of OVL_FS()
      ovl: validate superblock in OVL_FS()

 fs/overlayfs/Kconfig     |  9 +++++++++
 fs/overlayfs/copy_up.c   |  2 +-
 fs/overlayfs/export.c    | 10 +++++-----
 fs/overlayfs/inode.c     |  8 ++++----
 fs/overlayfs/namei.c     |  2 +-
 fs/overlayfs/ovl_entry.h | 14 ++++++++++++++
 fs/overlayfs/super.c     | 12 ++++++------
 fs/overlayfs/util.c      | 18 +++++++++---------
 8 files changed, 49 insertions(+), 26 deletions(-)

[PATCH v3 1/3] ovl: Kconfig: introduce CONFIG_OVERLAY_FS_DEBUG

[Andrea Righi]

Provide a Kconfig option to enable extra debugging checks for overlayfs.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
---
 fs/overlayfs/Kconfig | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
index 6708e54b0e30..fec5020c3495 100644
--- a/fs/overlayfs/Kconfig
+++ b/fs/overlayfs/Kconfig
@@ -124,3 +124,12 @@ config OVERLAY_FS_METACOPY
 	  that doesn't support this feature will have unexpected results.
 
 	  If unsure, say N.
+
+config OVERLAY_FS_DEBUG
+	bool "Overlayfs: turn on extra debugging checks"
+	default n
+	depends on OVERLAY_FS
+	help
+	  Say Y here to enable extra debugging checks in overlayfs.
+
+	  If unsure, say N.
-- 
2.39.2

[PATCH v3 2/3] ovl: make consistent use of OVL_FS()

[Andrea Righi]

Always use OVL_FS() to retrieve the corresponding struct ovl_fs from a
struct super_block.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
---
 fs/overlayfs/copy_up.c   |  2 +-
 fs/overlayfs/export.c    | 10 +++++-----
 fs/overlayfs/inode.c     |  8 ++++----
 fs/overlayfs/namei.c     |  2 +-
 fs/overlayfs/ovl_entry.h |  2 ++
 fs/overlayfs/super.c     | 12 ++++++------
 fs/overlayfs/util.c      | 18 +++++++++---------
 7 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index f658cc8ea492..60aa615820e7 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -905,7 +905,7 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
 static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode,
 				  int flags)
 {
-	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 
 	if (!ofs->config.metacopy)
 		return false;
diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index defd4e231ad2..f5f0ef8e3ce8 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -182,7 +182,7 @@ static int ovl_connect_layer(struct dentry *dentry)
  */
 static int ovl_check_encode_origin(struct dentry *dentry)
 {
-	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 
 	/* Upper file handle for pure upper */
 	if (!ovl_dentry_lower(dentry))
@@ -434,7 +434,7 @@ static struct dentry *ovl_lookup_real_inode(struct super_block *sb,
 					    struct dentry *real,
 					    const struct ovl_layer *layer)
 {
-	struct ovl_fs *ofs = sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(sb);
 	struct dentry *index = NULL;
 	struct dentry *this = NULL;
 	struct inode *inode;
@@ -655,7 +655,7 @@ static struct dentry *ovl_get_dentry(struct super_block *sb,
 				     struct ovl_path *lowerpath,
 				     struct dentry *index)
 {
-	struct ovl_fs *ofs = sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(sb);
 	const struct ovl_layer *layer = upper ? &ofs->layers[0] : lowerpath->layer;
 	struct dentry *real = upper ?: (index ?: lowerpath->dentry);
 
@@ -680,7 +680,7 @@ static struct dentry *ovl_get_dentry(struct super_block *sb,
 static struct dentry *ovl_upper_fh_to_d(struct super_block *sb,
 					struct ovl_fh *fh)
 {
-	struct ovl_fs *ofs = sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(sb);
 	struct dentry *dentry;
 	struct dentry *upper;
 
@@ -700,7 +700,7 @@ static struct dentry *ovl_upper_fh_to_d(struct super_block *sb,
 static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
 					struct ovl_fh *fh)
 {
-	struct ovl_fs *ofs = sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(sb);
 	struct ovl_path origin = { };
 	struct ovl_path *stack = &origin;
 	struct dentry *dentry = NULL;
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 541cf3717fc2..c27823f6e7aa 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -334,7 +334,7 @@ static const char *ovl_get_link(struct dentry *dentry,
 
 bool ovl_is_private_xattr(struct super_block *sb, const char *name)
 {
-	struct ovl_fs *ofs = sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(sb);
 
 	if (ofs->config.userxattr)
 		return strncmp(name, OVL_XATTR_USER_PREFIX,
@@ -689,7 +689,7 @@ int ovl_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
 int ovl_update_time(struct inode *inode, struct timespec64 *ts, int flags)
 {
 	if (flags & S_ATIME) {
-		struct ovl_fs *ofs = inode->i_sb->s_fs_info;
+		struct ovl_fs *ofs = OVL_FS(inode->i_sb);
 		struct path upperpath = {
 			.mnt = ovl_upper_mnt(ofs),
 			.dentry = ovl_upperdentry_dereference(OVL_I(inode)),
@@ -952,7 +952,7 @@ static inline void ovl_lockdep_annotate_inode_mutex_key(struct inode *inode)
 
 static void ovl_next_ino(struct inode *inode)
 {
-	struct ovl_fs *ofs = inode->i_sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(inode->i_sb);
 
 	inode->i_ino = atomic_long_inc_return(&ofs->last_ino);
 	if (unlikely(!inode->i_ino))
@@ -1284,7 +1284,7 @@ struct inode *ovl_get_trap_inode(struct super_block *sb, struct dentry *dir)
 static bool ovl_hash_bylower(struct super_block *sb, struct dentry *upper,
 			     struct dentry *lower, bool index)
 {
-	struct ovl_fs *ofs = sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(sb);
 
 	/* No, if pure upper */
 	if (!lower)
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index cfb3420b7df0..d0f196b85541 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -832,7 +832,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 {
 	struct ovl_entry *oe;
 	const struct cred *old_cred;
-	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 	struct ovl_entry *poe = dentry->d_parent->d_fsdata;
 	struct ovl_entry *roe = dentry->d_sb->s_root->d_fsdata;
 	struct ovl_path *stack = NULL, *origin_path = NULL;
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index fd11fe6d6d45..b32c38fdf3c7 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -95,6 +95,8 @@ static inline struct mnt_idmap *ovl_upper_mnt_idmap(struct ovl_fs *ofs)
 	return mnt_idmap(ovl_upper_mnt(ofs));
 }
 
+extern struct file_system_type ovl_fs_type;
+
 static inline struct ovl_fs *OVL_FS(struct super_block *sb)
 {
 	return (struct ovl_fs *)sb->s_fs_info;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index f97ad8b40dbb..2ed0f498fce4 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -259,7 +259,7 @@ static void ovl_free_fs(struct ovl_fs *ofs)
 
 static void ovl_put_super(struct super_block *sb)
 {
-	struct ovl_fs *ofs = sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(sb);
 
 	ovl_free_fs(ofs);
 }
@@ -267,7 +267,7 @@ static void ovl_put_super(struct super_block *sb)
 /* Sync real dirty inodes in upper filesystem (if it exists) */
 static int ovl_sync_fs(struct super_block *sb, int wait)
 {
-	struct ovl_fs *ofs = sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(sb);
 	struct super_block *upper_sb;
 	int ret;
 
@@ -315,7 +315,7 @@ static int ovl_sync_fs(struct super_block *sb, int wait)
  */
 static int ovl_statfs(struct dentry *dentry, struct kstatfs *buf)
 {
-	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 	struct dentry *root_dentry = dentry->d_sb->s_root;
 	struct path path;
 	int err;
@@ -364,7 +364,7 @@ static inline int ovl_xino_def(void)
 static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
 {
 	struct super_block *sb = dentry->d_sb;
-	struct ovl_fs *ofs = sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(sb);
 
 	seq_show_option(m, "lowerdir", ofs->config.lowerdir);
 	if (ofs->config.upperdir) {
@@ -396,7 +396,7 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
 
 static int ovl_remount(struct super_block *sb, int *flags, char *data)
 {
-	struct ovl_fs *ofs = sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(sb);
 	struct super_block *upper_sb;
 	int ret = 0;
 
@@ -2083,7 +2083,7 @@ static struct dentry *ovl_mount(struct file_system_type *fs_type, int flags,
 	return mount_nodev(fs_type, flags, raw_data, ovl_fill_super);
 }
 
-static struct file_system_type ovl_fs_type = {
+struct file_system_type ovl_fs_type = {
 	.owner		= THIS_MODULE,
 	.name		= "overlay",
 	.fs_flags	= FS_USERNS_MOUNT,
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 923d66d131c1..2425240ef139 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -18,25 +18,25 @@
 
 int ovl_want_write(struct dentry *dentry)
 {
-	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 	return mnt_want_write(ovl_upper_mnt(ofs));
 }
 
 void ovl_drop_write(struct dentry *dentry)
 {
-	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 	mnt_drop_write(ovl_upper_mnt(ofs));
 }
 
 struct dentry *ovl_workdir(struct dentry *dentry)
 {
-	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 	return ofs->workdir;
 }
 
 const struct cred *ovl_override_creds(struct super_block *sb)
 {
-	struct ovl_fs *ofs = sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(sb);
 
 	return override_creds(ofs->creator_cred);
 }
@@ -62,7 +62,7 @@ int ovl_can_decode_fh(struct super_block *sb)
 
 struct dentry *ovl_indexdir(struct super_block *sb)
 {
-	struct ovl_fs *ofs = sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(sb);
 
 	return ofs->indexdir;
 }
@@ -70,7 +70,7 @@ struct dentry *ovl_indexdir(struct super_block *sb)
 /* Index all files on copy up. For now only enabled for NFS export */
 bool ovl_index_all(struct super_block *sb)
 {
-	struct ovl_fs *ofs = sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(sb);
 
 	return ofs->config.nfs_export && ofs->config.index;
 }
@@ -78,7 +78,7 @@ bool ovl_index_all(struct super_block *sb)
 /* Verify lower origin on lookup. For now only enabled for NFS export */
 bool ovl_verify_lower(struct super_block *sb)
 {
-	struct ovl_fs *ofs = sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(sb);
 
 	return ofs->config.nfs_export && ofs->config.index;
 }
@@ -152,7 +152,7 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry)
 
 void ovl_path_upper(struct dentry *dentry, struct path *path)
 {
-	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 
 	path->mnt = ovl_upper_mnt(ofs);
 	path->dentry = ovl_dentry_upper(dentry);
@@ -415,7 +415,7 @@ bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags)
 
 bool ovl_redirect_dir(struct super_block *sb)
 {
-	struct ovl_fs *ofs = sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(sb);
 
 	return ofs->config.redirect_dir && !ofs->noxattr;
 }
-- 
2.39.2

[PATCH v3 3/3] ovl: validate superblock in OVL_FS()

[Andrea Righi]

When CONFIG_OVERLAY_FS_DEBUG is enabled add an explicit check to make
sure that OVL_FS() is always used with a valid overlayfs superblock.
Otherwise trigger a WARN_ON_ONCE().

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
---
 fs/overlayfs/ovl_entry.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index b32c38fdf3c7..e156649d9c71 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -97,8 +97,20 @@ static inline struct mnt_idmap *ovl_upper_mnt_idmap(struct ovl_fs *ofs)
 
 extern struct file_system_type ovl_fs_type;
 
+static inline bool is_ovl_fs_sb(struct super_block *sb)
+{
+	return sb->s_type == &ovl_fs_type;
+}
+
+#ifdef CONFIG_OVERLAY_FS_DEBUG
+#define OVL_VALIDATE_SB(__sb)	WARN_ON_ONCE(!is_ovl_fs_sb(__sb))
+#else
+#define OVL_VALIDATE_SB(__sb)
+#endif
+
 static inline struct ovl_fs *OVL_FS(struct super_block *sb)
 {
+	OVL_VALIDATE_SB(sb);
 	return (struct ovl_fs *)sb->s_fs_info;
 }
 
-- 
2.39.2

Re: [PATCH v3 3/3] ovl: validate superblock in OVL_FS()

[Miklos Szeredi]

On Sun, 21 May 2023 at 10:28, Andrea Righi <andrea.righi@canonical.com> wrote:
>
> When CONFIG_OVERLAY_FS_DEBUG is enabled add an explicit check to make
> sure that OVL_FS() is always used with a valid overlayfs superblock.
> Otherwise trigger a WARN_ON_ONCE().
>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> ---
>  fs/overlayfs/ovl_entry.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index b32c38fdf3c7..e156649d9c71 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -97,8 +97,20 @@ static inline struct mnt_idmap *ovl_upper_mnt_idmap(struct ovl_fs *ofs)
>
>  extern struct file_system_type ovl_fs_type;
>
> +static inline bool is_ovl_fs_sb(struct super_block *sb)
> +{
> +       return sb->s_type == &ovl_fs_type;
> +}
> +
> +#ifdef CONFIG_OVERLAY_FS_DEBUG
> +#define OVL_VALIDATE_SB(__sb)  WARN_ON_ONCE(!is_ovl_fs_sb(__sb))
> +#else
> +#define OVL_VALIDATE_SB(__sb)
> +#endif
> +
>  static inline struct ovl_fs *OVL_FS(struct super_block *sb)
>  {
> +       OVL_VALIDATE_SB(sb);

This could be written simply and naturally:

    if (IS_ENABLED(CONFIG_OVERLAY_FS_DEBUG))
         WARN_ON_ONCE(sb->s_type != &ovl_fs_type)

Thanks,
Miklos

Re: [PATCH v3 3/3] ovl: validate superblock in OVL_FS()

[Amir Goldstein]

On Mon, Jul 24, 2023 at 5:43 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Sun, 21 May 2023 at 10:28, Andrea Righi <andrea.righi@canonical.com> wrote:
> >
> > When CONFIG_OVERLAY_FS_DEBUG is enabled add an explicit check to make
> > sure that OVL_FS() is always used with a valid overlayfs superblock.
> > Otherwise trigger a WARN_ON_ONCE().
> >
> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> > ---
> >  fs/overlayfs/ovl_entry.h | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> > index b32c38fdf3c7..e156649d9c71 100644
> > --- a/fs/overlayfs/ovl_entry.h
> > +++ b/fs/overlayfs/ovl_entry.h
> > @@ -97,8 +97,20 @@ static inline struct mnt_idmap *ovl_upper_mnt_idmap(struct ovl_fs *ofs)
> >
> >  extern struct file_system_type ovl_fs_type;
> >
> > +static inline bool is_ovl_fs_sb(struct super_block *sb)
> > +{
> > +       return sb->s_type == &ovl_fs_type;
> > +}
> > +
> > +#ifdef CONFIG_OVERLAY_FS_DEBUG
> > +#define OVL_VALIDATE_SB(__sb)  WARN_ON_ONCE(!is_ovl_fs_sb(__sb))
> > +#else
> > +#define OVL_VALIDATE_SB(__sb)
> > +#endif
> > +
> >  static inline struct ovl_fs *OVL_FS(struct super_block *sb)
> >  {
> > +       OVL_VALIDATE_SB(sb);
>
> This could be written simply and naturally:
>
>     if (IS_ENABLED(CONFIG_OVERLAY_FS_DEBUG))
>          WARN_ON_ONCE(sb->s_type != &ovl_fs_type)
>

Andrea,

There is an inherent challenge with a cleanup series like this one
that touches many functions to avoid merge conflicts with other
devel branches. I did not try, but I expect there are conflicts
with the current overlayfs-next branch.

I also see at least one new direct reference of sb->s_fs_info
in ovl_maybe_validate_verity().

Please make sure to base your next submission on overlayfs-next
branch from git://git.kernel.org/pub/scm/linux/kernel/git/overlayfs/vfs.git.

Once you do that, we could apply your patches to overlayfs-next
so they won't get stale again.

Thanks,
Amir.

Re: [PATCH v3 3/3] ovl: validate superblock in OVL_FS()

[Amir Goldstein]

On Fri, Aug 11, 2023 at 12:14 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Mon, Jul 24, 2023 at 5:43 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Sun, 21 May 2023 at 10:28, Andrea Righi <andrea.righi@canonical.com> wrote:
> > >
> > > When CONFIG_OVERLAY_FS_DEBUG is enabled add an explicit check to make
> > > sure that OVL_FS() is always used with a valid overlayfs superblock.
> > > Otherwise trigger a WARN_ON_ONCE().
> > >
> > > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > > Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> > > ---
> > >  fs/overlayfs/ovl_entry.h | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> > > index b32c38fdf3c7..e156649d9c71 100644
> > > --- a/fs/overlayfs/ovl_entry.h
> > > +++ b/fs/overlayfs/ovl_entry.h
> > > @@ -97,8 +97,20 @@ static inline struct mnt_idmap *ovl_upper_mnt_idmap(struct ovl_fs *ofs)
> > >
> > >  extern struct file_system_type ovl_fs_type;
> > >
> > > +static inline bool is_ovl_fs_sb(struct super_block *sb)
> > > +{
> > > +       return sb->s_type == &ovl_fs_type;
> > > +}
> > > +
> > > +#ifdef CONFIG_OVERLAY_FS_DEBUG
> > > +#define OVL_VALIDATE_SB(__sb)  WARN_ON_ONCE(!is_ovl_fs_sb(__sb))
> > > +#else
> > > +#define OVL_VALIDATE_SB(__sb)
> > > +#endif
> > > +
> > >  static inline struct ovl_fs *OVL_FS(struct super_block *sb)
> > >  {
> > > +       OVL_VALIDATE_SB(sb);
> >
> > This could be written simply and naturally:
> >
> >     if (IS_ENABLED(CONFIG_OVERLAY_FS_DEBUG))
> >          WARN_ON_ONCE(sb->s_type != &ovl_fs_type)
> >
>
> Andrea,
>
> There is an inherent challenge with a cleanup series like this one
> that touches many functions to avoid merge conflicts with other
> devel branches. I did not try, but I expect there are conflicts
> with the current overlayfs-next branch.
>
> I also see at least one new direct reference of sb->s_fs_info
> in ovl_maybe_validate_verity().
>
> Please make sure to base your next submission on overlayfs-next
> branch from git://git.kernel.org/pub/scm/linux/kernel/git/overlayfs/vfs.git.
>
> Once you do that, we could apply your patches to overlayfs-next
> so they won't get stale again.

Nevermind, I had rebased overlayfs-next, so already applied your
patches with the needed conflict resolutions and addressed Miklos' comment.

Thanks,
Amir.

Re: [PATCH v3 0/3] overlayfs: debugging check for valid superblock

[Amir Goldstein]

On Sun, May 21, 2023 at 11:28 AM Andrea Righi
<andrea.righi@canonical.com> wrote:
>
> OVL_FS() is used to get a struct ovl_fs from a sturct super_block, but
> we don't have any check to determine if the superblock is valid or not.
>
> This can lead to unexpected behaviors or bugs that are pretty hard to
> track down.
>
> Add an explicit WARN_ON_ONCE() check to OVL_FS() to make sure it's
> always used with a valid overlayfs superblock.
>
> To avoid enabling this additional pendatic check everywhere, introduce
> the new config option CONFIG_OVERLAY_FS_DEBUG, that can be used in the
> future also for other additional debugging checks.
>
> Maybe a nicer solution could be to return an error from OVL_FS() when
> it's used with an invalid superblock and propagate the error in the rest
> of overlayfs code, but for now having at least the possibility to
> trigger a warning can help to catch potential bugs in advance.
>

Applied to overlayfs-next with following changes:
- open code WARN_ON_ONCE() in OVL_FS() (Miklos)
- rebase and add missing OVL_FS() accessors

Thanks,
Amir.

> Changelog (v2 -> v3):
>  - do not hide is_ovl_fs_sb() under CONFIG_OVERLAY_FS_DEBUG
>  - split consistent use of OVL_FS() and superblock validation in two
>    separate patches
>
> Changelog (v1 -> v2):
>  - replace BUG_ON() with WARN_ON_ONCE()
>  - introduce CONFIG_OVERLAY_FS_DEBUG
>
> Andrea Righi (3):
>       ovl: Kconfig: introduce CONFIG_OVERLAY_FS_DEBUG
>       ovl: make consistent use of OVL_FS()
>       ovl: validate superblock in OVL_FS()
>
>  fs/overlayfs/Kconfig     |  9 +++++++++
>  fs/overlayfs/copy_up.c   |  2 +-
>  fs/overlayfs/export.c    | 10 +++++-----
>  fs/overlayfs/inode.c     |  8 ++++----
>  fs/overlayfs/namei.c     |  2 +-
>  fs/overlayfs/ovl_entry.h | 14 ++++++++++++++
>  fs/overlayfs/super.c     | 12 ++++++------
>  fs/overlayfs/util.c      | 18 +++++++++---------
>  8 files changed, 49 insertions(+), 26 deletions(-)
>

Re: [PATCH v3 3/3] ovl: validate superblock in OVL_FS()

[Andrea Righi]

On Sat, Aug 12, 2023 at 07:26:01PM +0300, Amir Goldstein wrote:
> On Fri, Aug 11, 2023 at 12:14 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Mon, Jul 24, 2023 at 5:43 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Sun, 21 May 2023 at 10:28, Andrea Righi <andrea.righi@canonical.com> wrote:
> > > >
> > > > When CONFIG_OVERLAY_FS_DEBUG is enabled add an explicit check to make
> > > > sure that OVL_FS() is always used with a valid overlayfs superblock.
> > > > Otherwise trigger a WARN_ON_ONCE().
> > > >
> > > > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > > > Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> > > > ---
> > > >  fs/overlayfs/ovl_entry.h | 12 ++++++++++++
> > > >  1 file changed, 12 insertions(+)
> > > >
> > > > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> > > > index b32c38fdf3c7..e156649d9c71 100644
> > > > --- a/fs/overlayfs/ovl_entry.h
> > > > +++ b/fs/overlayfs/ovl_entry.h
> > > > @@ -97,8 +97,20 @@ static inline struct mnt_idmap *ovl_upper_mnt_idmap(struct ovl_fs *ofs)
> > > >
> > > >  extern struct file_system_type ovl_fs_type;
> > > >
> > > > +static inline bool is_ovl_fs_sb(struct super_block *sb)
> > > > +{
> > > > +       return sb->s_type == &ovl_fs_type;
> > > > +}
> > > > +
> > > > +#ifdef CONFIG_OVERLAY_FS_DEBUG
> > > > +#define OVL_VALIDATE_SB(__sb)  WARN_ON_ONCE(!is_ovl_fs_sb(__sb))
> > > > +#else
> > > > +#define OVL_VALIDATE_SB(__sb)
> > > > +#endif
> > > > +
> > > >  static inline struct ovl_fs *OVL_FS(struct super_block *sb)
> > > >  {
> > > > +       OVL_VALIDATE_SB(sb);
> > >
> > > This could be written simply and naturally:
> > >
> > >     if (IS_ENABLED(CONFIG_OVERLAY_FS_DEBUG))
> > >          WARN_ON_ONCE(sb->s_type != &ovl_fs_type)
> > >
> >
> > Andrea,
> >
> > There is an inherent challenge with a cleanup series like this one
> > that touches many functions to avoid merge conflicts with other
> > devel branches. I did not try, but I expect there are conflicts
> > with the current overlayfs-next branch.
> >
> > I also see at least one new direct reference of sb->s_fs_info
> > in ovl_maybe_validate_verity().
> >
> > Please make sure to base your next submission on overlayfs-next
> > branch from git://git.kernel.org/pub/scm/linux/kernel/git/overlayfs/vfs.git.
> >
> > Once you do that, we could apply your patches to overlayfs-next
> > so they won't get stale again.
> 
> Nevermind, I had rebased overlayfs-next, so already applied your
> patches with the needed conflict resolutions and addressed Miklos' comment.

Sorry for the late response, I was on vacation (with a poor internet
connection). However, it looks like there's not much to do for me at
this point, thanks for taking care of this! :)

-Andrea

> 
> Thanks,
> Amir.