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@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.

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