On Tue, Nov 12, 2024 at 8:46 PM Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > On Tue, 12 Nov 2024 at 09:56, Josef Bacik <josef@xxxxxxxxxxxxxx> wrote: > > > > @@ -119,14 +118,37 @@ static inline int fsnotify_file(struct file *file, __u32 mask) > > * handle creation / destruction events and not "real" file events. > > */ > > if (file->f_mode & (FMODE_NONOTIFY | FMODE_PATH)) > > + return false; > > + > > + /* Permission events require that watches are set before FS_OPEN_PERM */ > > + if (mask & ALL_FSNOTIFY_PERM_EVENTS & ~FS_OPEN_PERM && > > + !(file->f_mode & FMODE_NOTIFY_PERM)) > > + return false; > > This still all looks very strange. > > As far as I can tell, there is exactly one user of FS_OPEN_PERM in > 'mask', and that's fsnotify_open_perm(). Which is called in exactly > one place: security_file_open(), which is the wrong place to call it > anyway and is the only place where fsnotify is called from the > security layer. > > In fact, that looks like an active bug: if you enable FSNOTIFY, but > you *don't* enable CONFIG_SECURITY, the whole fsnotify_open_perm() > will never be called at all. > > And I just verified that yes, you can very much generate such a config. > See: 1cda52f1b461 fsnotify, lsm: Decouple fsnotify from lsm in linux-next. This patch set is based on the fs-next branch. > So the whole FS_OPEN_PERM thing looks like a special case, called from > a (broken) special place, and now polluting this "fsnotify_file()" > logic for no actual reason and making it all look unnecessarily messy. > > I'd suggest that the whole fsnotify_open_perm() simply be moved to > where it *should* be - in the open path - and not make a bad and > broken attempt at hiding inside the security layer, and not use this > "fsnotify_file()" logic at all. > > The open-time logic is different. It shouldn't even attempt - badly - > to look like it's the same thing as some regular file access. > OK, we can move setting the FMODE_NOTIFY_PERM to the open path. I have considered that it may be better to unhide it, but wasn't sure. Thanks, Amir.
