lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHk-=wjkBEch_Z9EMbup2bHtbtt7aoj-o5V6Nara+VxeUtckGw@mail.gmail.com> Date: Tue, 12 Nov 2024 12:12:20 -0800 From: Linus Torvalds <torvalds@...ux-foundation.org> To: Josef Bacik <josef@...icpanda.com> Cc: kernel-team@...com, linux-fsdevel@...r.kernel.org, jack@...e.cz, amir73il@...il.com, brauner@...nel.org, linux-xfs@...r.kernel.org, linux-btrfs@...r.kernel.org, linux-mm@...ck.org, linux-ext4@...r.kernel.org Subject: Re: [PATCH v7 05/18] fsnotify: introduce pre-content permission events On Tue, 12 Nov 2024 at 09:56, Josef Bacik <josef@...icpanda.com> wrote: > > #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > +static inline int fsnotify_pre_content(struct file *file) > +{ > + struct inode *inode = file_inode(file); > + > + /* > + * Pre-content events are only reported for regular files and dirs > + * if there are any pre-content event watchers on this sb. > + */ > + if ((!S_ISDIR(inode->i_mode) && !S_ISREG(inode->i_mode)) || > + !(inode->i_sb->s_iflags & SB_I_ALLOW_HSM) || > + !fsnotify_sb_has_priority_watchers(inode->i_sb, > + FSNOTIFY_PRIO_PRE_CONTENT)) > + return 0; > + > + return fsnotify_file(file, FS_PRE_ACCESS); > +} Yeah, no. None of this should check inode->i_sb->s_iflags at any point. The "is there a pre-content" thing should check one thing, and one thing only: that "is this file watched" flag. The whole indecipherable mess of inline functions that do random things in <linux/fsnotify.h> needs to be cleaned up, not made even more indecipherable. I'm NAKing this whole series until this is all sane and cleaned up, and I don't want to see a new hacky version being sent out tomorrow with just another layer of new hacks, with random new inline functions that call other inline functions and have complex odd conditionals that make no sense. Really. If the new hooks don't have that *SINGLE* bit test, they will not get merged. And that *SINGLE* bit test had better not be hidden under multiple layers of odd inline functions. You DO NOT get to use the same old broken complex function for the new hooks that then mix these odd helpers. This whole "add another crazy inline function using another crazy helper needs to STOP. Later on in the patch series you do +/* + * fsnotify_truncate_perm - permission hook before file truncate + */ +static inline int fsnotify_truncate_perm(const struct path *path, loff_t length) +{ + return fsnotify_pre_content(path, &length, 0); +} or things like this: +static inline bool fsnotify_file_has_pre_content_watches(struct file *file) +{ + if (!(file->f_mode & FMODE_NOTIFY_PERM)) + return false; + + if (!(file_inode(file)->i_sb->s_iflags & SB_I_ALLOW_HSM)) + return false; + + return fsnotify_file_object_watched(file, FSNOTIFY_PRE_CONTENT_EVENTS); +} and no, NONE of that should be tested at runtime. I repeat: you should have *ONE* inline function that basically does static inline bool fsnotify_file_watched(struct file *file) { return file && unlikely(file->f_mode & FMODE_NOTIFY_PERM); } and absolutely nothing else. If that file is set, the file has notification events, and you go to an out-of-line slow case. You don't inline the unlikely cases after that. And you make sure that you only set that special bit on files and filesystems that support it. You most definitely don't check for SB_I_ALLOW_HSM kind of flags at runtime in critical code. Linus
Powered by blists - more mailing lists