| Message ID | 20200904160537.76663-6-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ceph+fscrypt: context, filename and symlink support | expand |
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
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,
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 --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);
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(-)