| Message ID | 20230314235332.50270-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | fsverity: don't drop pagecache at end of FS_IOC_ENABLE_VERITY | expand |
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 >
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);