[PATCH v3 0/3] ceph: fscrypt: fix atomic open bug for encrypted directories

[PATCH v3 0/3] ceph: fscrypt: fix atomic open bug for encrypted directories

[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 v2:
- Make helper more generic and to be used both in lookup and atomic open
  operations
- Modify ceph_lookup (patch 0002) and ceph_atomic_open (patch 0003) to use
  the new helper

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 (3):
  fscrypt: new helper function - fscrypt_prepare_lookup_partial()
  ceph: switch ceph_open() to use new fscrypt helper
  ceph: switch ceph_open_atomic() to use the new fscrypt helper

 fs/ceph/dir.c           | 13 +++++++------
 fs/ceph/file.c          |  8 +++-----
 fs/crypto/hooks.c       | 37 +++++++++++++++++++++++++++++++++++++
 include/linux/fscrypt.h |  7 +++++++
 4 files changed, 54 insertions(+), 11 deletions(-)

[PATCH v3 1/3] fscrypt: new helper function - fscrypt_prepare_lookup_partial()

[Luís Henriques]

This patch introduces a new helper function which can be used both in
lookups and in atomic_open operations by filesystems that want to handle
filename encryption and no-key dentries themselves.

The reason for this function to be used in atomic open is that this
operation can act as a lookup if handed a dentry that is negative.  And in
this case we may need to set DCACHE_NOKEY_NAME.

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

diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
index 7b8c5a1104b5..4bebdeaadc52 100644
--- a/fs/crypto/hooks.c
+++ b/fs/crypto/hooks.c
@@ -117,6 +117,43 @@ int __fscrypt_prepare_readdir(struct inode *dir)
 }
 EXPORT_SYMBOL_GPL(__fscrypt_prepare_readdir);
 
