Re: [f2fs-dev] [PATCH V2 10/13] fscrypt_encrypt_page: Loop across all blocks mapped by a page range
Hi Chandan,
On Thu, May 02, 2019 at 11:22:05AM +0530, Chandan Rajendra wrote:
> On Thursday, May 2, 2019 3:59:01 AM IST Eric Biggers wrote:
> > Hi Chandan,
> >
> > On Wed, May 01, 2019 at 08:19:35PM +0530, Chandan Rajendra wrote:
> > > On Wednesday, May 1, 2019 4:38:41 AM IST Eric Biggers wrote:
> > > > On Tue, Apr 30, 2019 at 10:11:35AM -0700, Eric Biggers wrote:
> > > > > On Sun, Apr 28, 2019 at 10:01:18AM +0530, Chandan Rajendra wrote:
> > > > > > For subpage-sized blocks, this commit now encrypts all blocks
> > > > > > mapped by
> > > > > > a page range.
> > > > > >
> > > > > > Signed-off-by: Chandan Rajendra
> > > > > > ---
> > > > > > fs/crypto/crypto.c | 37 +
> > > > > > 1 file changed, 25 insertions(+), 12 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> > > > > > index 4f0d832cae71..2d65b431563f 100644
> > > > > > --- a/fs/crypto/crypto.c
> > > > > > +++ b/fs/crypto/crypto.c
> > > > > > @@ -242,18 +242,26 @@ struct page *fscrypt_encrypt_page(const
> > > > > > struct inode *inode,
> > > > >
> > > > > Need to update the function comment to clearly explain what this
> > > > > function
> > > > > actually does now.
> > > > >
> > > > > > {
> > > > > > struct fscrypt_ctx *ctx;
> > > > > > struct page *ciphertext_page = page;
> > > > > > + int i, page_nr_blks;
> > > > > > int err;
> > > > > >
> > > > > > BUG_ON(len % FS_CRYPTO_BLOCK_SIZE != 0);
> > > > > >
> > > > >
> > > > > Make a 'blocksize' variable so you don't have to keep calling
> > > > > i_blocksize().
> > > > >
> > > > > Also, you need to check whether 'len' and 'offs' are
> > > > > filesystem-block-aligned,
> > > > > since the code now assumes it.
> > > > >
> > > > > const unsigned int blocksize = i_blocksize(inode);
> > > > >
> > > > > if (!IS_ALIGNED(len | offs, blocksize))
> > > > > return -EINVAL;
> > > > >
> > > > > However, did you check whether that's always true for ubifs? It
> > > > > looks like it
> > > > > may expect to encrypt a prefix of a block, that is only padded to the
> > > > > next
> > > > > 16-byte boundary.
> > > > >
> > > > > > + page_nr_blks = len >> inode->i_blkbits;
> > > > > > +
> > > > > > if (inode->i_sb->s_cop->flags & FS_CFLG_OWN_PAGES) {
> > > > > > /* with inplace-encryption we just encrypt the page */
> > > > > > - err = fscrypt_do_page_crypto(inode, FS_ENCRYPT,
> > > > > > lblk_num, page,
> > > > > > -ciphertext_page, len, offs,
> > > > > > -gfp_flags);
> > > > > > - if (err)
> > > > > > - return ERR_PTR(err);
> > > > > > -
> > > > > > + for (i = 0; i < page_nr_blks; i++) {
> > > > > > + err = fscrypt_do_page_crypto(inode, FS_ENCRYPT,
> > > > > > + lblk_num, page,
> > > > > > + ciphertext_page,
> > > > > > + i_blocksize(inode),
> > > > > > offs,
> > > > > > + gfp_flags);
> > > > > > + if (err)
> > > > > > + return ERR_PTR(err);
> > > >
> > > > Apparently ubifs does encrypt data shorter than the filesystem block
> > > > size, so
> > > > this part is wrong.
> > > >
> > > > I suggest we split this into two functions,
> > > > fscrypt_encrypt_block_inplace() and
> > > > fscrypt_encrypt_blocks(), so that it's conceptually simpler what each
> > > > function
> > > > does. Currently this works completely differently depending on whether
> > > > the
> > > > filesystem set FS_CFLG_OWN_PAGES in its fscrypt_operations, which is
> > > > weird.
> > > >
> > > > I also noticed that using fscrypt_ctx for writes seems to be
> > > > unnecessary.
> > > > AFAICS, page_private(bounce_page) could point directly to the pagecache
> > > > page.
> > > > That would simplify things a lot, especially since then fscrypt_ctx
> > > > could be
> > > > removed entirely after you convert reads to use read_callbacks_ctx.
> > > >
> > > > IMO, these would be worthwhile cleanups for fscrypt by themselves,
> > > > without
> > > > waiting for the read_callbacks stuff to be finalized. Finalizing the
> > > > read_callbacks stuff will probably require reaching a consensus about
> > > > how they
> > > > should work with future filesystem features like fsverity and
> > > > compression.
> > > >
> > > > So to move things forward, I'm considering sending out a series with
> > > > the above
> > > > cleanups for fscrypt, plus the equivalent of your patches:
> > > >
> > > > "fscrypt_encrypt_page: Loop across all blocks mapped by a page
> > > > range"
> > > > "fscrypt_zeroout_range: Encrypt all zeroed out blocks of a page"
> > > > "Add decryption support for sub-pagesized blocks"
Re: [f2fs-dev] [PATCH V2 10/13] fscrypt_encrypt_page: Loop across all blocks mapped by a page range
On Thursday, May 2, 2019 3:59:01 AM IST Eric Biggers wrote:
> Hi Chandan,
>
> On Wed, May 01, 2019 at 08:19:35PM +0530, Chandan Rajendra wrote:
> > On Wednesday, May 1, 2019 4:38:41 AM IST Eric Biggers wrote:
> > > On Tue, Apr 30, 2019 at 10:11:35AM -0700, Eric Biggers wrote:
> > > > On Sun, Apr 28, 2019 at 10:01:18AM +0530, Chandan Rajendra wrote:
> > > > > For subpage-sized blocks, this commit now encrypts all blocks mapped
> > > > > by
> > > > > a page range.
> > > > >
> > > > > Signed-off-by: Chandan Rajendra
> > > > > ---
> > > > > fs/crypto/crypto.c | 37 +
> > > > > 1 file changed, 25 insertions(+), 12 deletions(-)
> > > > >
> > > > > diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> > > > > index 4f0d832cae71..2d65b431563f 100644
> > > > > --- a/fs/crypto/crypto.c
> > > > > +++ b/fs/crypto/crypto.c
> > > > > @@ -242,18 +242,26 @@ struct page *fscrypt_encrypt_page(const struct
> > > > > inode *inode,
> > > >
> > > > Need to update the function comment to clearly explain what this
> > > > function
> > > > actually does now.
> > > >
> > > > > {
> > > > > struct fscrypt_ctx *ctx;
> > > > > struct page *ciphertext_page = page;
> > > > > + int i, page_nr_blks;
> > > > > int err;
> > > > >
> > > > > BUG_ON(len % FS_CRYPTO_BLOCK_SIZE != 0);
> > > > >
> > > >
> > > > Make a 'blocksize' variable so you don't have to keep calling
> > > > i_blocksize().
> > > >
> > > > Also, you need to check whether 'len' and 'offs' are
> > > > filesystem-block-aligned,
> > > > since the code now assumes it.
> > > >
> > > > const unsigned int blocksize = i_blocksize(inode);
> > > >
> > > > if (!IS_ALIGNED(len | offs, blocksize))
> > > > return -EINVAL;
> > > >
> > > > However, did you check whether that's always true for ubifs? It looks
> > > > like it
> > > > may expect to encrypt a prefix of a block, that is only padded to the
> > > > next
> > > > 16-byte boundary.
> > > >
> > > > > + page_nr_blks = len >> inode->i_blkbits;
> > > > > +
> > > > > if (inode->i_sb->s_cop->flags & FS_CFLG_OWN_PAGES) {
> > > > > /* with inplace-encryption we just encrypt the page */
> > > > > - err = fscrypt_do_page_crypto(inode, FS_ENCRYPT,
> > > > > lblk_num, page,
> > > > > - ciphertext_page, len, offs,
> > > > > - gfp_flags);
> > > > > - if (err)
> > > > > - return ERR_PTR(err);
> > > > > -
> > > > > + for (i = 0; i < page_nr_blks; i++) {
> > > > > + err = fscrypt_do_page_crypto(inode, FS_ENCRYPT,
> > > > > + lblk_num, page,
> > > > > + ciphertext_page,
> > > > > + i_blocksize(inode),
> > > > > offs,
> > > > > + gfp_flags);
> > > > > + if (err)
> > > > > + return ERR_PTR(err);
> > >
> > > Apparently ubifs does encrypt data shorter than the filesystem block
> > > size, so
> > > this part is wrong.
> > >
> > > I suggest we split this into two functions,
> > > fscrypt_encrypt_block_inplace() and
> > > fscrypt_encrypt_blocks(), so that it's conceptually simpler what each
> > > function
> > > does. Currently this works completely differently depending on whether
> > > the
> > > filesystem set FS_CFLG_OWN_PAGES in its fscrypt_operations, which is
> > > weird.
> > >
> > > I also noticed that using fscrypt_ctx for writes seems to be unnecessary.
> > > AFAICS, page_private(bounce_page) could point directly to the pagecache
> > > page.
> > > That would simplify things a lot, especially since then fscrypt_ctx could
> > > be
> > > removed entirely after you convert reads to use read_callbacks_ctx.
> > >
> > > IMO, these would be worthwhile cleanups for fscrypt by themselves, without
> > > waiting for the read_callbacks stuff to be finalized. Finalizing the
> > > read_callbacks stuff will probably require reaching a consensus about how
> > > they
> > > should work with future filesystem features like fsverity and compression.
> > >
> > > So to move things forward, I'm considering sending out a series with the
> > > above
> > > cleanups for fscrypt, plus the equivalent of your patches:
> > >
> > > "fscrypt_encrypt_page: Loop across all blocks mapped by a page range"
> > > "fscrypt_zeroout_range: Encrypt all zeroed out blocks of a page"
> > > "Add decryption support for sub-pagesized blocks" (fs/crypto/ part only)
> > >
> > > Then hopefully we can get all that applied for 5.3 so that fs/crypto/
> > > itself is
> > > ready for blocksize != PAGE_SIZE; and get your changes to
> > > ext4_bio_write_page(),
> > > __ext4_block_zero_page_range(), and ext4_block_write_begin() applied too,
> > > so
Re: [f2fs-dev] [PATCH V2 10/13] fscrypt_encrypt_page: Loop across all blocks mapped by a page range
Hi Chandan,
On Wed, May 01, 2019 at 08:19:35PM +0530, Chandan Rajendra wrote:
> On Wednesday, May 1, 2019 4:38:41 AM IST Eric Biggers wrote:
> > On Tue, Apr 30, 2019 at 10:11:35AM -0700, Eric Biggers wrote:
> > > On Sun, Apr 28, 2019 at 10:01:18AM +0530, Chandan Rajendra wrote:
> > > > For subpage-sized blocks, this commit now encrypts all blocks mapped by
> > > > a page range.
> > > >
> > > > Signed-off-by: Chandan Rajendra
> > > > ---
> > > > fs/crypto/crypto.c | 37 +
> > > > 1 file changed, 25 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> > > > index 4f0d832cae71..2d65b431563f 100644
> > > > --- a/fs/crypto/crypto.c
> > > > +++ b/fs/crypto/crypto.c
> > > > @@ -242,18 +242,26 @@ struct page *fscrypt_encrypt_page(const struct
> > > > inode *inode,
> > >
> > > Need to update the function comment to clearly explain what this function
> > > actually does now.
> > >
> > > > {
> > > > struct fscrypt_ctx *ctx;
> > > > struct page *ciphertext_page = page;
> > > > + int i, page_nr_blks;
> > > > int err;
> > > >
> > > > BUG_ON(len % FS_CRYPTO_BLOCK_SIZE != 0);
> > > >
> > >
> > > Make a 'blocksize' variable so you don't have to keep calling
> > > i_blocksize().
> > >
> > > Also, you need to check whether 'len' and 'offs' are
> > > filesystem-block-aligned,
> > > since the code now assumes it.
> > >
> > > const unsigned int blocksize = i_blocksize(inode);
> > >
> > > if (!IS_ALIGNED(len | offs, blocksize))
> > > return -EINVAL;
> > >
> > > However, did you check whether that's always true for ubifs? It looks
> > > like it
> > > may expect to encrypt a prefix of a block, that is only padded to the next
> > > 16-byte boundary.
> > >
> > > > + page_nr_blks = len >> inode->i_blkbits;
> > > > +
> > > > if (inode->i_sb->s_cop->flags & FS_CFLG_OWN_PAGES) {
> > > > /* with inplace-encryption we just encrypt the page */
> > > > - err = fscrypt_do_page_crypto(inode, FS_ENCRYPT,
> > > > lblk_num, page,
> > > > -ciphertext_page, len, offs,
> > > > -gfp_flags);
> > > > - if (err)
> > > > - return ERR_PTR(err);
> > > > -
> > > > + for (i = 0; i < page_nr_blks; i++) {
> > > > + err = fscrypt_do_page_crypto(inode, FS_ENCRYPT,
> > > > + lblk_num, page,
> > > > + ciphertext_page,
> > > > + i_blocksize(inode),
> > > > offs,
> > > > + gfp_flags);
> > > > + if (err)
> > > > + return ERR_PTR(err);
> >
> > Apparently ubifs does encrypt data shorter than the filesystem block size,
> > so
> > this part is wrong.
> >
> > I suggest we split this into two functions, fscrypt_encrypt_block_inplace()
> > and
> > fscrypt_encrypt_blocks(), so that it's conceptually simpler what each
> > function
> > does. Currently this works completely differently depending on whether the
> > filesystem set FS_CFLG_OWN_PAGES in its fscrypt_operations, which is weird.
> >
> > I also noticed that using fscrypt_ctx for writes seems to be unnecessary.
> > AFAICS, page_private(bounce_page) could point directly to the pagecache
> > page.
> > That would simplify things a lot, especially since then fscrypt_ctx could be
> > removed entirely after you convert reads to use read_callbacks_ctx.
> >
> > IMO, these would be worthwhile cleanups for fscrypt by themselves, without
> > waiting for the read_callbacks stuff to be finalized. Finalizing the
> > read_callbacks stuff will probably require reaching a consensus about how
> > they
> > should work with future filesystem features like fsverity and compression.
> >
> > So to move things forward, I'm considering sending out a series with the
> > above
> > cleanups for fscrypt, plus the equivalent of your patches:
> >
> > "fscrypt_encrypt_page: Loop across all blocks mapped by a page range"
> > "fscrypt_zeroout_range: Encrypt all zeroed out blocks of a page"
> > "Add decryption support for sub-pagesized blocks" (fs/crypto/ part only)
> >
> > Then hopefully we can get all that applied for 5.3 so that fs/crypto/
> > itself is
> > ready for blocksize != PAGE_SIZE; and get your changes to
> > ext4_bio_write_page(),
> > __ext4_block_zero_page_range(), and ext4_block_write_begin() applied too, so
> > that ext4 is partially ready for encryption with blocksize != PAGE_SIZE.
> >
> > Then only the read_callbacks stuff will remain, to get encryption support
> > into
> > fs/mpage.c and fs/buffer.c. Do you think that's a good plan?
>
> Hi Eric,
>
> IMHO, I will continue posting the next
Re: [f2fs-dev] [PATCH V2 10/13] fscrypt_encrypt_page: Loop across all blocks mapped by a page range
On Wednesday, May 1, 2019 4:38:41 AM IST Eric Biggers wrote:
> On Tue, Apr 30, 2019 at 10:11:35AM -0700, Eric Biggers wrote:
> > On Sun, Apr 28, 2019 at 10:01:18AM +0530, Chandan Rajendra wrote:
> > > For subpage-sized blocks, this commit now encrypts all blocks mapped by
> > > a page range.
> > >
> > > Signed-off-by: Chandan Rajendra
> > > ---
> > > fs/crypto/crypto.c | 37 +
> > > 1 file changed, 25 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> > > index 4f0d832cae71..2d65b431563f 100644
> > > --- a/fs/crypto/crypto.c
> > > +++ b/fs/crypto/crypto.c
> > > @@ -242,18 +242,26 @@ struct page *fscrypt_encrypt_page(const struct
> > > inode *inode,
> >
> > Need to update the function comment to clearly explain what this function
> > actually does now.
> >
> > > {
> > > struct fscrypt_ctx *ctx;
> > > struct page *ciphertext_page = page;
> > > + int i, page_nr_blks;
> > > int err;
> > >
> > > BUG_ON(len % FS_CRYPTO_BLOCK_SIZE != 0);
> > >
> >
> > Make a 'blocksize' variable so you don't have to keep calling i_blocksize().
> >
> > Also, you need to check whether 'len' and 'offs' are
> > filesystem-block-aligned,
> > since the code now assumes it.
> >
> > const unsigned int blocksize = i_blocksize(inode);
> >
> > if (!IS_ALIGNED(len | offs, blocksize))
> > return -EINVAL;
> >
> > However, did you check whether that's always true for ubifs? It looks like
> > it
> > may expect to encrypt a prefix of a block, that is only padded to the next
> > 16-byte boundary.
> >
> > > + page_nr_blks = len >> inode->i_blkbits;
> > > +
> > > if (inode->i_sb->s_cop->flags & FS_CFLG_OWN_PAGES) {
> > > /* with inplace-encryption we just encrypt the page */
> > > - err = fscrypt_do_page_crypto(inode, FS_ENCRYPT, lblk_num, page,
> > > - ciphertext_page, len, offs,
> > > - gfp_flags);
> > > - if (err)
> > > - return ERR_PTR(err);
> > > -
> > > + for (i = 0; i < page_nr_blks; i++) {
> > > + err = fscrypt_do_page_crypto(inode, FS_ENCRYPT,
> > > + lblk_num, page,
> > > + ciphertext_page,
> > > + i_blocksize(inode), offs,
> > > + gfp_flags);
> > > + if (err)
> > > + return ERR_PTR(err);
>
> Apparently ubifs does encrypt data shorter than the filesystem block size, so
> this part is wrong.
>
> I suggest we split this into two functions, fscrypt_encrypt_block_inplace()
> and
> fscrypt_encrypt_blocks(), so that it's conceptually simpler what each function
> does. Currently this works completely differently depending on whether the
> filesystem set FS_CFLG_OWN_PAGES in its fscrypt_operations, which is weird.
>
> I also noticed that using fscrypt_ctx for writes seems to be unnecessary.
> AFAICS, page_private(bounce_page) could point directly to the pagecache page.
> That would simplify things a lot, especially since then fscrypt_ctx could be
> removed entirely after you convert reads to use read_callbacks_ctx.
>
> IMO, these would be worthwhile cleanups for fscrypt by themselves, without
> waiting for the read_callbacks stuff to be finalized. Finalizing the
> read_callbacks stuff will probably require reaching a consensus about how they
> should work with future filesystem features like fsverity and compression.
>
> So to move things forward, I'm considering sending out a series with the above
> cleanups for fscrypt, plus the equivalent of your patches:
>
> "fscrypt_encrypt_page: Loop across all blocks mapped by a page range"
> "fscrypt_zeroout_range: Encrypt all zeroed out blocks of a page"
> "Add decryption support for sub-pagesized blocks" (fs/crypto/ part only)
>
> Then hopefully we can get all that applied for 5.3 so that fs/crypto/ itself
> is
> ready for blocksize != PAGE_SIZE; and get your changes to
> ext4_bio_write_page(),
> __ext4_block_zero_page_range(), and ext4_block_write_begin() applied too, so
> that ext4 is partially ready for encryption with blocksize != PAGE_SIZE.
>
> Then only the read_callbacks stuff will remain, to get encryption support into
> fs/mpage.c and fs/buffer.c. Do you think that's a good plan?
Hi Eric,
IMHO, I will continue posting the next version of the current patchset and if
there are no serious reservations from FS maintainers the "read callbacks"
patchset can be merged. In such a scenario, the cleanups being
non-complicated, can be merged later.
--
chandan
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH V2 10/13] fscrypt_encrypt_page: Loop across all blocks mapped by a page range
On Tue, Apr 30, 2019 at 10:11:35AM -0700, Eric Biggers wrote:
> On Sun, Apr 28, 2019 at 10:01:18AM +0530, Chandan Rajendra wrote:
> > For subpage-sized blocks, this commit now encrypts all blocks mapped by
> > a page range.
> >
> > Signed-off-by: Chandan Rajendra
> > ---
> > fs/crypto/crypto.c | 37 +
> > 1 file changed, 25 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> > index 4f0d832cae71..2d65b431563f 100644
> > --- a/fs/crypto/crypto.c
> > +++ b/fs/crypto/crypto.c
> > @@ -242,18 +242,26 @@ struct page *fscrypt_encrypt_page(const struct inode
> > *inode,
>
> Need to update the function comment to clearly explain what this function
> actually does now.
>
> > {
> > struct fscrypt_ctx *ctx;
> > struct page *ciphertext_page = page;
> > + int i, page_nr_blks;
> > int err;
> >
> > BUG_ON(len % FS_CRYPTO_BLOCK_SIZE != 0);
> >
>
> Make a 'blocksize' variable so you don't have to keep calling i_blocksize().
>
> Also, you need to check whether 'len' and 'offs' are filesystem-block-aligned,
> since the code now assumes it.
>
> const unsigned int blocksize = i_blocksize(inode);
>
> if (!IS_ALIGNED(len | offs, blocksize))
> return -EINVAL;
>
> However, did you check whether that's always true for ubifs? It looks like it
> may expect to encrypt a prefix of a block, that is only padded to the next
> 16-byte boundary.
>
> > + page_nr_blks = len >> inode->i_blkbits;
> > +
> > if (inode->i_sb->s_cop->flags & FS_CFLG_OWN_PAGES) {
> > /* with inplace-encryption we just encrypt the page */
> > - err = fscrypt_do_page_crypto(inode, FS_ENCRYPT, lblk_num, page,
> > -ciphertext_page, len, offs,
> > -gfp_flags);
> > - if (err)
> > - return ERR_PTR(err);
> > -
> > + for (i = 0; i < page_nr_blks; i++) {
> > + err = fscrypt_do_page_crypto(inode, FS_ENCRYPT,
> > + lblk_num, page,
> > + ciphertext_page,
> > + i_blocksize(inode), offs,
> > + gfp_flags);
> > + if (err)
> > + return ERR_PTR(err);
Apparently ubifs does encrypt data shorter than the filesystem block size, so
this part is wrong.
I suggest we split this into two functions, fscrypt_encrypt_block_inplace() and
fscrypt_encrypt_blocks(), so that it's conceptually simpler what each function
does. Currently this works completely differently depending on whether the
filesystem set FS_CFLG_OWN_PAGES in its fscrypt_operations, which is weird.
I also noticed that using fscrypt_ctx for writes seems to be unnecessary.
AFAICS, page_private(bounce_page) could point directly to the pagecache page.
That would simplify things a lot, especially since then fscrypt_ctx could be
removed entirely after you convert reads to use read_callbacks_ctx.
IMO, these would be worthwhile cleanups for fscrypt by themselves, without
waiting for the read_callbacks stuff to be finalized. Finalizing the
read_callbacks stuff will probably require reaching a consensus about how they
should work with future filesystem features like fsverity and compression.
So to move things forward, I'm considering sending out a series with the above
cleanups for fscrypt, plus the equivalent of your patches:
"fscrypt_encrypt_page: Loop across all blocks mapped by a page range"
"fscrypt_zeroout_range: Encrypt all zeroed out blocks of a page"
"Add decryption support for sub-pagesized blocks" (fs/crypto/ part only)
Then hopefully we can get all that applied for 5.3 so that fs/crypto/ itself is
ready for blocksize != PAGE_SIZE; and get your changes to ext4_bio_write_page(),
__ext4_block_zero_page_range(), and ext4_block_write_begin() applied too, so
that ext4 is partially ready for encryption with blocksize != PAGE_SIZE.
Then only the read_callbacks stuff will remain, to get encryption support into
fs/mpage.c and fs/buffer.c. Do you think that's a good plan?
Thanks!
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH V2 10/13] fscrypt_encrypt_page: Loop across all blocks mapped by a page range
On Sun, Apr 28, 2019 at 10:01:18AM +0530, Chandan Rajendra wrote:
> For subpage-sized blocks, this commit now encrypts all blocks mapped by
> a page range.
>
> Signed-off-by: Chandan Rajendra
> ---
> fs/crypto/crypto.c | 37 +
> 1 file changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> index 4f0d832cae71..2d65b431563f 100644
> --- a/fs/crypto/crypto.c
> +++ b/fs/crypto/crypto.c
> @@ -242,18 +242,26 @@ struct page *fscrypt_encrypt_page(const struct inode
> *inode,
Need to update the function comment to clearly explain what this function
actually does now.
> {
> struct fscrypt_ctx *ctx;
> struct page *ciphertext_page = page;
> + int i, page_nr_blks;
> int err;
>
> BUG_ON(len % FS_CRYPTO_BLOCK_SIZE != 0);
>
Make a 'blocksize' variable so you don't have to keep calling i_blocksize().
Also, you need to check whether 'len' and 'offs' are filesystem-block-aligned,
since the code now assumes it.
const unsigned int blocksize = i_blocksize(inode);
if (!IS_ALIGNED(len | offs, blocksize))
return -EINVAL;
However, did you check whether that's always true for ubifs? It looks like it
may expect to encrypt a prefix of a block, that is only padded to the next
16-byte boundary.
> + page_nr_blks = len >> inode->i_blkbits;
> +
> if (inode->i_sb->s_cop->flags & FS_CFLG_OWN_PAGES) {
> /* with inplace-encryption we just encrypt the page */
> - err = fscrypt_do_page_crypto(inode, FS_ENCRYPT, lblk_num, page,
> - ciphertext_page, len, offs,
> - gfp_flags);
> - if (err)
> - return ERR_PTR(err);
> -
> + for (i = 0; i < page_nr_blks; i++) {
> + err = fscrypt_do_page_crypto(inode, FS_ENCRYPT,
> + lblk_num, page,
> + ciphertext_page,
> + i_blocksize(inode), offs,
> + gfp_flags);
> + if (err)
> + return ERR_PTR(err);
> + ++lblk_num;
> + offs += i_blocksize(inode);
> + }
> return ciphertext_page;
> }
>
> @@ -269,12 +277,17 @@ struct page *fscrypt_encrypt_page(const struct inode
> *inode,
> goto errout;
>
> ctx->control_page = page;
> - err = fscrypt_do_page_crypto(inode, FS_ENCRYPT, lblk_num,
> - page, ciphertext_page, len, offs,
> - gfp_flags);
> - if (err) {
> - ciphertext_page = ERR_PTR(err);
> - goto errout;
> +
> + for (i = 0; i < page_nr_blks; i++) {
> + err = fscrypt_do_page_crypto(inode, FS_ENCRYPT, lblk_num,
> + page, ciphertext_page,
> + i_blocksize(inode), offs, gfp_flags);
As I mentioned elsewhere, renaming fscrypt_do_page_crypto() to
fscrypt_crypt_block() would make more sense now.
> + if (err) {
> + ciphertext_page = ERR_PTR(err);
> + goto errout;
> + }
> + ++lblk_num;
> + offs += i_blocksize(inode);
> }
> SetPagePrivate(ciphertext_page);
> set_page_private(ciphertext_page, (unsigned long)ctx);
> --
> 2.19.1
>
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH V2 10/13] fscrypt_encrypt_page: Loop across all blocks mapped by a page range
For subpage-sized blocks, this commit now encrypts all blocks mapped by
a page range.
Signed-off-by: Chandan Rajendra
---
fs/crypto/crypto.c | 37 +
1 file changed, 25 insertions(+), 12 deletions(-)
diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 4f0d832cae71..2d65b431563f 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -242,18 +242,26 @@ struct page *fscrypt_encrypt_page(const struct inode
*inode,
{
struct fscrypt_ctx *ctx;
struct page *ciphertext_page = page;
+ int i, page_nr_blks;
int err;
BUG_ON(len % FS_CRYPTO_BLOCK_SIZE != 0);
+ page_nr_blks = len >> inode->i_blkbits;
+
if (inode->i_sb->s_cop->flags & FS_CFLG_OWN_PAGES) {
/* with inplace-encryption we just encrypt the page */
- err = fscrypt_do_page_crypto(inode, FS_ENCRYPT, lblk_num, page,
-ciphertext_page, len, offs,
-gfp_flags);
- if (err)
- return ERR_PTR(err);
-
+ for (i = 0; i < page_nr_blks; i++) {
+ err = fscrypt_do_page_crypto(inode, FS_ENCRYPT,
+ lblk_num, page,
+ ciphertext_page,
+ i_blocksize(inode), offs,
+ gfp_flags);
+ if (err)
+ return ERR_PTR(err);
+ ++lblk_num;
+ offs += i_blocksize(inode);
+ }
return ciphertext_page;
}
@@ -269,12 +277,17 @@ struct page *fscrypt_encrypt_page(const struct inode
*inode,
goto errout;
ctx->control_page = page;
- err = fscrypt_do_page_crypto(inode, FS_ENCRYPT, lblk_num,
-page, ciphertext_page, len, offs,
-gfp_flags);
- if (err) {
- ciphertext_page = ERR_PTR(err);
- goto errout;
+
+ for (i = 0; i < page_nr_blks; i++) {
+ err = fscrypt_do_page_crypto(inode, FS_ENCRYPT, lblk_num,
+ page, ciphertext_page,
+ i_blocksize(inode), offs, gfp_flags);
+ if (err) {
+ ciphertext_page = ERR_PTR(err);
+ goto errout;
+ }
+ ++lblk_num;
+ offs += i_blocksize(inode);
}
SetPagePrivate(ciphertext_page);
set_page_private(ciphertext_page, (unsigned long)ctx);
--
2.19.1
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
