| Message ID | 20230905003227.326998-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | quota: explicitly forbid quota files from being encrypted | expand |
On Mon 04-09-23 17:32:27, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > Since commit d7e7b9af104c ("fscrypt: stop using keyrings subsystem for > fscrypt_master_key"), xfstest generic/270 causes a WARNING when run on > f2fs with test_dummy_encryption in the mount options: > > $ kvm-xfstests -c f2fs/encrypt generic/270 > [...] > WARNING: CPU: 1 PID: 2453 at fs/crypto/keyring.c:240 fscrypt_destroy_keyring+0x1f5/0x260 > > The cause of the WARNING is that not all encrypted inodes have been > evicted before fscrypt_destroy_keyring() is called, which violates an > assumption. This happens because the test uses an external quota file, > which gets automatically encrypted due to test_dummy_encryption. > > Encryption of quota files has never really been supported. On ext4, > ext4_quota_read() does not decrypt the data, so encrypted quota files > are always considered invalid on ext4. On f2fs, f2fs_quota_read() uses > the pagecache, so trying to use an encrypted quota file gets farther, > resulting in the issue described above being possible. But this was > never intended to be possible, and there is no use case for it. > > Therefore, make the quota support layer explicitly reject using > IS_ENCRYPTED inodes when quotaon is attempted. > > Cc: stable@vger.kernel.org > Signed-off-by: Eric Biggers <ebiggers@google.com> Looks good. I'll queue this patch into my tree and send it to Linus for RC2. Honza > --- > fs/quota/dquot.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c > index 9e72bfe8bbad9..7e268cd2727cc 100644 > --- a/fs/quota/dquot.c > +++ b/fs/quota/dquot.c > @@ -2339,6 +2339,20 @@ static int vfs_setup_quota_inode(struct inode *inode, int type) > if (sb_has_quota_loaded(sb, type)) > return -EBUSY; > > + /* > + * Quota files should never be encrypted. They should be thought of as > + * filesystem metadata, not user data. New-style internal quota files > + * cannot be encrypted by users anyway, but old-style external quota > + * files could potentially be incorrectly created in an encrypted > + * directory, hence this explicit check. Some reasons why encrypted > + * quota files don't work include: (1) some filesystems that support > + * encryption don't handle it in their quota_read and quota_write, and > + * (2) cleaning up encrypted quota files at unmount would need special > + * consideration, as quota files are cleaned up later than user files. > + */ > + if (IS_ENCRYPTED(inode)) > + return -EINVAL; > + > dqopt->files[type] = igrab(inode); > if (!dqopt->files[type]) > return -EIO; > > base-commit: 708283abf896dd4853e673cc8cba70acaf9bf4ea > -- > 2.42.0 >
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c index 9e72bfe8bbad9..7e268cd2727cc 100644 --- a/fs/quota/dquot.c +++ b/fs/quota/dquot.c @@ -2339,6 +2339,20 @@ static int vfs_setup_quota_inode(struct inode *inode, int type) if (sb_has_quota_loaded(sb, type)) return -EBUSY; + /* + * Quota files should never be encrypted. They should be thought of as + * filesystem metadata, not user data. New-style internal quota files + * cannot be encrypted by users anyway, but old-style external quota + * files could potentially be incorrectly created in an encrypted + * directory, hence this explicit check. Some reasons why encrypted + * quota files don't work include: (1) some filesystems that support + * encryption don't handle it in their quota_read and quota_write, and + * (2) cleaning up encrypted quota files at unmount would need special + * consideration, as quota files are cleaned up later than user files. + */ + if (IS_ENCRYPTED(inode)) + return -EINVAL; + dqopt->files[type] = igrab(inode); if (!dqopt->files[type]) return -EIO;