[2/2] ceph: switch atomic open to use new fscrypt helper

  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(-)
  

On Mon, Mar 13, 2023 at 12:33:10PM +0000, Luís Henriques wrote:
> 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;

Note that this patch does not apply to upstream or even to linux-next.

I'd be glad to take patch 1 through the fscrypt tree for 6.4.  But I'm wondering
what the current plans are for getting ceph's fscrypt support upstream?

- Eric
  

Eric Biggers <ebiggers@kernel.org> writes:

> On Mon, Mar 13, 2023 at 12:33:10PM +0000, Luís Henriques wrote:
>> 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;
>
> Note that this patch does not apply to upstream or even to linux-next.

True, I should have mentioned that in the cover-letter.  This patch should
be applied against the 'testing' branch in https://github.com/ceph/ceph-client,
which is where the ceph fscrypt currently lives.

> I'd be glad to take patch 1 through the fscrypt tree for 6.4.  But I'm wondering
> what the current plans are for getting ceph's fscrypt support upstream?

As far as I know, the current plan is to try to merge the ceph code during
the next merge window for 6.4 (but Xiubo and Ilya may correct me if I'm
wrong).  Also, regarding who picks which patch, I'm fine with you picking
the first one.  But I'll let the ceph maintainers say what they think,
because it may be easier for them to keep both patches together due to the
testing infrastructure being used.

Anyway, I'll send out a new rev tomorrow taking your comments into
account.  Thanks, Eric!

Cheers,
  

On 14/03/2023 02:42, Luís Henriques wrote:
> Eric Biggers <ebiggers@kernel.org> writes:
>
>> On Mon, Mar 13, 2023 at 12:33:10PM +0000, Luís Henriques wrote:
>>> 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;
>> Note that this patch does not apply to upstream or even to linux-next.
> True, I should have mentioned that in the cover-letter.  This patch should
> be applied against the 'testing' branch in https://github.com/ceph/ceph-client,
> which is where the ceph fscrypt currently lives.
>
>> I'd be glad to take patch 1 through the fscrypt tree for 6.4.  But I'm wondering
>> what the current plans are for getting ceph's fscrypt support upstream?
> As far as I know, the current plan is to try to merge the ceph code during
> the next merge window for 6.4 (but Xiubo and Ilya may correct me if I'm
> wrong).  Also, regarding who picks which patch, I'm fine with you picking
> the first one.  But I'll let the ceph maintainers say what they think,
> because it may be easier for them to keep both patches together due to the
> testing infrastructure being used.
>
> Anyway, I'll send out a new rev tomorrow taking your comments into
> account.  Thanks, Eric!

Eric, Luis,

It will be fine if Eric could merge patch 1 into the fscrypt tree. Then 
I will merge the patch 1 into the ceph-client's testing by tagging as 
[DO NOT MERGE] to run our tests.

And locally we are still running the test, and there have several fixes 
followed and need more time to review.

Thanks

- Xiubo

> Cheers,
  

Xiubo Li <xiubli@redhat.com> writes:

> On 14/03/2023 02:42, Luís Henriques wrote:
>> Eric Biggers <ebiggers@kernel.org> writes:
>>
>>> On Mon, Mar 13, 2023 at 12:33:10PM +0000, Luís Henriques wrote:
>>>> 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;
>>> Note that this patch does not apply to upstream or even to linux-next.
>> True, I should have mentioned that in the cover-letter.  This patch should
>> be applied against the 'testing' branch in https://github.com/ceph/ceph-client,
>> which is where the ceph fscrypt currently lives.
>>
>>> I'd be glad to take patch 1 through the fscrypt tree for 6.4.  But I'm wondering
>>> what the current plans are for getting ceph's fscrypt support upstream?
>> As far as I know, the current plan is to try to merge the ceph code during
>> the next merge window for 6.4 (but Xiubo and Ilya may correct me if I'm
>> wrong).  Also, regarding who picks which patch, I'm fine with you picking
>> the first one.  But I'll let the ceph maintainers say what they think,
>> because it may be easier for them to keep both patches together due to the
>> testing infrastructure being used.
>>
>> Anyway, I'll send out a new rev tomorrow taking your comments into
>> account.  Thanks, Eric!
>
> Eric, Luis,
>
> It will be fine if Eric could merge patch 1 into the fscrypt tree. Then I will
> merge the patch 1 into the ceph-client's testing by tagging as [DO NOT MERGE] to
> run our tests.

Awesome, so Eric can pick the first patch.  Thanks.

Cheers,