[PATCH] ceph: always set the initial i_blkbits to CEPH_FSCRYPT_BLOCK_SHIFT

[PATCH] ceph: always set the initial i_blkbits to CEPH_FSCRYPT_BLOCK_SHIFT

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

The fscrypt code will use i_blkbits to setup the 'ci_data_unit_bits'
when allocating the new inode, but ceph will initiate i_blkbits
ater when filling the inode, which is too late. Since the
'ci_data_unit_bits' will only be used by the fscrypt framework so
initiating i_blkbits with CEPH_FSCRYPT_BLOCK_SHIFT is safe.

Fixes: commit 5b1188847180 ("fscrypt: support crypto data unit size
       less than filesystem block size")
URL: https://tracker.ceph.com/issues/64035
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/inode.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 62af452cdba4..d96d97b31f68 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -79,6 +79,8 @@ struct inode *ceph_new_inode(struct inode *dir, struct dentry *dentry,
 	if (!inode)
 		return ERR_PTR(-ENOMEM);
 
+	inode->i_blkbits = CEPH_FSCRYPT_BLOCK_SHIFT;
+
 	if (!S_ISLNK(*mode)) {
 		err = ceph_pre_init_acls(dir, mode, as_ctx);
 		if (err < 0)
-- 
2.39.1

Re: [PATCH] ceph: always set the initial i_blkbits to CEPH_FSCRYPT_BLOCK_SHIFT

[Eric Biggers]

On Thu, Jan 18, 2024 at 04:04:04PM +0800, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> The fscrypt code will use i_blkbits to setup the 'ci_data_unit_bits'
> when allocating the new inode, but ceph will initiate i_blkbits
> ater when filling the inode, which is too late. Since the
> 'ci_data_unit_bits' will only be used by the fscrypt framework so
> initiating i_blkbits with CEPH_FSCRYPT_BLOCK_SHIFT is safe.
> 
> Fixes: commit 5b1188847180 ("fscrypt: support crypto data unit size
>        less than filesystem block size")

The Fixes line should be one line, and the word "commit" should not be there

> URL: https://tracker.ceph.com/issues/64035
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/inode.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 62af452cdba4..d96d97b31f68 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -79,6 +79,8 @@ struct inode *ceph_new_inode(struct inode *dir, struct dentry *dentry,
>  	if (!inode)
>  		return ERR_PTR(-ENOMEM);
>  
> +	inode->i_blkbits = CEPH_FSCRYPT_BLOCK_SHIFT;
> +
>  	if (!S_ISLNK(*mode)) {
>  		err = ceph_pre_init_acls(dir, mode, as_ctx);
>  		if (err < 0)

Looks good, you can add:

    Reviewed-by: Eric Biggers <ebiggers@google.com>

Sorry for the trouble; I need to start running xfstests on CephFS!

In the future please also Cc the fscrypt mailing list on things like this.

Maybe you'd like to also send a patch that updates the comment for
fscrypt_prepare_new_inode() to clarify that i_blkbits needs to be set first?

- Eric

Re: [PATCH] ceph: always set the initial i_blkbits to CEPH_FSCRYPT_BLOCK_SHIFT

[Xiubo Li]

On 1/25/24 09:53, Eric Biggers wrote:
> On Thu, Jan 18, 2024 at 04:04:04PM +0800, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> The fscrypt code will use i_blkbits to setup the 'ci_data_unit_bits'
>> when allocating the new inode, but ceph will initiate i_blkbits
>> ater when filling the inode, which is too late. Since the
>> 'ci_data_unit_bits' will only be used by the fscrypt framework so
>> initiating i_blkbits with CEPH_FSCRYPT_BLOCK_SHIFT is safe.
>>
>> Fixes: commit 5b1188847180 ("fscrypt: support crypto data unit size
>>         less than filesystem block size")
> The Fixes line should be one line, and the word "commit" should not be there

Sure, I will fix it.

>> URL: https://tracker.ceph.com/issues/64035
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   fs/ceph/inode.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>> index 62af452cdba4..d96d97b31f68 100644
>> --- a/fs/ceph/inode.c
>> +++ b/fs/ceph/inode.c
>> @@ -79,6 +79,8 @@ struct inode *ceph_new_inode(struct inode *dir, struct dentry *dentry,
>>   	if (!inode)
>>   		return ERR_PTR(-ENOMEM);
>>   
>> +	inode->i_blkbits = CEPH_FSCRYPT_BLOCK_SHIFT;
>> +
>>   	if (!S_ISLNK(*mode)) {
>>   		err = ceph_pre_init_acls(dir, mode, as_ctx);
>>   		if (err < 0)
> Looks good, you can add:
>
>      Reviewed-by: Eric Biggers <ebiggers@google.com>
>
> Sorry for the trouble; I need to start running xfstests on CephFS!
Thanks Eric, no worry, this will only be seen when the 
'test_dummy_encryption' is enabled.
> In the future please also Cc the fscrypt mailing list on things like this.

Sure, I meant to Cc you but I think I forgot that. Sorry.

> Maybe you'd like to also send a patch that updates the comment for
> fscrypt_prepare_new_inode() to clarify that i_blkbits needs to be set first?

Will fix it.

Thanks Eric.

- Xiubo


> - Eric
>