Re: [PATCH 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open()

["Luís Henriques" <lhenriques@suse.de>]

Eric Biggers <ebiggers@kernel.org> writes:

> On Mon, Mar 13, 2023 at 12:33:09PM +0000, Luís Henriques wrote:
>> + * The regular open path will use fscrypt_file_open for that, but in the
>> + * atomic open a different approach is required.
>
> This should actually be fscrypt_prepare_lookup, not fscrypt_file_open, right?

Ups, I missed this comment.

I was comparing the regular open() with the atomic_open() paths.  I think
I really mean fscrypt_file_open() because that's where the encryption info
is (or may be) set by calling fscrypt_require_key().  atomic_open needs
something similar, but combined with a lookup.

Maybe I can rephrase it to:

  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.

Cheers,
-- 
Luís

>> +int fscrypt_prepare_atomic_open(struct inode *dir, struct dentry *dentry)
>> +{
>> +	int err;
>> +
>> +	if (!IS_ENCRYPTED(dir))
>> +		return 0;
>> +
>> +	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);
> [...]
>> +static inline int fscrypt_prepare_atomic_open(struct inode *dir,
>> +					      struct dentry *dentry)
>> +{
>> +	return -EOPNOTSUPP;
>> +}
>
> This has different behavior on unencrypted directories depending on whether
> CONFIG_FS_ENCRYPTION is enabled or not.  That's bad.
>
> In patch 2, the caller you are introducing has already checked IS_ENCRYPTED().
>
> Also, your kerneldoc comment for fscrypt_prepare_atomic_open() says it is for
> *encrypted* directories.
>
> So IMO, just remove the IS_ENCRYPTED() check from the CONFIG_FS_ENCRYPTION
> version of fscrypt_prepare_atomic_open().
>
> - Eric