On Wed, Jun 12, 2024 at 05:09:55PM +1000, NeilBrown wrote: > > When a file is opened and created with open(..., O_CREAT) we get > both the CREATE and OPEN fsnotify events and would expect them in that > order. For most filesystems we get them in that order because > open_last_lookups() calls fsnofify_create() and then do_open() (from > path_openat()) calls vfs_open()->do_dentry_open() which calls > fsnotify_open(). > > However when ->atomic_open is used, the > do_dentry_open() -> fsnotify_open() > call happens from finish_open() which is called from the ->atomic_open > handler in lookup_open() which is called *before* open_last_lookups() > calls fsnotify_create. So we get the "open" notification before > "create" - which is backwards. ltp testcase inotify02 tests this and > reports the inconsistency. > > This patch lifts the fsnotify_open() call out of do_dentry_open() and > places it higher up the call stack. There are three callers of > do_dentry_open(). > > For vfs_open() and kernel_file_open() the fsnotify_open() is placed > directly in that caller so there should be no behavioural change. > > For finish_open() there are two cases: > - finish_open is used in ->atomic_open handlers. For these we add a > call to fsnotify_open() at the top of do_open() if FMODE_OPENED is > set - which means do_dentry_open() has been called. > - finish_open is used in ->tmpfile() handlers. For these a similar > call to fsnotify_open() is added to vfs_tmpfile() > > With this patch NFSv3 is restored to its previous behaviour (before > ->atomic_open support was added) of generating CREATE notifications > before OPEN, and NFSv4 now has that same correct ordering that is has > not had before. I haven't tested other filesystems. > > Fixes: 7c6c5249f061 ("NFS: add atomic_open for NFSv3 to handle O_TRUNC correctly.") > Reported-by: James Clark <james.clark@xxxxxxx> > Closes: https://lore.kernel.org/all/01c3bf2e-eb1f-4b7f-a54f-d2a05dd3d8c8@xxxxxxx > Signed-off-by: NeilBrown <neilb@xxxxxxx> > --- We should take this is a bugfix because it doesn't change behavior. But then we should follow this up with a patch series that tries to rectify the open/close imbalance because I find that pretty ugly. That's at least my opinion. We should aim to only generate an open event when may_open() succeeds and don't generate a close event when the open has failed. Maybe: +#ifdef CONFIG_FSNOTIFY +#define file_nonotify(f) ((f)->f_mode |= __FMODE_NONOTIFY) +#else +#define file_nonotify(f) ((void)(f)) +#endif will do. Basic open permissions failing should count as failure to open and thus also turn of a close event. The somewhat ugly part is imho that security hooks introduce another layer of complexity. While we do count security_file_permission() as a failure to open we wouldn't e.g., count security_file_post_open() as a failure to open (Though granted here that "*_post_open()" makes it easier.). But it is really ugly that LSMs get to say "no" _after_ the file has been opened. I suspect this is some IMA or EVM thing where they hash the contents or something but it's royally ugly and I complained about this before. But maybe such things should just generate an LSM layer event via fsnotify in the future (FSNOTIFY_MAC) or something... Then userspace can see "Hey, the VFS said yes but then the MAC stuff said no."