diff mbox series

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

Message ID 20230313123310.13040-3-lhenriques@suse.de (mailing list archive)
State New, archived
Headers show
Series ceph: fscrypt: fix atomic open bug for encrypted directories | expand

Commit Message

Luis Henriques March 13, 2023, 12:33 p.m. UTC
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(-)

Comments

Eric Biggers March 13, 2023, 6:11 p.m. UTC | #1
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
Luis Henriques March 13, 2023, 6:42 p.m. UTC | #2
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,
Xiubo Li March 14, 2023, 12:38 a.m. UTC | #3
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,
Luis Henriques March 14, 2023, 9:27 a.m. UTC | #4
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,
diff mbox series

Patch

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