Message-ID: <CAOQ4uxiZfJYHrYmE2k0vWrgbLLbDQ2LTrVggYwL3pv4FUyjctQ@mail.gmail.com>
Date: Tue, 4 Mar 2025 17:41:35 +0100
From: Amir Goldstein <amir73il@...il.com>
To: Seyediman Seyedarab <imandevel@...il.com>
Cc: jack@...e.cz, linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-kernel-mentees@...ts.linux.dev
Subject: Re: [PATCH] inotify: disallow watches on unsupported filesystems

On Tue, Mar 4, 2025 at 5:05 PM Seyediman Seyedarab <imandevel@...il.com> wrote:
>
> On 25/03/04 12:57PM, Amir Goldstein wrote:
> > On Tue, Mar 4, 2025 at 8:59 AM Seyediman Seyedarab <imandevel@...il.com> wrote:
> > >
> > > currently, inotify_add_watch() allows adding watches on filesystems
> > > where inotify does not work correctly, without returning an explicit
> > > error. This behavior is misleading and can cause confusion for users
> > > expecting inotify to work on a certain filesystem.
> >
> > That maybe so, but it's not that inotify does not work at all,
> > in fact it probably works most of the time for those fs,
> > so there may be users setting inotify watches on those fs,
> > so it is not right to regress those users.
> >
> > >
> > > This patch explicitly rejects inotify usage on filesystems where it
> > > is known to be unreliable, such as sysfs, procfs, overlayfs, 9p, fuse,
> > > and others.
> >
> > Where did you get this list of fs from?
> > Why do you claim that inotify does not work on overlayfs?
> > Specifically, there are two LTP tests inotify07 and inotify08
> > that test inotify over overlayfs.
> >
> > This makes me question other fs on your list.
>
> Thanks for the review! I may have overlooked overlayfs, but these
> following discussions led me to include it in the blacklist:
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/882147
> https://github.com/libuv/libuv/issues/1201
> https://github.com/moby/moby/issues/11705
>
> Apparently, things have changed in v4.10, so I may have been wrong
> about overlayfs. I can test each filesystem and provide a response
> instead of blindly relying on various bug reports. However, let's
> first discuss whether the patch is necessary in the first place.
>
> > >
> > > By returning -EOPNOTSUPP, the limitation is made explicit, preventing
> > > users from making incorrect assumptions about inotify behavior.
> > >
> > > Signed-off-by: Seyediman Seyedarab <ImanDevel@...il.com>
> > > ---
> > >  fs/notify/inotify/inotify_user.c | 22 ++++++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > >
> > > diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> > > index b372fb2c56bd..9b96438f4d46 100644
> > > --- a/fs/notify/inotify/inotify_user.c
> > > +++ b/fs/notify/inotify/inotify_user.c
> > > @@ -87,6 +87,13 @@ static const struct ctl_table inotify_table[] = {
> > >         },
> > >  };
> > >
> > > +static const unsigned long unwatchable_fs[] = {
> > > +       PROC_SUPER_MAGIC,      SYSFS_MAGIC,       TRACEFS_MAGIC,
> > > +       DEBUGFS_MAGIC,        CGROUP_SUPER_MAGIC, SECURITYFS_MAGIC,
> > > +       RAMFS_MAGIC,          DEVPTS_SUPER_MAGIC, BPF_FS_MAGIC,
> > > +       OVERLAYFS_SUPER_MAGIC, FUSE_SUPER_MAGIC,   NFS_SUPER_MAGIC
> > > +};
> > > +
> > >  static void __init inotify_sysctls_init(void)
> > >  {
> > >         register_sysctl("fs/inotify", inotify_table);
> > > @@ -690,6 +697,14 @@ static struct fsnotify_group *inotify_new_group(unsigned int max_events)
> > >  }
> > >
> > >
> > > +static inline bool is_unwatchable_fs(struct inode *inode)
> > > +{
> > > +       for (int i = 0; i < ARRAY_SIZE(unwatchable_fs); i++)
> > > +               if (inode->i_sb->s_magic == unwatchable_fs[i])
> > > +                       return true;
> > > +       return false;
> > > +}
> >
> > This is not a good practice for black listing fs.
> >
> > See commit 0b3b094ac9a7b ("fanotify: Disallow permission events
> > for proc filesystem") for a better practice, but again, we cannot just
> > stop supporting inotify on fs where it was supported.
>
> Following the same approach as 0b3b094ac9a7b ("fanotify: Disallow
> permission events for the proc filesystem") would require setting
> a specific flag for each fs that isn't supported by inotify. If this
> is more suitable, I can work on implementing it.
>
> I understand why it might seem like disallowing users from monitoring
> these filesystems could break userspace in some way. BUT, programs
> work incorrectly precisely because they do not receive any information
> from the kernel, so in other words they are already broken. There is no
> way for them to know if the fs is supported or not. I mean, even we are
> not sure at the moment, then how would they know.
>

Programs not knowing is a problem that could be fixed with a new API
or new init flag to fanotify/inotify.

Existing programs that would break due to this change is unacceptable.

> As an example, 'Waybar' is a userspace program affected by this patch.
> Since it relies on monitoring sysfs, it isn't working properly anyway.
> This is also due to the issue mentioned earlier... inotify_add_watch()
> returns without an error, so the developers haven't realized that
> inotify isn't actually supported on sysfs. There are over five
> discussions regarding this issue that you can find them here:
> https://github.com/Alexays/Waybar/pull/3474
>

You need to distinguish "inotify does not work"
from "inotify does not notify on 'remote' changes"
that is changes that happen on the network fs server or inside the
kernel (in sysfs case) vs. changes that happen via vfs syscalls
on the mounted fs, be it p9, cifs, sysfs.

There are several discussions about supporting "remote change"
notifications for network filesystems - this is a more complex problem.

In any case, I believe performing operations on the mounted fs
generated inotify events for all the fs that you listed and without
a claim that nobody is using this facility we cannot regress this
behavior without an opt-in from the application.

Thanks,
Amir.

Powered by blists - more mailing lists