On Mon, Dec 9, 2024 at 1:26 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Mon 09-12-24 13:11:04, Jan Kara wrote:
> > > Then I took a closer look at the function called in the problematic code
> > > and noticed that fsnotify_file_area_perm(), is a NOOP when
> > > CONFIG_FANOTIFY_ACCESS_PERMISSIONS is not set (which was the case in my
> > > .config). This also explains why this was not found before, as
> > > distributional .config file have this option enabled. Setting the option
> > > to y solves the issue, too
> >
> > Well, I agree with you on all the points but the real question is, how come
> > the test FMODE_FSNOTIFY_HSM(file->f_mode) was true on our kernel when you
> > clearly don't run HSM software, even more so with
> > CONFIG_FANOTIFY_ACCESS_PERMISSIONS disabled. That's the real cause of this
> > problem. Something fishy is going on here... checking...
> >
> > Ah, because I've botched out file_set_fsnotify_mode() in case
> > CONFIG_FANOTIFY_ACCESS_PERMISSIONS is disabled. This should fix the
> > problem:
> >
> > index 1a9ef8f6784d..778a88fcfddc 100644
> > --- a/include/linux/fsnotify.h
> > +++ b/include/linux/fsnotify.h
> > @@ -215,6 +215,7 @@ static inline int fsnotify_open_perm(struct file *file)
> > #else
> > static inline void file_set_fsnotify_mode(struct file *file)
> > {
> > + file->f_mode |= FMODE_NONOTIFY_PERM;
> > }
> >
> > I'm going to test this with CONFIG_FANOTIFY_ACCESS_PERMISSIONS disabled and
> > push out a fixed version. Thanks again for the report and analysis!
>
> So this was not enough, What we need is:
> index 1a9ef8f6784d..778a88fcfddc 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -215,6 +215,10 @@ static inline int fsnotify_open_perm(struct file *file)
> #else
> static inline void file_set_fsnotify_mode(struct file *file)
> {
> + /* Is it a file opened by fanotify? */
> + if (FMODE_FSNOTIFY_NONE(file->f_mode))
> + return;
> + file->f_mode |= FMODE_NONOTIFY_PERM;
> }
>
> This passes testing for me so I've pushed it out and the next linux-next
> build should have this fix.
This fix is not obvious to the code reviewer (especially when that is
reviewer Linus...)
Perhaps it would be safer and less hidden to do:
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -211,11 +211,16 @@ typedef int (dio_iodone_t)(struct kiocb *iocb,
loff_t offset,
#define FMODE_FSNOTIFY_NONE(mode) \
((mode & FMODE_FSNOTIFY_MASK) == FMODE_NONOTIFY)
+#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
#define FMODE_FSNOTIFY_PERM(mode) \
((mode & FMODE_FSNOTIFY_MASK) == 0 || \
(mode & FMODE_FSNOTIFY_MASK) == (FMODE_NONOTIFY | FMODE_NONOTIFY_PERM))
#define FMODE_FSNOTIFY_HSM(mode) \
((mode & FMODE_FSNOTIFY_MASK) == 0)
+#else
+#define FMODE_FSNOTIFY_PERM(mode) 0
+#define FMODE_FSNOTIFY_HSM(mode) 0
+#endif
Similar to IS_POSIXACL()
Thanks,
Amir.
