ceph-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] ceph: fscrypt: fix atomic open bug for encrypted directories
@ 2023-03-14 10:39 Luís Henriques
  2023-03-14 10:39 ` [PATCH v2 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() Luís Henriques
  2023-03-14 10:39 ` [PATCH v2 2/2] ceph: switch atomic open to use new fscrypt helper Luís Henriques
  0 siblings, 2 replies; 4+ messages in thread
From: Luís Henriques @ 2023-03-14 10:39 UTC (permalink / raw)
  To: Eric Biggers, Xiubo Li, Jeff Layton
  Cc: Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt,
	ceph-devel, linux-kernel, Luís Henriques

Hi!

I started seeing fstest generic/123 failing in ceph fscrypt, when running it
with 'test_dummy_encryption'.  This test is quite simple:

1. Creates a directory with write permissions for root only
2. Writes into a file in that directory
3. Uses 'su' to try to modify that file as a different user, and
   gets -EPERM

All the test steps succeed, but the test fails to cleanup: 'rm -rf <dir>'
will fail with -ENOTEMPTY.  'strace' shows that calling unlinkat() to remove
the file got a -ENOENT and then -ENOTEMPTY for the directory.

This is because 'su' does a drop_caches ('su (874): drop_caches: 2' in
dmesg), and ceph's atomic open will do:

	if (IS_ENCRYPTED(dir)) {
		set_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags);
		if (!fscrypt_has_encryption_key(dir)) {
			spin_lock(&dentry->d_lock);
			dentry->d_flags |= DCACHE_NOKEY_NAME;
			spin_unlock(&dentry->d_lock);
		}
	}

Although 'dir' has the encryption key available, fscrypt_has_encryption_key()
will return 'false' because fscrypt info isn't yet set after the cache
cleanup.

The first patch will add a new helper for the atomic_open that will force
the fscrypt info to be loaded into an inode that has been evicted recently
but for which the key is still available.

The second patch switches ceph atomic_open to use the new fscrypt helper.

Cheers,
--
Luís

Changes since v1:
- Dropped IS_ENCRYPTED() from helper function because kerneldoc says
  already that it applies to encrypted directories and, most importantly,
  because it would introduce a different behaviour for
  CONFIG_FS_ENCRYPTION and !CONFIG_FS_ENCRYPTION.
- Rephrased helper kerneldoc

