From: Victor Hsieh <victorhsieh@google.com>
To: Eric Biggers <ebiggers@kernel.org>
Cc: fsverity@lists.linux.dev, linux-fsdevel@vger.kernel.org,
stable@vger.kernel.org
Subject: Re: [PATCH] fsverity: don't drop pagecache at end of FS_IOC_ENABLE_VERITY
Date: Wed, 15 Mar 2023 13:01:38 -0700 [thread overview]
Message-ID: <CAFCauYOtRO3i0=HuogKngkxO4_au7ftD7aB+M0A-kcsdMUXmfA@mail.gmail.com> (raw)
In-Reply-To: <20230314235332.50270-1-ebiggers@kernel.org>
Reviewed-by: Victor Hsieh <victorhsieh@google.com>
On Tue, Mar 14, 2023 at 4:55 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> From: Eric Biggers <ebiggers@google.com>
>
> The full pagecache drop at the end of FS_IOC_ENABLE_VERITY is causing
> performance problems and is hindering adoption of fsverity. It was
> intended to solve a race condition where unverified pages might be left
> in the pagecache. But actually it doesn't solve it fully.
>
> Since the incomplete solution for this race condition has too much
> performance impact for it to be worth it, let's remove it for now.
>
> Fixes: 3fda4c617e84 ("fs-verity: implement FS_IOC_ENABLE_VERITY ioctl")
> Cc: stable@vger.kernel.org
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> fs/verity/enable.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/fs/verity/enable.c b/fs/verity/enable.c
> index e13db6507b38b..7a0e3a84d370b 100644
> --- a/fs/verity/enable.c
> +++ b/fs/verity/enable.c
> @@ -8,7 +8,6 @@
> #include "fsverity_private.h"
>
> #include <linux/mount.h>
> -#include <linux/pagemap.h>
> #include <linux/sched/signal.h>
> #include <linux/uaccess.h>
>
> @@ -367,25 +366,27 @@ int fsverity_ioctl_enable(struct file *filp, const void __user *uarg)
> goto out_drop_write;
>
> err = enable_verity(filp, &arg);
> - if (err)
> - goto out_allow_write_access;
>
> /*
> - * Some pages of the file may have been evicted from pagecache after
> - * being used in the Merkle tree construction, then read into pagecache
> - * again by another process reading from the file concurrently. Since
> - * these pages didn't undergo verification against the file digest which
> - * fs-verity now claims to be enforcing, we have to wipe the pagecache
> - * to ensure that all future reads are verified.
> + * We no longer drop the inode's pagecache after enabling verity. This
> + * used to be done to try to avoid a race condition where pages could be
> + * evicted after being used in the Merkle tree construction, then
> + * re-instantiated by a concurrent read. Such pages are unverified, and
> + * the backing storage could have filled them with different content, so
> + * they shouldn't be used to fulfill reads once verity is enabled.
> + *
> + * But, dropping the pagecache has a big performance impact, and it
> + * doesn't fully solve the race condition anyway. So for those reasons,
> + * and also because this race condition isn't very important relatively
> + * speaking (especially for small-ish files, where the chance of a page
> + * being used, evicted, *and* re-instantiated all while enabling verity
> + * is quite small), we no longer drop the inode's pagecache.
> */
> - filemap_write_and_wait(inode->i_mapping);
> - invalidate_inode_pages2(inode->i_mapping);
>
> /*
> * allow_write_access() is needed to pair with deny_write_access().
> * Regardless, the filesystem won't allow writing to verity files.
> */
> -out_allow_write_access:
> allow_write_access(filp);
> out_drop_write:
> mnt_drop_write_file(filp);
>
> base-commit: f959325e6ac3f499450088b8d9c626d1177be160
> --
> 2.39.2
>
prev parent reply other threads:[~2023-03-15 20:01 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-14 23:53 [PATCH] fsverity: don't drop pagecache at end of FS_IOC_ENABLE_VERITY Eric Biggers
2023-03-15 20:01 ` Victor Hsieh [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAFCauYOtRO3i0=HuogKngkxO4_au7ftD7aB+M0A-kcsdMUXmfA@mail.gmail.com' \
--to=victorhsieh@google.com \
--cc=ebiggers@kernel.org \
--cc=fsverity@lists.linux.dev \
--cc=linux-fsdevel@vger.kernel.org \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).