On Fri, Aug 2, 2024 at 5:59 PM Josef Bacik <josef@xxxxxxxxxxxxxx> wrote: > > On Thu, Aug 01, 2024 at 05:15:06PM +0200, Mateusz Guzik wrote: > > I'm not confident this is particularly valuable, but if it is, it > > probably should hide behind some debug flags. > > I'm still going to disagree here, putting it behind a debug flag means it'll > never get caught, and it obviously proved valuable because we're discussing this > particular case. > > Is it racy? Yup sure. I think that your solution is the right way to fix it, > and then we can have a > > WARN_ON(!(file->f_mode & FMODE_NO_EXEC_CHECKED)); > > or however we choose to flag the file, that way we are no longer racing with the > mount flags and only validating that a check that should have already occurred > has in fact occurred. Thanks, > To my understanding the submitter ran into the thing tripping over the racy check, so this check did not find a real bug elsewhere in this instance. The only case that I know of where this fired and found a real problem was after ntfs constructed a bogus inode: https://lore.kernel.org/linux-fsdevel/20230818191239.3cprv2wncyyy5yxj@f/ But that is a deficiency in debug facilities in the vfs layer -- this only tripped over because syzkaller tried to exec a sufficiently bogus inode, while the vfs layer should have prevented that from happening to begin with. There should be well-defined spot where the filesystem claims the inode in fully constructed at which the vfs layer verifies its state (thus in particular i_mode). If implemented it would have caught the problem before the inode escaped ntfs and presumably would find some other problems which the kernel as is does not correctly report. This in part depends on someone(tm) implementing VFS_* debug macros first, preferably in a way which can dump inode info on assertion failure. I have this at the bottom on a TODO list for a rainy day. However, it's not my call to make as to what to do here. I outlined my $0,04 and I'm buggering off. -- Mateusz Guzik <mjguzik gmail.com>
