diff mbox series

[RFC,v2,05/18] fscrypt: don't balk when inode is already marked encrypted

Message ID 20200904160537.76663-6-jlayton@kernel.org
State Superseded
Headers show
Series ceph+fscrypt: context, filename and symlink support | expand

Commit Message

Jeff Layton Sept. 4, 2020, 4:05 p.m. UTC
Cephfs (currently) sets this flag early and only fetches the context
later. Eventually we may not need this, but for now it prevents this
warning from popping.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/crypto/keysetup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Eric Biggers Sept. 8, 2020, 3:52 a.m. UTC | #1
On Fri, Sep 04, 2020 at 12:05:24PM -0400, Jeff Layton wrote:
> Cephfs (currently) sets this flag early and only fetches the context
> later. Eventually we may not need this, but for now it prevents this
> warning from popping.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/crypto/keysetup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
> index ad64525ec680..3b4ec16fc528 100644
> --- a/fs/crypto/keysetup.c
> +++ b/fs/crypto/keysetup.c
> @@ -567,7 +567,7 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  		const union fscrypt_context *dummy_ctx =
>  			fscrypt_get_dummy_context(inode->i_sb);
>  
> -		if (IS_ENCRYPTED(inode) || !dummy_ctx) {
> +		if (!dummy_ctx) {
>  			fscrypt_warn(inode,
>  				     "Error %d getting encryption context",
>  				     res);

This makes errors reading the encryption xattr of an encrypted inode be ignored
when the filesystem is mounted with test_dummy_encryption.  That's undesirable.

Isn't this change actually no longer needed, now that new inodes will use
fscrypt_prepare_new_inode() instead of fscrypt_get_encryption_info()?

- Eric
Jeff Layton Sept. 8, 2020, 12:54 p.m. UTC | #2
On Mon, 2020-09-07 at 20:52 -0700, Eric Biggers wrote:
> On Fri, Sep 04, 2020 at 12:05:24PM -0400, Jeff Layton wrote:
> > Cephfs (currently) sets this flag early and only fetches the context
> > later. Eventually we may not need this, but for now it prevents this
> > warning from popping.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/crypto/keysetup.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
> > index ad64525ec680..3b4ec16fc528 100644
> > --- a/fs/crypto/keysetup.c
> > +++ b/fs/crypto/keysetup.c
> > @@ -567,7 +567,7 @@ int fscrypt_get_encryption_info(struct inode *inode)
> >  		const union fscrypt_context *dummy_ctx =
> >  			fscrypt_get_dummy_context(inode->i_sb);
> >  
> > -		if (IS_ENCRYPTED(inode) || !dummy_ctx) {
> > +		if (!dummy_ctx) {
> >  			fscrypt_warn(inode,
> >  				     "Error %d getting encryption context",
> >  				     res);
> 
> This makes errors reading the encryption xattr of an encrypted inode be ignored
> when the filesystem is mounted with test_dummy_encryption.  That's undesirable.
> 
> Isn't this change actually no longer needed, now that new inodes will use
> fscrypt_prepare_new_inode() instead of fscrypt_get_encryption_info()?

No. This is really for when we're reading in a new inode from the MDS.
We can tell that there is a context present in some of those cases, but
may not be able to read it yet. That said, it may be possible to pull in
the context at the point where we set S_ENCRYPTED. I'll take a look.

Thanks,
Eric Biggers Sept. 8, 2020, 11:08 p.m. UTC | #3
On Tue, Sep 08, 2020 at 08:54:35AM -0400, Jeff Layton wrote:
> On Mon, 2020-09-07 at 20:52 -0700, Eric Biggers wrote:
> > On Fri, Sep 04, 2020 at 12:05:24PM -0400, Jeff Layton wrote:
> > > Cephfs (currently) sets this flag early and only fetches the context
> > > later. Eventually we may not need this, but for now it prevents this
> > > warning from popping.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/crypto/keysetup.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
> > > index ad64525ec680..3b4ec16fc528 100644
> > > --- a/fs/crypto/keysetup.c
> > > +++ b/fs/crypto/keysetup.c
> > > @@ -567,7 +567,7 @@ int fscrypt_get_encryption_info(struct inode *inode)
> > >  		const union fscrypt_context *dummy_ctx =
> > >  			fscrypt_get_dummy_context(inode->i_sb);
> > >  
> > > -		if (IS_ENCRYPTED(inode) || !dummy_ctx) {
> > > +		if (!dummy_ctx) {
> > >  			fscrypt_warn(inode,
> > >  				     "Error %d getting encryption context",
> > >  				     res);
> > 
> > This makes errors reading the encryption xattr of an encrypted inode be ignored
> > when the filesystem is mounted with test_dummy_encryption.  That's undesirable.
> > 
> > Isn't this change actually no longer needed, now that new inodes will use
> > fscrypt_prepare_new_inode() instead of fscrypt_get_encryption_info()?
> 
> No. This is really for when we're reading in a new inode from the MDS.
> We can tell that there is a context present in some of those cases, but
> may not be able to read it yet. That said, it may be possible to pull in
> the context at the point where we set S_ENCRYPTED. I'll take a look.

fscrypt_prepare_new_inode() sets ->i_crypt_info on the new inode.
fscrypt_get_encryption_info() isn't needed after that.  If it does get called
anyway, it will be a no-op since it will see fscrypt_has_encryption_key(inode).

And when allocating an in-memory inode for an existing file,
fscrypt_get_encryption_info() shouldn't be called either.  It gets called later
by filesystem operations that need the encryption key, usually ->open().

- Eric
diff mbox series

Patch

diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index ad64525ec680..3b4ec16fc528 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -567,7 +567,7 @@  int fscrypt_get_encryption_info(struct inode *inode)
 		const union fscrypt_context *dummy_ctx =
 			fscrypt_get_dummy_context(inode->i_sb);
 
-		if (IS_ENCRYPTED(inode) || !dummy_ctx) {
+		if (!dummy_ctx) {
 			fscrypt_warn(inode,
 				     "Error %d getting encryption context",
 				     res);