| Message ID | 20200305084138.653498-1-ebiggers@kernel.org |
|---|---|
| State | Not Applicable |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | fscrypt: don't evict dirty inodes after removing key | expand |
On Thu, Mar 05, 2020 at 12:41:38AM -0800, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > After FS_IOC_REMOVE_ENCRYPTION_KEY removes a key, it syncs the > filesystem and tries to get and put all inodes that were unlocked by the > key so that unused inodes get evicted via fscrypt_drop_inode(). > Normally, the inodes are all clean due to the sync. > > However, after the filesystem is sync'ed, userspace can modify and close > one of the files. (Userspace is *supposed* to close the files before > removing the key. But it doesn't always happen, and the kernel can't > assume it.) This causes the inode to be dirtied and have i_count == 0. > Then, fscrypt_drop_inode() failed to consider this case and indicated > that the inode can be dropped, causing the write to be lost. > > On f2fs, other problems such as a filesystem freeze could occur due to > the inode being freed while still on f2fs's dirty inode list. > > Fix this bug by making fscrypt_drop_inode() only drop clean inodes. > > I've written an xfstest which detects this bug on ext4, f2fs, and ubifs. > > Fixes: b1c0ec3599f4 ("fscrypt: add FS_IOC_REMOVE_ENCRYPTION_KEY ioctl") > Cc: <stable@vger.kernel.org> # v5.4+ > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > fs/crypto/keysetup.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c > index 65cb09fa6ead..08c9f216a54d 100644 > --- a/fs/crypto/keysetup.c > +++ b/fs/crypto/keysetup.c > @@ -538,6 +538,15 @@ int fscrypt_drop_inode(struct inode *inode) > return 0; > mk = ci->ci_master_key->payload.data[0]; > > + /* > + * With proper, non-racy use of FS_IOC_REMOVE_ENCRYPTION_KEY, all inodes > + * protected by the key were cleaned by sync_filesystem(). But if > + * userspace is still using the files, inodes can be dirtied between > + * then and now. We mustn't lose any writes, so skip dirty inodes here. > + */ > + if (inode->i_state & I_DIRTY_ALL) > + return 0; > + > /* > * Note: since we aren't holding ->mk_secret_sem, the result here can > * immediately become outdated. But there's no correctness problem with > -- Applied to fscrypt.git#for-stable for 5.6. - Eric
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c index 65cb09fa6ead..08c9f216a54d 100644 --- a/fs/crypto/keysetup.c +++ b/fs/crypto/keysetup.c @@ -538,6 +538,15 @@ int fscrypt_drop_inode(struct inode *inode) return 0; mk = ci->ci_master_key->payload.data[0]; + /* + * With proper, non-racy use of FS_IOC_REMOVE_ENCRYPTION_KEY, all inodes + * protected by the key were cleaned by sync_filesystem(). But if + * userspace is still using the files, inodes can be dirtied between + * then and now. We mustn't lose any writes, so skip dirty inodes here. + */ + if (inode->i_state & I_DIRTY_ALL) + return 0; + /* * Note: since we aren't holding ->mk_secret_sem, the result here can * immediately become outdated. But there's no correctness problem with