Re: [PATCH] fsverity: don't drop pagecache at end of FS_IOC_ENABLE_VERITY

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
>




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux