Message ID | e7785ffe237e581a7ba7e45d2724fca4d8fa1470.1687988380.git.sweettea-kernel@dorminy.me (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | btrfs: add encryption feature | expand |
On Wed, Jun 28, 2023 at 08:35:24PM -0400, Sweet Tea Dorminy wrote: > From: Omar Sandoval <osandov@osandov.com> > > Initially, only normal data extents, using the normal (non-direct) IO > path, will be encrypted. This change forbids various other bits: > - allows reflinking only if both inodes have the same encryption status > - disables direct IO on encrypted inodes > - disable inline data on encrypted inodes > > Signed-off-by: Omar Sandoval <osandov@osandov.com> > Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me> > --- > fs/btrfs/file.c | 4 ++-- > fs/btrfs/inode.c | 3 ++- > fs/btrfs/reflink.c | 7 +++++++ > 3 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c > index 392bc7d512a0..354962a7dd72 100644 > --- a/fs/btrfs/file.c > +++ b/fs/btrfs/file.c > @@ -1502,7 +1502,7 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) > goto relock; > } > > - if (check_direct_IO(fs_info, from, pos)) { > + if (IS_ENCRYPTED(inode) || check_direct_IO(fs_info, from, pos)) { > btrfs_inode_unlock(BTRFS_I(inode), ilock_flags); > goto buffered; > } > @@ -3741,7 +3741,7 @@ static ssize_t btrfs_direct_read(struct kiocb *iocb, struct iov_iter *to) > ssize_t read = 0; > ssize_t ret; > > - if (fsverity_active(inode)) > + if (IS_ENCRYPTED(inode) || fsverity_active(inode)) What's different about fscrypt vs fsverity that makes the inode flag a good check for encryption while verity relies on the presence of the extra context metadata? Is the enable model not subject to the same race where S_VERITY gets set ahead of actually storing the verity info/item? I think it's fine for these "skip" cases, but I imagine if we have cases of "I want a fully ready encrypted file" the verity-style check could be better? > return 0; > > if (check_direct_read(btrfs_sb(inode->i_sb), to, iocb->ki_pos)) > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index dbbb67293e34..48eadc4f187f 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -630,7 +630,8 @@ static noinline int cow_file_range_inline(struct btrfs_inode *inode, u64 size, > * compressed) data fits in a leaf and the configured maximum inline > * size. > */ > - if (size < i_size_read(&inode->vfs_inode) || > + if (IS_ENCRYPTED(&inode->vfs_inode) || > + size < i_size_read(&inode->vfs_inode) || > size > fs_info->sectorsize || > data_len > BTRFS_MAX_INLINE_DATA_SIZE(fs_info) || > data_len > fs_info->max_inline) > diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c > index 0474bbe39da7..ad722f495c9b 100644 > --- a/fs/btrfs/reflink.c > +++ b/fs/btrfs/reflink.c > @@ -1,6 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0 > > #include <linux/blkdev.h> > +#include <linux/fscrypt.h> > #include <linux/iversion.h> > #include "ctree.h" > #include "fs.h" > @@ -811,6 +812,12 @@ static int btrfs_remap_file_range_prep(struct file *file_in, loff_t pos_in, > ASSERT(inode_in->i_sb == inode_out->i_sb); > } > > + /* > + * Can only reflink encrypted files if both files are encrypted. > + */ > + if (IS_ENCRYPTED(inode_in) != IS_ENCRYPTED(inode_out)) > + return -EINVAL; > + > /* Don't make the dst file partly checksummed */ > if ((BTRFS_I(inode_in)->flags & BTRFS_INODE_NODATASUM) != > (BTRFS_I(inode_out)->flags & BTRFS_INODE_NODATASUM)) { > -- > 2.40.1 >
On 7/7/23 19:36, Boris Burkov wrote: > On Wed, Jun 28, 2023 at 08:35:24PM -0400, Sweet Tea Dorminy wrote: >> From: Omar Sandoval <osandov@osandov.com> >> >> Initially, only normal data extents, using the normal (non-direct) IO >> path, will be encrypted. This change forbids various other bits: >> - allows reflinking only if both inodes have the same encryption status >> - disables direct IO on encrypted inodes >> - disable inline data on encrypted inodes >> >> Signed-off-by: Omar Sandoval <osandov@osandov.com> >> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me> >> --- >> fs/btrfs/file.c | 4 ++-- >> fs/btrfs/inode.c | 3 ++- >> fs/btrfs/reflink.c | 7 +++++++ >> 3 files changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c >> index 392bc7d512a0..354962a7dd72 100644 >> --- a/fs/btrfs/file.c >> +++ b/fs/btrfs/file.c >> @@ -1502,7 +1502,7 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) >> goto relock; >> } >> >> - if (check_direct_IO(fs_info, from, pos)) { >> + if (IS_ENCRYPTED(inode) || check_direct_IO(fs_info, from, pos)) { >> btrfs_inode_unlock(BTRFS_I(inode), ilock_flags); >> goto buffered; >> } >> @@ -3741,7 +3741,7 @@ static ssize_t btrfs_direct_read(struct kiocb *iocb, struct iov_iter *to) >> ssize_t read = 0; >> ssize_t ret; >> >> - if (fsverity_active(inode)) >> + if (IS_ENCRYPTED(inode) || fsverity_active(inode)) > > What's different about fscrypt vs fsverity that makes the inode flag a > good check for encryption while verity relies on the presence of the > extra context metadata? > > Is the enable model not subject to the same race where S_VERITY gets set > ahead of actually storing the verity info/item? > > I think it's fine for these "skip" cases, but I imagine if we have cases > of "I want a fully ready encrypted file" the verity-style check could be > better? > Clear solution: enable direct encrypted IO :). I've removed this check altogether, thanks for the note.
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c index 392bc7d512a0..354962a7dd72 100644 --- a/fs/btrfs/file.c +++ b/fs/btrfs/file.c @@ -1502,7 +1502,7 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) goto relock; } - if (check_direct_IO(fs_info, from, pos)) { + if (IS_ENCRYPTED(inode) || check_direct_IO(fs_info, from, pos)) { btrfs_inode_unlock(BTRFS_I(inode), ilock_flags); goto buffered; } @@ -3741,7 +3741,7 @@ static ssize_t btrfs_direct_read(struct kiocb *iocb, struct iov_iter *to) ssize_t read = 0; ssize_t ret; - if (fsverity_active(inode)) + if (IS_ENCRYPTED(inode) || fsverity_active(inode)) return 0; if (check_direct_read(btrfs_sb(inode->i_sb), to, iocb->ki_pos)) diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index dbbb67293e34..48eadc4f187f 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -630,7 +630,8 @@ static noinline int cow_file_range_inline(struct btrfs_inode *inode, u64 size, * compressed) data fits in a leaf and the configured maximum inline * size. */ - if (size < i_size_read(&inode->vfs_inode) || + if (IS_ENCRYPTED(&inode->vfs_inode) || + size < i_size_read(&inode->vfs_inode) || size > fs_info->sectorsize || data_len > BTRFS_MAX_INLINE_DATA_SIZE(fs_info) || data_len > fs_info->max_inline) diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c index 0474bbe39da7..ad722f495c9b 100644 --- a/fs/btrfs/reflink.c +++ b/fs/btrfs/reflink.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0 #include <linux/blkdev.h> +#include <linux/fscrypt.h> #include <linux/iversion.h> #include "ctree.h" #include "fs.h" @@ -811,6 +812,12 @@ static int btrfs_remap_file_range_prep(struct file *file_in, loff_t pos_in, ASSERT(inode_in->i_sb == inode_out->i_sb); } + /* + * Can only reflink encrypted files if both files are encrypted. + */ + if (IS_ENCRYPTED(inode_in) != IS_ENCRYPTED(inode_out)) + return -EINVAL; + /* Don't make the dst file partly checksummed */ if ((BTRFS_I(inode_in)->flags & BTRFS_INODE_NODATASUM) != (BTRFS_I(inode_out)->flags & BTRFS_INODE_NODATASUM)) {