Message-ID: <CAOQ4uxgFJX+AJbswKwQP3oFE273JDOO3UAvtxHz4r8+tVkHJnQ@mail.gmail.com>
Date: Wed, 13 Nov 2024 23:35:16 +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 Wed, Nov 13, 2024 at 10:22 PM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> On Wed, 13 Nov 2024 at 11:11, Amir Goldstein <amir73il@...il.com> wrote:
> >
> > >
> > > 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.
>
> So I don't mind adding a new inline function for convenience.
>
> But I do mind the whole "multiple levels of inline functions" model,
> and the thing I _particularly_ hate is the "mask is usually constant
> so that the effect of the inline function is practically two different
> things" as exemplified by "fsnotify_file()" and friends.
>
> At that point, the inline function isn't a helper any more, it's a
> hindrance to understanding what the heck is going on.
>
> Basically, as an example: fsnotify_file() is actually two very
> different things depending on the "mask" argument, an that argument is
> *typically* a constant.
>
> In fact, in fsnotify_file_area_perm() is very much is a constant, but
> to make it extra non-obvious, it's a *hidden* constant, using
>
>         __u32 fsnotify_mask = FS_ACCESS_PERM;
>
> to hide the fact that it's actually calling fsnotify_file() with that
> constant argument.

Yeh, that specific "obfuscation" is a leftover from history.
It is already gone in the patches that we sent.

>
> And in fsnotify_open() it's not exactly a constant, but it's kind of
> one: when you actually look at fsnotify_file(), it has that "I do a
> different filtering event based on mask", and the two different
> constants fsnotify_open() uses are actually the same for that mask.
>
> In other words, that whole "mask" test part of fsnotify_file()
>
>         /* Permission events require group prio >= FSNOTIFY_PRIO_CONTENT */
>         if (mask & ALL_FSNOTIFY_PERM_EVENTS &&
>             !fsnotify_sb_has_priority_watchers(path->dentry->d_sb,
>                                                FSNOTIFY_PRIO_CONTENT))
>                 return 0;
>
> mess is actually STATICALLY TRUE OR FALSE, but it's made out to be
> somehow an "arghumenty" to the function, and it's really obfuscated.
>

Yeh. I see that problem absolutely.
This is already gone in the patch that I send you today:
- All the old hooks call fsnotify_file() that only checks FMODE_NONOTIFY
  and calls fsnotify_path()
- The permission hooks now check FMODE_NONOTIFY_PERM
  and call fsnotify_path()

> That is the kind of "helper inline" that I don't want to see in the
> new paths. Making that conditional more complicated was part of what I
> objected to in one of the patches.
>
> > Those convenience helpers help me to maintain readability and code
> > reuse.
>
> ABSOLUTELY NOT.
>
> That "convenience helkper" does exactly the opposite. It explicitly
> and actively obfuscates when the actual
> fsnotify_sb_has_priority_watchers() filtering is done.
>
> That helper is evil.
>
> Just go and look at the actual uses, let's take
> fsnotify_file_area_perm() as an example. As mentioned, as an extra
> level of obfuscation, that horrid "helper" function tries to hide how
> "mask" is constant by doing
>
>         __u32 fsnotify_mask = FS_ACCESS_PERM;
>
> and then never modifying it, and then doing
>
>         return fsnotify_file(file, fsnotify_mask);
>
> but if you walk through the logic, you now see that ok, that means
> that the "mask" conditional fsnotify_file() is actually just
>
>     FS_ACCESS_PERM & ALL_FSNOTIFY_PERM_EVENTS
>
> which is always true, so it means that fsnotify_file_area_perm()
> unconditionally does that
>
>     fsnotify_sb_has_priority_watchers(..)
>
> filitering.
>
> And dammit, you shouldn't have to walk through that pointless "helper"
> variable, and that pointless "helper" inline function to see that. It
> shouldn't be the case that fsnotify_file() does two completely
> different things based on a constant argument.
>

ok. that's going to be history soon.
I will send this cleanup patch regardless of the pre-content series.

> It would have literally been much clearer to just have two explicitly
> different versions of that function, *WITHOUT* some kind of
> pseudo-conditional that isn't actually a conditional, and just have
> fsnotify_file_area_perm() be very explicit about the fact that it uses
> the fsnotify_sb_has_priority_watchers() logic.
>
> IOW, that conditional only makes it harder to see what the actual
> rules are. For no good reason.
>
> Look, magically for some reason fsnotify_name() could do the same
> thing without this kind of silly obfuscation. It just unconditonally
> calls fsnotify_sb_has_watchers() to filter the events. No silly games
> with doing two entirely different things based on a random constant
> argument.
>
> So this is why I say that any new fsnotify events will be NAK'ed and
> not merged by me unless it's all obvious, and unless it all obviously
> DOES NOT USE these inline garbage "helper" functions.
>
> The new logic had better be very obviously *only* using the
> file->f_mode bits, and just calling out-of-line to do the work. If I
> have to walk through several layers of inline functions, and look at
> what magic arguments those inline functions get just to see what the
> hell they actually do, I'm not going to merge it.

Sure for new hooks with new check-on-open semantics that is
going to be easy to do. The historic reason for the heavy inlining
is trying to optimize out indirect calls when we do not have the
luxury of using the check-on-open semantics.

>
> Because I'm really tired of actively obfuscated VFS hooks that use
> inline functions to hide what the hell they are doing and whether they
> are expensive or not.
>
> Your fsnotify_file_range() uses fsnotify_parent(), which is another of
> those "it does two different things" functions that either call
> fsnotify() on the dentry, or call __fsnotify_parent() on it if it's an
> inode, which means that it's another case of "what does this actually
> do" which is pointlessly hard to follow, since clearly for a truncate
> event it can't be a directory.
>
> And to make matters worse, fsnotify_truncate_perm() actually checks
> truncate events for directories and regular files, when truncates
> can't actually happen for anything but regular files in the first
> place. So  your helper function does a nonsensical cray-cray test that
> shouldn't exist.

Ha, right, that's a stupid copy&paste braino.
Easy to fix.

The simplest thing to do for the new hooks I think is to make
fsnotify_file_range() extern and then you won't need to look beyond it,
because it already comes after the unlikley FMODE_NONOTIFY_ bits check.

Will work on the rest of the series following those guidelines.
Let me know if the patch I sent you has taken a wrong direction.

Thanks,
Amir.

Powered by blists - more mailing lists