[PATCH] ovl: make consistent use of OVL_FS()

[PATCH] 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 and make sure that it is exclusively used with an
overlayfs superblock (otherwise, trigger a BUG).

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 |  4 ++++
 fs/overlayfs/super.c     | 10 +++++-----
 fs/overlayfs/util.c      | 18 +++++++++---------
 7 files changed, 29 insertions(+), 25 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..b91b3694ae26 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -5,6 +5,8 @@
  * Copyright (C) 2016 Red Hat, Inc.
  */
 
+#include <uapi/linux/magic.h>
+
 struct ovl_config {
 	char *lowerdir;
 	char *upperdir;
@@ -97,6 +99,8 @@ static inline struct mnt_idmap *ovl_upper_mnt_idmap(struct ovl_fs *ofs)
 
 static inline struct ovl_fs *OVL_FS(struct super_block *sb)
 {
+	/* Make sure OVL_FS() is always used with an overlayfs superblock */
+	BUG_ON(sb->s_magic != OVERLAYFS_SUPER_MAGIC);
 	return (struct ovl_fs *)sb->s_fs_info;
 }
 
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index f97ad8b40dbb..879d601ba61e 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;
 
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

Re: [PATCH] ovl: make consistent use of OVL_FS()

[Amir Goldstein]

On Sat, May 20, 2023 at 3:20 PM Andrea Righi <andrea.righi@canonical.com> wrote:
>
> Always use OVL_FS() to retrieve the corresponding struct ovl_fs from a

I don't mind this cleanup, but...

> struct super_block and make sure that it is exclusively used with an
> overlayfs superblock (otherwise, trigger a BUG).
>
> 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 |  4 ++++
>  fs/overlayfs/super.c     | 10 +++++-----
>  fs/overlayfs/util.c      | 18 +++++++++---------
>  7 files changed, 29 insertions(+), 25 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..b91b3694ae26 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -5,6 +5,8 @@
>   * Copyright (C) 2016 Red Hat, Inc.
>   */
>
> +#include <uapi/linux/magic.h>
> +
>  struct ovl_config {
>         char *lowerdir;
>         char *upperdir;
> @@ -97,6 +99,8 @@ static inline struct mnt_idmap *ovl_upper_mnt_idmap(struct ovl_fs *ofs)
>
>  static inline struct ovl_fs *OVL_FS(struct super_block *sb)
>  {
> +       /* Make sure OVL_FS() is always used with an overlayfs superblock */
> +       BUG_ON(sb->s_magic != OVERLAYFS_SUPER_MAGIC);

1. Adding new BUG_ON to kernel code is not acceptable - if anything
    you can add WARN_ON_ONCE()
2. If anything, you should check s_type == s_ovl_fs_type, not s_magic
3. It is very unclear to me that this check has that much value and OVL_FS()
    macro is very commonly used inside internal helpers, so please add a
    "why" to your patch - why do you think that it is desired and/or valuable
    to fortify OVL_FS() like this?

Thanks,
Amir.

Re: [PATCH] ovl: make consistent use of OVL_FS()

[Andrea Righi]

On Sat, May 20, 2023 at 03:33:32PM +0300, Amir Goldstein wrote:
> On Sat, May 20, 2023 at 3:20 PM Andrea Righi <andrea.righi@canonical.com> wrote:
...
> > @@ -97,6 +99,8 @@ static inline struct mnt_idmap *ovl_upper_mnt_idmap(struct ovl_fs *ofs)
> >
> >  static inline struct ovl_fs *OVL_FS(struct super_block *sb)
> >  {
> > +       /* Make sure OVL_FS() is always used with an overlayfs superblock */
> > +       BUG_ON(sb->s_magic != OVERLAYFS_SUPER_MAGIC);
> 
> 1. Adding new BUG_ON to kernel code is not acceptable - if anything
>     you can add WARN_ON_ONCE()

OK, but accessing a pointer to a struct ovl_fs that is not really a
struct ovl_fs can potentially have nasty effects, even data corruption
maybe? I'd rather crash the system now rather than experiencing random
behaviors later...

> 2. If anything, you should check s_type == s_ovl_fs_type, not s_magic

Hm.. is there a fast way to determine when sb->s_type == overlayfs?
Using get_fs_type() here seems quite expensive and I'm not even sure if
it's doable, is there a better way that I don't see?

> 3. It is very unclear to me that this check has that much value and OVL_FS()
>     macro is very commonly used inside internal helpers, so please add a
>     "why" to your patch - why do you think that it is desired and/or valuable
>     to fortify OVL_FS() like this?

Sure, I can send a v2 explaining why I think this is needed. Basically I
was debugging a custom overlayfs patch and after a while I realized that
I was accessing the sb->s_fs_info of a real path (not an overlayfs sb),
using OVL_FS() with a proper check would have saved a me a bunch of
time.

Thanks for looking at this!
-Andrea

Re: [PATCH] ovl: make consistent use of OVL_FS()

[Amir Goldstein]

On Sat, May 20, 2023 at 4:19 PM Andrea Righi <andrea.righi@canonical.com> wrote:
>
> On Sat, May 20, 2023 at 03:33:32PM +0300, Amir Goldstein wrote:
> > On Sat, May 20, 2023 at 3:20 PM Andrea Righi <andrea.righi@canonical.com> wrote:
> ...
> > > @@ -97,6 +99,8 @@ static inline struct mnt_idmap *ovl_upper_mnt_idmap(struct ovl_fs *ofs)
> > >
> > >  static inline struct ovl_fs *OVL_FS(struct super_block *sb)
> > >  {
> > > +       /* Make sure OVL_FS() is always used with an overlayfs superblock */
> > > +       BUG_ON(sb->s_magic != OVERLAYFS_SUPER_MAGIC);
> >
> > 1. Adding new BUG_ON to kernel code is not acceptable - if anything
> >     you can add WARN_ON_ONCE()
>
> OK, but accessing a pointer to a struct ovl_fs that is not really a
> struct ovl_fs can potentially have nasty effects, even data corruption
> maybe? I'd rather crash the system now rather than experiencing random
> behaviors later...
>

What you would rather do does not matter here.
No new BUG_ON() is a rule set by Linus.
Yes, some people (security people mostly) will prefer to crash the system
over an "undefined" behavior later, but many non-security people would
much rather have some processes stuck or crash than losing access to
the entire system.
There is no one good answer, but it is Linus who gets to decide.

> > 2. If anything, you should check s_type == s_ovl_fs_type, not s_magic
>
> Hm.. is there a fast way to determine when sb->s_type == overlayfs?
> Using get_fs_type() here seems quite expensive and I'm not even sure if
> it's doable, is there a better way that I don't see?
>

Not sure what you mean.
This is what I meant:

+extern struct file_system_type ovl_fs_type;
+
 static inline struct ovl_fs *OVL_FS(struct super_block *sb)
 {
+       WARN_ON_ONCE(sb->s_type != &ovl_fs_type);
+
        return (struct ovl_fs *)sb->s_fs_info;
 }

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index f97ad8b40dbb..0c1f9d1e9135 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -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",

> > 3. It is very unclear to me that this check has that much value and OVL_FS()
> >     macro is very commonly used inside internal helpers, so please add a
> >     "why" to your patch - why do you think that it is desired and/or valuable
> >     to fortify OVL_FS() like this?
>
> Sure, I can send a v2 explaining why I think this is needed. Basically I
> was debugging a custom overlayfs patch and after a while I realized that
> I was accessing the sb->s_fs_info of a real path (not an overlayfs sb),
> using OVL_FS() with a proper check would have saved a me a bunch of
> time.
>

I think if you add this extra pedantic check, it should be ifdefed with
some Kconfig for extra debugging.

Maybe you could add CONFIG_OVERLAY_FS_DEBUG like some
other fs have. I am not sure if fortifying OVL_FS() is worth it, but
maybe this config will gain more pedantics checks in the future.

It's really up to Miklos to decide if this is interesting for overlayfs.

Thanks,
Amir.

Re: [PATCH] ovl: make consistent use of OVL_FS()

[Andrea Righi]

On Sat, May 20, 2023 at 06:55:44PM +0300, Amir Goldstein wrote:
> On Sat, May 20, 2023 at 4:19 PM Andrea Righi <andrea.righi@canonical.com> wrote:
> >
> > On Sat, May 20, 2023 at 03:33:32PM +0300, Amir Goldstein wrote:
> > > On Sat, May 20, 2023 at 3:20 PM Andrea Righi <andrea.righi@canonical.com> wrote:
> > ...
> > > > @@ -97,6 +99,8 @@ static inline struct mnt_idmap *ovl_upper_mnt_idmap(struct ovl_fs *ofs)
> > > >
> > > >  static inline struct ovl_fs *OVL_FS(struct super_block *sb)
> > > >  {
> > > > +       /* Make sure OVL_FS() is always used with an overlayfs superblock */
> > > > +       BUG_ON(sb->s_magic != OVERLAYFS_SUPER_MAGIC);
> > >
> > > 1. Adding new BUG_ON to kernel code is not acceptable - if anything
> > >     you can add WARN_ON_ONCE()
> >
> > OK, but accessing a pointer to a struct ovl_fs that is not really a
> > struct ovl_fs can potentially have nasty effects, even data corruption
> > maybe? I'd rather crash the system now rather than experiencing random
> > behaviors later...
> >
> 
> What you would rather do does not matter here.
> No new BUG_ON() is a rule set by Linus.
> Yes, some people (security people mostly) will prefer to crash the system
> over an "undefined" behavior later, but many non-security people would
> much rather have some processes stuck or crash than losing access to
> the entire system.
> There is no one good answer, but it is Linus who gets to decide.

I see, WARN_ON_ONCE() then seems more appropriate.

> 
> > > 2. If anything, you should check s_type == s_ovl_fs_type, not s_magic
> >
> > Hm.. is there a fast way to determine when sb->s_type == overlayfs?
> > Using get_fs_type() here seems quite expensive and I'm not even sure if
> > it's doable, is there a better way that I don't see?
> >
> 
> Not sure what you mean.
> This is what I meant:
> 
> +extern struct file_system_type ovl_fs_type;
> +
>  static inline struct ovl_fs *OVL_FS(struct super_block *sb)
>  {
> +       WARN_ON_ONCE(sb->s_type != &ovl_fs_type);
> +
>         return (struct ovl_fs *)sb->s_fs_info;
>  }
> 
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index f97ad8b40dbb..0c1f9d1e9135 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -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",

Ah yes, we need to make ovl_fs_type non static. If it's acceptable, that
would work.

> 
> > > 3. It is very unclear to me that this check has that much value and OVL_FS()
> > >     macro is very commonly used inside internal helpers, so please add a
> > >     "why" to your patch - why do you think that it is desired and/or valuable
> > >     to fortify OVL_FS() like this?
> >
> > Sure, I can send a v2 explaining why I think this is needed. Basically I
> > was debugging a custom overlayfs patch and after a while I realized that
> > I was accessing the sb->s_fs_info of a real path (not an overlayfs sb),
> > using OVL_FS() with a proper check would have saved a me a bunch of
> > time.
> >
> 
> I think if you add this extra pedantic check, it should be ifdefed with
> some Kconfig for extra debugging.
> 
> Maybe you could add CONFIG_OVERLAY_FS_DEBUG like some
> other fs have. I am not sure if fortifying OVL_FS() is worth it, but
> maybe this config will gain more pedantics checks in the future.
> 
> It's really up to Miklos to decide if this is interesting for overlayfs.

I like the CONFIG_OVERLAY_FS_DEBUG idea. I'll send a v2 with these
changes, let's see if there's some interest in it. Thank you so much for
your suggestions!

-Andrea