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-=wgUV27XF8g23=aWNJecRbn8fCDDW2=10y9yJ122+d8JrA@mail.gmail.com> Date: Mon, 11 Nov 2024 16:37:30 -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 v6 06/17] fsnotify: generate pre-content permission event on open On Mon, 11 Nov 2024 at 16:00, Amir Goldstein <amir73il@...il.com> wrote: > > I think that's a good idea for pre-content events, because it's fine > to say that if the sb/mount was not watched by a pre-content event listener > at the time of file open, then we do not care. Right. > The problem is that legacy inotify/fanotify watches can be added after > file is open, so that is allegedly why this optimization was not done for > fsnotify hooks in the past. So honestly, even if the legacy fsnotify hooks can't look at the file flag, they could damn well look at an inode flag. And I'm not even convinced that we couldn't fix them to just look at a file flag, and say "tough luck, somebody opened that file before you started watching, you don't get to see what they did". So even if we don't look at a file->f_mode flag, the lergacy cases should look at i_fsnotify_mask, and do that *first*. IOW, not do it like fsnotify_object_watched() does now, which is just broken. Again, it looks at inode->i_sb->s_fsnotify_mask completely pointlessly, but it also does it much too late - it gets called after we've already called into the fsnotify() code and have messed up the I$ etc. The "linode->i_sb->s_fsnotify_mask" is not only an extra indirection, it should be very *literally* pointless. If some bit isn't set in i_sb->s_fsnotify_mask, then there should be no way to set that bit in inode->i_fsnotify_mask. So the only time we should access i_sb->s_fsnotify_mask is when i_notify_mask gets *modified*, not when it gets tested. But even if that silly and pointless i_sb->s_fsnotify_mask thing is removed, fsnotify_object_watched() is *still* wrong, because it requires that mnt_mask as an argument, which means that the caller now has to look it up - all this entirely pointless work that should never be done if the bit wasn't set in inode->i_fsnotify_mask. So I really think fsnotify is doing *everything* wrong. And I most certainly don't want to add more runtime hooks to *critical* code like open/read/write. Right now, many of the fsnotify things are for "metadata", ie for bigger file creation / removal / move etc. And yes, the "don't do this if there are no fsnotify watchers AT ALL" does actually end up meaning that most of the time I never see any of it in profiles, because the fsnotify_sb_has_watchers() culls out that case. And while the fsnotify_sb_has_watchers() thing is broken garbage and does too many indirections and is not testing the right thing, at least it's inlined and you don't get the function calls. That doesn't make fsnotify "right", but at least it's not in my face. I see the sb accesses, and I hate them, but it's usually at least hidden. Admittedly not as well hidden as it *should* be, since it does the access tests in the wrong order, but the old fsnotify_open() doesn't strike me as "terminally broken". It doesn't have a permission test after the open has already done things, and it's inlined enough that it isn't actively offensive. And most of the other fsnotify things have the same pattern - not great, but not actively offensive. These new patches make it in my face. So I do require that the *new* cases at least get it right. The fact that we have old code that is misdesigned and gets it wrong and should also be improved isn't an excuse to add *more* badly coded stuff. And yes, if somebody fixes the old fsnotify stuff to check just the i_fsnotify_mask in the inline function, and moves all the other silly checks out-of-line, that would be an improvement. I'd very much applaud that. But it's a separate thing from adding new hooks. Linus
Powered by blists - more mailing lists