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

[Amir Goldstein]

On Sat, May 20, 2023 at 4:19 PM Andrea Righi <andrea.righi@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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.