Re: [PATCH 3/4] fsnotify: assert that file_start_write() is not held in permission hooks

From: Amir Goldstein <amir73il@gmail.com>
To: Jan Kara <jack@suse.cz>
Cc: Christian Brauner <brauner@kernel.org>,
	Jeff Layton <jlayton@kernel.org>,
	 Josef Bacik <josef@toxicpanda.com>,
	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 3/4] fsnotify: assert that file_start_write() is not held in permission hooks
Date: Fri, 8 Dec 2023 23:02:35 +0200	[thread overview]
Message-ID: <CAOQ4uxgHNBSBenADnMkqZWmb3t2qzfhG-E722-0KJ=Cwzf2UYw@mail.gmail.com> (raw)
In-Reply-To: <20231208184608.n5fcrkj3peancy3u@quack3>

On Fri, Dec 8, 2023 at 8:46 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 07-12-23 14:38:24, Amir Goldstein wrote:
> > filesystem may be modified in the context of fanotify permission events
> > (e.g. by HSM service), so assert that sb freeze protection is not held.
> >
> > If the assertion fails, then the following deadlock would be possible:
> >
> > CPU0                          CPU1                    CPU2
> > -------------------------------------------------------------------------
> > file_start_write()#0
> > ...
> >   fsnotify_perm()
> >     fanotify_get_response() =>        (read event and fill file)
> >                               ...
> >                               ...                     freeze_super()
> >                               ...                       sb_wait_write()
> >                               ...
> >                               vfs_write()
> >                                 file_start_write()#1
> >
> > This example demonstrates a use case of an hierarchical storage management
> > (HSM) service that uses fanotify permission events to fill the content of
> > a file before access, while a 3rd process starts fsfreeze.
> >
> > This creates a circular dependeny:
> >   file_start_write()#0 => fanotify_get_response =>
> >     file_start_write()#1 =>
> >       sb_wait_write() =>
> >         file_end_write()#0
> >
> > Where file_end_write()#0 can never be called and none of the threads can
> > make progress.
> >
> > The assertion is checked for both MAY_READ and MAY_WRITE permission
> > hooks in preparation for a pre-modify permission event.
> >
> > The assertion is not checked for an open permission event, because
> > do_open() takes mnt_want_write() in O_TRUNC case, meaning that it is not
> > safe to write to filesystem in the content of an open permission event.
>                                      ^^^^^ context
>
> BTW, isn't this a bit inconvenient? I mean filling file contents on open
> looks quite natural... Do you plan to fill files only on individual read /
> write events? I was under the impression simple HSM handlers would be doing
> it on open.
>

Naive HSMs perhaps... The problem with filling on open is that the pattern
open();fstat();close() is quite common with applications and we found open()
to be a sub-optimal predicate for near future read().

Filling the file on first read() access or directory on first
readdir() access does
a better job in "swapping in" the correct files.
A simple HSM would just fill the entire file/dir on the first PRE_ACCESS event.
that's not any more or less simple than filling it on an OPEN_PERM event.

Another point that could get lost when reading to above deadlock is that
filling the file content before open(O_TRUNC) would be really dumb,
because swap in is costly and you are going to throw away the data.
If we really wanted to provide HSM with a safe way to fill files on open,
we would probably need to report the open flags with the open event.
I actually think that reporting the open flags would be nice even with
an async open event.

Thanks,
Amir.