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-=wirrmNUD9mD5OByfJ3XFb7rgept4kARNQuA+xCHTSDhyw@mail.gmail.com> Date: Wed, 13 Nov 2024 13:22:17 -0800 From: Linus Torvalds <torvalds@...ux-foundation.org> To: Amir Goldstein <amir73il@...il.com> 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, 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. 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. 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. 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. 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. That makes it another "this is not a helper function, this is just obfuscation". Linus
Powered by blists - more mailing lists