Received-SPF: pass (google.com: domain of linux-kernel+bounces-211605-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 12 Jun 2024 15:53:08 +0200
From: Christian Brauner <brauner@kernel.org>
To: NeilBrown <neilb@suse.de>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>, Jan Kara <jack@suse.cz>, 
	Amir Goldstein <amir73il@gmail.com>, James Clark <james.clark@arm.com>, ltp@lists.linux.it, 
	linux-nfs@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>, 
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v2] VFS: generate FS_CREATE before FS_OPEN when
 ->atomic_open used.
Message-ID: <20240612-nennung-ungnade-ae9bdc5f8c4c@brauner>
References: <171817619547.14261.975798725161704336@noble.neil.brown.name>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <171817619547.14261.975798725161704336@noble.neil.brown.name>

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@arm.com>
> Closes: https://lore.kernel.org/all/01c3bf2e-eb1f-4b7f-a54f-d2a05dd3d8c8@arm.com
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---

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."