+/**
+ * fscrypt_prepare_lookup_partial() - prepare lookup without filenames handling
+ * @dir: inode of parent directory
+ * @dentry: dentry to lookup
+ *
+ * This function can be used by filesystems that want/need to handle filename
+ * encryption and no-key name encoding themselves, and thus aren't able to use
+ * function fscrypt_prepare_lookup().
+
+ * This helper can be called from lookup and atomic open operations.  It will
+ * try to set the encryption info on the in @dir if the key is available and, if
+ * it isn't, it will also set the @dentry as non-key.
+ *
+ * The reason it needs to get 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 use fscrypt_prepare_lookup(), but if a filesystem can't use this
+ * function (because it handles the filename encryption and no-key dentries) the
+ * atomic_open requires a different approach.
+ *
+ * Return: 0 on success, or an error code if fscrypt_get_encryption_info()
+ * 	   fails.
+ */
+int fscrypt_prepare_lookup_partial(struct inode *dir, struct dentry *dentry)
+{
+	int 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_lookup_partial);
+
 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..d9a94cd56cda 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_lookup_partial(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_lookup_partial(struct inode *dir,
+						 struct dentry *dentry)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int __fscrypt_prepare_setattr(struct dentry *dentry,
 					    struct iattr *attr)
 {

[PATCH v3 2/3] ceph: switch ceph_open() to use new fscrypt helper

[Luís Henriques]

Instead of setting the no-key dentry in ceph_lookup(), use the new
fscrypt_prepare_lookup_partial() helper.  We still need to mark the
directory as incomplete if the directory was just unlocked.

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

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index d8cc6e9d5351..836bc695e2e0 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -784,14 +784,15 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
 		return ERR_PTR(-ENAMETOOLONG);
 
 	if (IS_ENCRYPTED(dir)) {
-		err = ceph_fscrypt_prepare_readdir(dir);
+		bool had_key = fscrypt_has_encryption_key(dir);
+
+		err = fscrypt_prepare_lookup_partial(dir, dentry);
 		if (err < 0)
 			return ERR_PTR(err);
-		if (!fscrypt_has_encryption_key(dir)) {
-			spin_lock(&dentry->d_lock);
-			dentry->d_flags |= DCACHE_NOKEY_NAME;
-			spin_unlock(&dentry->d_lock);
-		}
+
+		/* mark directory as incomplete if it has been unlocked */
+		if (!had_key && fscrypt_has_encryption_key(dir))
+			ceph_dir_clear_complete(dir);
 	}
 
 	/* can we conclude ENOENT locally? */

[PATCH v3 3/3] ceph: switch ceph_open_atomic() to use the new fscrypt helper

[Luís Henriques]

Switch ceph_atomic_open() to use new fscrypt helper function
fscrypt_prepare_lookup_partial().  This fixes a bug in the atomic open
operation 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..2448d0f1a9ea 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_lookup_partial(dir, dentry);
+		if (err < 0)
+			goto out_req;
 	}
 
 	if (flags & O_CREAT) {

Re: [PATCH v3 0/3] ceph: fscrypt: fix atomic open bug for encrypted directories

[Xiubo Li]

On 17/03/2023 02:14, Luís Henriques wrote:
> 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 v2:
> - Make helper more generic and to be used both in lookup and atomic open
>    operations
> - Modify ceph_lookup (patch 0002) and ceph_atomic_open (patch 0003) to use
>    the new helper
>
> 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')

This series looks good to me.

And I have run the test locally and worked well.


> Luís Henriques (3):
>    fscrypt: new helper function - fscrypt_prepare_lookup_partial()

Eric,

If possible I we can pick this together to ceph repo and need your ack 
about this. Or you can pick it to the crypto repo then please feel free 
to add:

Tested-by: Xiubo Li <xiubli@redhat.com> and Reviewed-by: Xiubo Li 
<xiubli@redhat.com>

Thanks

- Xiubo

>    ceph: switch ceph_open() to use new fscrypt helper
>    ceph: switch ceph_open_atomic() to use the new fscrypt helper
>
>   fs/ceph/dir.c           | 13 +++++++------
>   fs/ceph/file.c          |  8 +++-----
>   fs/crypto/hooks.c       | 37 +++++++++++++++++++++++++++++++++++++
>   include/linux/fscrypt.h |  7 +++++++
>   4 files changed, 54 insertions(+), 11 deletions(-)
>
-- 
Best Regards,

Xiubo Li (李秀波)

Email: xiubli@redhat.com/xiubli@ibm.com
Slack: @Xiubo Li

Re: [PATCH v3 0/3] ceph: fscrypt: fix atomic open bug for encrypted directories

[Ilya Dryomov]

On Mon, Mar 20, 2023 at 2:07 AM Xiubo Li <xiubli@redhat.com> wrote:
>
>
> On 17/03/2023 02:14, Luís Henriques wrote:
> > 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 v2:
> > - Make helper more generic and to be used both in lookup and atomic open
> >    operations
> > - Modify ceph_lookup (patch 0002) and ceph_atomic_open (patch 0003) to use
> >    the new helper
> >
> > 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')
>
> This series looks good to me.
>
> And I have run the test locally and worked well.
>
>
> > Luís Henriques (3):
> >    fscrypt: new helper function - fscrypt_prepare_lookup_partial()
>
> Eric,
>
> If possible I we can pick this together to ceph repo and need your ack
> about this. Or you can pick it to the crypto repo then please feel free
> to add:
>
> Tested-by: Xiubo Li <xiubli@redhat.com> and Reviewed-by: Xiubo Li
> <xiubli@redhat.com>

I would prefer the fscrypt helper to go through the fscrypt tree.

Thanks,

                Ilya

Re: [PATCH v3 0/3] ceph: fscrypt: fix atomic open bug for encrypted directories

[Xiubo Li]

On 20/03/2023 19:20, Ilya Dryomov wrote:
> On Mon, Mar 20, 2023 at 2:07 AM Xiubo Li <xiubli@redhat.com> wrote:
>>
>> On 17/03/2023 02:14, Luís Henriques wrote:
>>> 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 v2:
>>> - Make helper more generic and to be used both in lookup and atomic open
>>>     operations
>>> - Modify ceph_lookup (patch 0002) and ceph_atomic_open (patch 0003) to use
>>>     the new helper
>>>
>>> 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')
>> This series looks good to me.
>>
>> And I have run the test locally and worked well.
>>
>>
>>> Luís Henriques (3):
>>>     fscrypt: new helper function - fscrypt_prepare_lookup_partial()
>> Eric,
>>
>> If possible I we can pick this together to ceph repo and need your ack
>> about this. Or you can pick it to the crypto repo then please feel free
>> to add:
>>
>> Tested-by: Xiubo Li <xiubli@redhat.com> and Reviewed-by: Xiubo Li
>> <xiubli@redhat.com>
> I would prefer the fscrypt helper to go through the fscrypt tree.

Sure. This also LGTM.

Thanks

- Xiubo

> Thanks,
>
>                  Ilya
>

Re: [PATCH v3 0/3] ceph: fscrypt: fix atomic open bug for encrypted directories

[Luís Henriques]

Xiubo Li <xiubli@redhat.com> writes:

> On 17/03/2023 02:14, Luís Henriques wrote:
>> 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 v2:
>> - Make helper more generic and to be used both in lookup and atomic open
>>    operations
>> - Modify ceph_lookup (patch 0002) and ceph_atomic_open (patch 0003) to use
>>    the new helper
>>
>> 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')
>
> This series looks good to me.
>
> And I have run the test locally and worked well.

Awesome, thanks a lot Xiubo.  I've been testing it locally as well and I
haven't observed any breakage either.

Cheers,
-- 
Luís

Re: [PATCH v3 0/3] ceph: fscrypt: fix atomic open bug for encrypted directories

[Eric Biggers]

On Mon, Mar 20, 2023 at 08:47:18PM +0800, Xiubo Li wrote:
> 
> On 20/03/2023 19:20, Ilya Dryomov wrote:
> > On Mon, Mar 20, 2023 at 2:07 AM Xiubo Li <xiubli@redhat.com> wrote:
> > > 
> > > On 17/03/2023 02:14, Luís Henriques wrote:
> > > > 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 v2:
> > > > - Make helper more generic and to be used both in lookup and atomic open
> > > >     operations
> > > > - Modify ceph_lookup (patch 0002) and ceph_atomic_open (patch 0003) to use
> > > >     the new helper
> > > > 
> > > > 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')
> > > This series looks good to me.
> > > 
> > > And I have run the test locally and worked well.
> > > 
> > > 
> > > > Luís Henriques (3):
> > > >     fscrypt: new helper function - fscrypt_prepare_lookup_partial()
> > > Eric,
> > > 
> > > If possible I we can pick this together to ceph repo and need your ack
> > > about this. Or you can pick it to the crypto repo then please feel free
> > > to add:
> > > 
> > > Tested-by: Xiubo Li <xiubli@redhat.com> and Reviewed-by: Xiubo Li
> > > <xiubli@redhat.com>
> > I would prefer the fscrypt helper to go through the fscrypt tree.
> 
> Sure. This also LGTM.
> 
> Thanks
> 

I've applied it to
https://git.kernel.org/pub/scm/fs/fscrypt/linux.git/log/?h=for-next

But I ended up reworking the comment a bit and moving the function to be just
below __fscrypt_prepare_lookup().  So I sent out v4 that matches what I applied.

BTW, I'm wondering if anyone has had any thoughts about the race condition I
described at https://lore.kernel.org/r/ZBC1P4Gn6eAKD61+@sol.localdomain/.  In
particular, I'm wondering whether this helper function will need to be changed
or not.  Maybe not, because ceph could look at DCACHE_NOKEY_NAME to determine
whether the name should be treated as a no-key name or not, instead of checking
fscrypt_has_encryption_key() again (as I think it is doing currently)?

- Eric

Re: [PATCH v3 0/3] ceph: fscrypt: fix atomic open bug for encrypted directories

[Luís Henriques]

Eric Biggers <ebiggers@kernel.org> writes:

> On Mon, Mar 20, 2023 at 08:47:18PM +0800, Xiubo Li wrote:
>> 
>> On 20/03/2023 19:20, Ilya Dryomov wrote:
>> > On Mon, Mar 20, 2023 at 2:07 AM Xiubo Li <xiubli@redhat.com> wrote:
>> > > 
>> > > On 17/03/2023 02:14, Luís Henriques wrote:
>> > > > 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 v2:
>> > > > - Make helper more generic and to be used both in lookup and atomic open
>> > > >     operations
>> > > > - Modify ceph_lookup (patch 0002) and ceph_atomic_open (patch 0003) to use
>> > > >     the new helper
>> > > > 
>> > > > 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')
>> > > This series looks good to me.
>> > > 
>> > > And I have run the test locally and worked well.
>> > > 
>> > > 
>> > > > Luís Henriques (3):
>> > > >     fscrypt: new helper function - fscrypt_prepare_lookup_partial()
>> > > Eric,
>> > > 
>> > > If possible I we can pick this together to ceph repo and need your ack
>> > > about this. Or you can pick it to the crypto repo then please feel free
>> > > to add:
>> > > 
>> > > Tested-by: Xiubo Li <xiubli@redhat.com> and Reviewed-by: Xiubo Li
>> > > <xiubli@redhat.com>
>> > I would prefer the fscrypt helper to go through the fscrypt tree.
>> 
>> Sure. This also LGTM.
>> 
>> Thanks
>> 
>
> I've applied it to
> https://git.kernel.org/pub/scm/fs/fscrypt/linux.git/log/?h=for-next
>
> But I ended up reworking the comment a bit and moving the function to be just
> below __fscrypt_prepare_lookup().  So I sent out v4 that matches what I applied.

Awesome, thanks a lot, Eric.

> BTW, I'm wondering if anyone has had any thoughts about the race condition I
> described at https://lore.kernel.org/r/ZBC1P4Gn6eAKD61+@sol.localdomain/.  In
> particular, I'm wondering whether this helper function will need to be changed
> or not.  Maybe not, because ceph could look at DCACHE_NOKEY_NAME to determine
> whether the name should be treated as a no-key name or not, instead of checking
> fscrypt_has_encryption_key() again (as I think it is doing currently)?

I started looking into that but, to be honest, I haven't yet reached any
conclusion.  It looks like the ceph code that handles filenames *may* have
this race too (I'm looking at ceph_fill_trace()) but I'm still not 100%
sure.  In any case, I think that an eventual fix for this race (if it does
indeed exist!) will likely be restricted to the ceph code and won't touch
the generic fscrypt code.  But I'm still looking...

Cheers,
-- 
Luís