diff mbox series

[RFC,1/2] fscrypt: new helper function - __fscrypt_prepare_atomic_open()

Message ID 20230309121910.18939-2-lhenriques@suse.de
State New
Headers
Series ceph: fscrypt: fix atomic open bug for encrypted directories |

Commit Message

Luis Henriques March 9, 2023, 12:19 p.m. UTC 
  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 use fscrypt_file_open for that but in the atomic open a
different approach is required.

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

Comments

Eric Biggers March 9, 2023, 6:23 p.m. UTC | #1 
On Thu, Mar 09, 2023 at 12:19:09PM +0000, 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 use fscrypt_file_open for that but in the atomic open a
> different approach is required.
> 
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>  fs/crypto/hooks.c       | 14 ++++++++++++++
>  include/linux/fscrypt.h |  6 ++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
> index 7b8c5a1104b5..cbb828ecc5eb 100644
> --- a/fs/crypto/hooks.c
> +++ b/fs/crypto/hooks.c
> @@ -117,6 +117,20 @@ int __fscrypt_prepare_readdir(struct inode *dir)
>  }
>  EXPORT_SYMBOL_GPL(__fscrypt_prepare_readdir);
>  
> +int __fscrypt_prepare_atomic_open(struct inode *dir, struct dentry *dentry)

Anything exported to filesystems should have a kerneldoc comment.  That would be
a good place to put some of the explanation that you currently have only in the
commit message.

Also, double-underscored functions are not for use by filesystems directly.
Normally the pattern would be to make fscrypt_prepare_atomic_open() an inline
function that checks IS_ENCRYPTED() and calls an out-of-line function
__fscrypt_prepare_atomic_open().  But if it happens to be simpler to make the
caller handle the IS_ENCRYPTED() check in this case, then there should simply be
one function: fscrypt_prepare_atomic_open() (no leading underscores).

> +{
> +	int err = fscrypt_get_encryption_info(dir, true);
> +
> +	if (err || (!err && !fscrypt_has_encryption_key(dir))) {
> +		spin_lock(&dentry->d_lock);
> +		dentry->d_flags |= DCACHE_NOKEY_NAME;
> +		spin_unlock(&dentry->d_lock);
> +	}

Why does DCACHE_NOKEY_NAME need to be set on error?

Also note that the '!err &&' part has no effect.

- Eric
Luis Henriques March 10, 2023, 12:05 p.m. UTC | #2 
Eric Biggers <ebiggers@kernel.org> writes:

> On Thu, Mar 09, 2023 at 12:19:09PM +0000, 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 use fscrypt_file_open for that but in the atomic open a
>> different approach is required.
>> 
>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>> ---
>>  fs/crypto/hooks.c       | 14 ++++++++++++++
>>  include/linux/fscrypt.h |  6 ++++++
>>  2 files changed, 20 insertions(+)
>> 
>> diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
>> index 7b8c5a1104b5..cbb828ecc5eb 100644
>> --- a/fs/crypto/hooks.c
>> +++ b/fs/crypto/hooks.c
>> @@ -117,6 +117,20 @@ int __fscrypt_prepare_readdir(struct inode *dir)
>>  }
>>  EXPORT_SYMBOL_GPL(__fscrypt_prepare_readdir);
>>  
>> +int __fscrypt_prepare_atomic_open(struct inode *dir, struct dentry *dentry)
>
> Anything exported to filesystems should have a kerneldoc comment.  That would be
> a good place to put some of the explanation that you currently have only in the
> commit message.
>
> Also, double-underscored functions are not for use by filesystems directly.
> Normally the pattern would be to make fscrypt_prepare_atomic_open() an inline
> function that checks IS_ENCRYPTED() and calls an out-of-line function
> __fscrypt_prepare_atomic_open().  But if it happens to be simpler to make the
> caller handle the IS_ENCRYPTED() check in this case, then there should simply be
> one function: fscrypt_prepare_atomic_open() (no leading underscores).

Thank you, Eric.  I'll make sure that next rev will take these comments
into account.  It definitely makes sense to move (or duplicate) the
details as a kerneldoc comment.

>> +{
>> +	int err = fscrypt_get_encryption_info(dir, true);
>> +
>> +	if (err || (!err && !fscrypt_has_encryption_key(dir))) {
>> +		spin_lock(&dentry->d_lock);
>> +		dentry->d_flags |= DCACHE_NOKEY_NAME;
>> +		spin_unlock(&dentry->d_lock);
>> +	}
>
> Why does DCACHE_NOKEY_NAME need to be set on error?
> 
> Also note that the '!err &&' part has no effect.

To be honest, I wasn't really sure that if the d_flags should be set on
error either.  I'll drop that, and then the 'if' statement will make more
sense without the '||'.

Cheers
diff mbox series

Patch

diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
index 7b8c5a1104b5..cbb828ecc5eb 100644
--- a/fs/crypto/hooks.c
+++ b/fs/crypto/hooks.c
@@ -117,6 +117,20 @@  int __fscrypt_prepare_readdir(struct inode *dir)
 }
 EXPORT_SYMBOL_GPL(__fscrypt_prepare_readdir);
 
+int __fscrypt_prepare_atomic_open(struct inode *dir, struct dentry *dentry)
+{
+	int err = fscrypt_get_encryption_info(dir, true);
+
+	if (err || (!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..51c4b216a625 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,11 @@  static inline int __fscrypt_prepare_readdir(struct inode *dir)
 	return -EOPNOTSUPP;
 }
 
+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)
 {