Re: [PATCH 0/4] Prepare for fsnotify pre-content permission events

From: Amir Goldstein <amir73il@gmail.com>
To: Josef Bacik <josef@toxicpanda.com>
Cc: Christian Brauner <brauner@kernel.org>, Jan Kara <jack@suse.cz>,
	Jeff Layton <jlayton@kernel.org>,  Christoph Hellwig <hch@lst.de>,
	David Howells <dhowells@redhat.com>, Jens Axboe <axboe@kernel.dk>,
	 Miklos Szeredi <miklos@szeredi.hu>,
	Al Viro <viro@zeniv.linux.org.uk>,
	 linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 0/4] Prepare for fsnotify pre-content permission events
Date: Fri, 15 Dec 2023 19:00:08 +0200	[thread overview]
Message-ID: <CAOQ4uxj5GuOk7FdrZYdDVMmvp+CSBJty0SzsHN2T50NUBRFV4Q@mail.gmail.com> (raw)
In-Reply-To: <CAOQ4uxiBGNmHcYCg2r_=pWFJVwx0WPmdmqQyrzDQdgWsiUTNYA@mail.gmail.com>

On Fri, Dec 8, 2023 at 9:34 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, Dec 7, 2023 at 11:51 PM Josef Bacik <josef@toxicpanda.com> wrote:
> >
> > On Thu, Dec 07, 2023 at 02:38:21PM +0200, Amir Goldstein wrote:
> > > Hi Jan & Christian,
> > >
> > > I am not planning to post the fanotify pre-content event patches [1]
> > > for 6.8.  Not because they are not ready, but because the usersapce
> > > example is not ready.
> > >
> > > Also, I think it is a good idea to let the large permission hooks
> > > cleanup work to mature over the 6.8 cycle, before we introduce the
> > > pre-content events.
> > >
> > > However, I would like to include the following vfs prep patches along
> > > with the vfs.rw PR for 6.8, which could be titled as the subject of
> > > this cover letter.
> > >
> > > Patch 1 is a variant of a cleanup suggested by Christoph to get rid
> > > of the generic_copy_file_range() exported symbol.
> > >
> > > Patches 2,3 add the file_write_not_started() assertion to fsnotify
> > > file permission hooks.  IMO, it is important to merge it along with
> > > vfs.rw because:
> > >
> > > 1. This assert is how I tested vfs.rw does what it aimed to achieve
> > > 2. This will protect us from new callers that break the new order
> > > 3. The commit message of patch 3 provides the context for the entire
> > >    series and can be included in the PR message
> > >
> > > Patch 4 is the final change of fsnotify permission hook locations/args
> > > and is the last of the vfs prerequsites for pre-content events.
> > >
> > > If we merge patch 4 for 6.8, it will be much easier for the development
> > > of fanotify pre-content events in 6.9 dev cycle, which be contained
> > > within the fsnotify subsystem.
> >
> > Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> >
> > Can you get an fstest added that exercises the freeze deadlock?
>
> I suppose that you mean a test that exercises the lockdep assertion?
> This is much easier to do, so I don't see the point in actually testing
> the deadlock. The only thing is that the assertion will not be backported
> so this test would protect us from future regression, but will not nudge
> stable kernel users to backport the deadlock fix, which I don't think they
> should be doing anyway.
>
> It is actually already exercised by tests overlay/068,069, but I can add
> a generic test to get wider testing coverage.

Here is a WIP test:
https://github.com/amir73il/xfstests/commits/start-write-safe

I tested it by reverting "fs: move file_start_write() into
direct_splice_actor()"
and seeing that it triggers the assert.

Thanks,
Amir.