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: <CAOQ4uxjQHh=fUnBw=KwuchjRt_4JbaZAqrkDd93E2_mrqv_Pkw@mail.gmail.com> Date: Wed, 13 Nov 2024 20:11:39 +0100 From: Amir Goldstein <amir73il@...il.com> To: Linus Torvalds <torvalds@...ux-foundation.org> Cc: Josef Bacik <josef@...icpanda.com>, kernel-team@...com, linux-fsdevel@...r.kernel.org, jack@...e.cz, 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, Nov 12, 2024 at 9:12 PM Linus Torvalds <torvalds@...ux-foundation.org> wrote: > > 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. Up to here I understand. > > This whole "add another crazy inline function using another crazy > helper needs to STOP. Later on in the patch series you do > The patch that I sent did add another convenience helper fsnotify_path(), but as long as it is not hiding crazy tests, and does not expand to huge inlined code, I don't see the problem. Those convenience helpers help me to maintain readability and code reuse. I do agree that the new hooks that can use the new open-time check semantics should not expand to huge inlined code. > +/* > + * 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); > +} > This example that you pointed at, I do not understand. truncate() does not happen on an open file, so I cannot use the FMODE_NONOTIFY_ test. This is what I have in my WIP branch: static inline int fsnotify_file_range(const struct path *path, const loff_t *ppos, size_t count) { struct file_range range; const void *data; int data_type; /* Report page aligned range only when pos is known */ if (ppos) { range.path = path; range.pos = PAGE_ALIGN_DOWN(*ppos); range.count = PAGE_ALIGN(*ppos + count) - range.pos; data = ⦥ data_type = FSNOTIFY_EVENT_FILE_RANGE; } else { data = path; data_type = FSNOTIFY_EVENT_PATH; } return fsnotify_parent(path->dentry, FS_PRE_ACCESS, data, data_type); } /* * fsnotify_truncate_perm - permission hook before file truncate */ static inline int fsnotify_truncate_perm(const struct path *path, loff_t length) { struct inode *inode = d_inode(path->dentry); /* * 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) || !unlikely(fsnotify_sb_has_priority_watchers(inode->i_sb, FSNOTIFY_PRIO_PRE_CONTENT))) return 0; return fsnotify_file_range(path, &length, 0); } fsnotify_file_range() does not need to be inlined, but I do want to reuse the code of fsnotify_file_range() which is also called for the common file pre-access hook. So did you mean that the unlikely stuff (i.e. fsnotify_file_range()) should be an indirect call? or something else? Thanks, Amir.
Powered by blists - more mailing lists