On Tue, Nov 12, 2024 at 9:12 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, 12 Nov 2024 at 09:56, Josef Bacik <josef@xxxxxxxxxxxxxx> 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.
