Reviewed-by: Victor Hsieh <victorhsieh@xxxxxxxxxx>
On Tue, Mar 14, 2023 at 4:55 PM Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
>
> From: Eric Biggers <ebiggers@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx>
> ---
> 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
>