Changes since initial RFC (after Eric's review):
- Added kerneldoc comments to the new fscrypt helper
- Dropped '__' from helper name (now fscrypt_prepare_atomic_open())
- Added IS_ENCRYPTED() check in helper
- DCACHE_NOKEY_NAME is not set if fscrypt_get_encryption_info() returns an
  error
- Fixed helper for !CONFIG_FS_ENCRYPTION (now defined 'static inline')

Luís Henriques (2):
  fscrypt: new helper function - fscrypt_prepare_atomic_open()
  ceph: switch atomic open to use new fscrypt helper

 fs/ceph/file.c          |  8 +++-----
 fs/crypto/hooks.c       | 33 +++++++++++++++++++++++++++++++++
 include/linux/fscrypt.h |  7 +++++++
 3 files changed, 43 insertions(+), 5 deletions(-)


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH v2 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open()
  2023-03-14 10:39 [PATCH v2 0/2] ceph: fscrypt: fix atomic open bug for encrypted directories Luís Henriques
@ 2023-03-14 10:39 ` Luís Henriques
  2023-03-14 13:37   ` Xiubo Li
  2023-03-14 10:39 ` [PATCH v2 2/2] ceph: switch atomic open to use new fscrypt helper Luís Henriques
  1 sibling, 1 reply; 4+ messages in thread
From: Luís Henriques @ 2023-03-14 10:39 UTC (permalink / raw)
  To: Eric Biggers, Xiubo Li, Jeff Layton
  Cc: Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt,
	ceph-devel, linux-kernel, Luís Henriques

This patch introduces a new helper function which prepares an atomic_open.
Because atomic open can act as a lookup if handed a dentry that is negative,
we need to set DCACHE_NOKEY_NAME if the key for the parent isn't available.

The reason for getting the encryption info before checking if the directory has
the encryption key is because we may have the key available but the encryption
info isn't yet set (maybe due to a drop_caches).  The regular open path will
call fscrypt_file_open() which uses function fscrypt_require_key() for setting
the encryption info if needed.  The atomic open needs to do something similar.

Signed-off-by: Luís Henriques <lhenriques@suse.de>
---
 fs/crypto/hooks.c       | 33 +++++++++++++++++++++++++++++++++
 include/linux/fscrypt.h |  7 +++++++
 2 files changed, 40 insertions(+)

diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
index 7b8c5a1104b5..165ddf01bf9f 100644
--- a/fs/crypto/hooks.c
+++ b/fs/crypto/hooks.c
@@ -117,6 +117,39 @@ int __fscrypt_prepare_readdir(struct inode *dir)
 }
 EXPORT_SYMBOL_GPL(__fscrypt_prepare_readdir);
 
+/**
+ * fscrypt_prepare_atomic_open() - prepare an atomic open on an encrypted directory
+ * @dir: inode of parent directory
+ * @dentry: dentry being open
+ *
+ * Because atomic open can act as a lookup if handed a dentry that is negative,
+ * we need to set DCACHE_NOKEY_NAME if the key for the parent isn't available.
+ *
+ * The reason for getting the encryption info before checking if the directory
+ * has the encryption key is because the key may be available but the encryption
+ * info isn't yet set (maybe due to a drop_caches).  The regular open path will
+ * call fscrypt_file_open() which uses function fscrypt_require_key() for
+ * setting the encryption info if needed.  The atomic open needs to do something
+ * similar.
+ *
+ * Return: 0 on success, or an error code if fscrypt_get_encryption_info()
+ * fails.
+ */
+int fscrypt_prepare_atomic_open(struct inode *dir, struct dentry *dentry)
+{
+	int err;
+
+	err = fscrypt_get_encryption_info(dir, true);
+	if (!err && !fscrypt_has_encryption_key(dir)) {
+		spin_lock(&dentry->d_lock);
+		dentry->d_flags |= DCACHE_NOKEY_NAME;
+		spin_unlock(&dentry->d_lock);
+	}
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(fscrypt_prepare_atomic_open);
+
 int __fscrypt_prepare_setattr(struct dentry *dentry, struct iattr *attr)
 {
 	if (attr->ia_valid & ATTR_SIZE)
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 4f5f8a651213..c70acb2a737a 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -362,6 +362,7 @@ int __fscrypt_prepare_rename(struct inode *old_dir, struct dentry *old_dentry,
 int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry,
 			     struct fscrypt_name *fname);
 int __fscrypt_prepare_readdir(struct inode *dir);
+int fscrypt_prepare_atomic_open(struct inode *dir, struct dentry *dentry);
 int __fscrypt_prepare_setattr(struct dentry *dentry, struct iattr *attr);
 int fscrypt_prepare_setflags(struct inode *inode,
 			     unsigned int oldflags, unsigned int flags);
@@ -688,6 +689,12 @@ static inline int __fscrypt_prepare_readdir(struct inode *dir)
 	return -EOPNOTSUPP;
 }
 
+static inline int fscrypt_prepare_atomic_open(struct inode *dir,
+					      struct dentry *dentry)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int __fscrypt_prepare_setattr(struct dentry *dentry,
 					    struct iattr *attr)
 {

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH v2 2/2] ceph: switch atomic open to use new fscrypt helper
  2023-03-14 10:39 [PATCH v2 0/2] ceph: fscrypt: fix atomic open bug for encrypted directories Luís Henriques
  2023-03-14 10:39 ` [PATCH v2 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() Luís Henriques
@ 2023-03-14 10:39 ` Luís Henriques
  1 sibling, 0 replies; 4+ messages in thread
From: Luís Henriques @ 2023-03-14 10:39 UTC (permalink / raw)
  To: Eric Biggers, Xiubo Li, Jeff Layton
  Cc: Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt,
	ceph-devel, linux-kernel, Luís Henriques

Switch ceph atomic open to use fscrypt_prepare_atomic_open().  This fixes
a bug where a dentry is incorrectly set with DCACHE_NOKEY_NAME when 'dir'
has been evicted but the key is still available (for example, where there's
a drop_caches).

Signed-off-by: Luís Henriques <lhenriques@suse.de>
---
 fs/ceph/file.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index dee3b445f415..5ad57cc4c13b 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -795,11 +795,9 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
 	ihold(dir);
 	if (IS_ENCRYPTED(dir)) {
 		set_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags);
-		if (!fscrypt_has_encryption_key(dir)) {
-			spin_lock(&dentry->d_lock);
-			dentry->d_flags |= DCACHE_NOKEY_NAME;
-			spin_unlock(&dentry->d_lock);
-		}
+		err = fscrypt_prepare_atomic_open(dir, dentry);
+		if (err)
+			goto out_req;
 	}
 
 	if (flags & O_CREAT) {

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open()
  2023-03-14 10:39 ` [PATCH v2 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() Luís Henriques
@ 2023-03-14 13:37   ` Xiubo Li
  0 siblings, 0 replies; 4+ messages in thread
From: Xiubo Li @ 2023-03-14 13:37 UTC (permalink / raw)
  To: Luís Henriques, Eric Biggers, Jeff Layton
  Cc: Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt,
	ceph-devel, linux-kernel


On 14/03/2023 18:39, Luís Henriques wrote:
> This patch introduces a new helper function which prepares an atomic_open.
> Because atomic open can act as a lookup if handed a dentry that is negative,
> we need to set DCACHE_NOKEY_NAME if the key for the parent isn't available.
>
> The reason for getting the encryption info before checking if the directory has
> the encryption key is because we may have the key available but the encryption
> info isn't yet set (maybe due to a drop_caches).  The regular open path will
> call fscrypt_file_open() which uses function fscrypt_require_key() for setting
> the encryption info if needed.  The atomic open needs to do something similar.
>
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>   fs/crypto/hooks.c       | 33 +++++++++++++++++++++++++++++++++
>   include/linux/fscrypt.h |  7 +++++++
>   2 files changed, 40 insertions(+)
>
> diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
> index 7b8c5a1104b5..165ddf01bf9f 100644
> --- a/fs/crypto/hooks.c
> +++ b/fs/crypto/hooks.c
> @@ -117,6 +117,39 @@ int __fscrypt_prepare_readdir(struct inode *dir)
>   }
>   EXPORT_SYMBOL_GPL(__fscrypt_prepare_readdir);
>   
> +/**
> + * fscrypt_prepare_atomic_open() - prepare an atomic open on an encrypted directory
> + * @dir: inode of parent directory
> + * @dentry: dentry being open
> + *
> + * Because atomic open can act as a lookup if handed a dentry that is negative,
> + * we need to set DCACHE_NOKEY_NAME if the key for the parent isn't available.
> + *
> + * The reason for getting the encryption info before checking if the directory
> + * has the encryption key is because the key may be available but the encryption
> + * info isn't yet set (maybe due to a drop_caches).  The regular open path will
> + * call fscrypt_file_open() which uses function fscrypt_require_key() for
> + * setting the encryption info if needed.  The atomic open needs to do something
> + * similar.
> + *
> + * Return: 0 on success, or an error code if fscrypt_get_encryption_info()
> + * fails.
> + */
> +int fscrypt_prepare_atomic_open(struct inode *dir, struct dentry *dentry)
> +{
> +	int err;
> +
> +	err = fscrypt_get_encryption_info(dir, true);
> +	if (!err && !fscrypt_has_encryption_key(dir)) {
> +		spin_lock(&dentry->d_lock);
> +		dentry->d_flags |= DCACHE_NOKEY_NAME;
> +		spin_unlock(&dentry->d_lock);
> +	}
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(fscrypt_prepare_atomic_open);
> +
>   int __fscrypt_prepare_setattr(struct dentry *dentry, struct iattr *attr)
>   {
>   	if (attr->ia_valid & ATTR_SIZE)
> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> index 4f5f8a651213..c70acb2a737a 100644
> --- a/include/linux/fscrypt.h
> +++ b/include/linux/fscrypt.h
> @@ -362,6 +362,7 @@ int __fscrypt_prepare_rename(struct inode *old_dir, struct dentry *old_dentry,
>   int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry,
>   			     struct fscrypt_name *fname);
>   int __fscrypt_prepare_readdir(struct inode *dir);
> +int fscrypt_prepare_atomic_open(struct inode *dir, struct dentry *dentry);
>   int __fscrypt_prepare_setattr(struct dentry *dentry, struct iattr *attr);
>   int fscrypt_prepare_setflags(struct inode *inode,
>   			     unsigned int oldflags, unsigned int flags);
> @@ -688,6 +689,12 @@ static inline int __fscrypt_prepare_readdir(struct inode *dir)
>   	return -EOPNOTSUPP;
>   }
>   
> +static inline int fscrypt_prepare_atomic_open(struct inode *dir,
> +					      struct dentry *dentry)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
>   static inline int __fscrypt_prepare_setattr(struct dentry *dentry,
>   					    struct iattr *attr)
>   {
>
Reviewed-by: Xiubo Li <xiubli@redhat.com>



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2023-03-14 13:41 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-14 10:39 [PATCH v2 0/2] ceph: fscrypt: fix atomic open bug for encrypted directories Luís Henriques
2023-03-14 10:39 ` [PATCH v2 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() Luís Henriques
2023-03-14 13:37   ` Xiubo Li
2023-03-14 10:39 ` [PATCH v2 2/2] ceph: switch atomic open to use new fscrypt helper Luís Henriques

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).