linux-kernel - Re: [PATCH v3] erofs: fix file handle encoding for 64-bit NIDs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3e068311-9223-4c1b-9091-15eb2d867ede@huawei.com>
Date: Wed, 7 May 2025 09:53:22 +0800
From: Hongbo Li <lihongbo22@...wei.com>
To: Gao Xiang <hsiangkao@...ux.alibaba.com>, <xiang@...nel.org>,
	<chao@...nel.org>, <zbestahu@...il.com>, <jefflexu@...ux.alibaba.com>
CC: <dhavale@...gle.com>, <linux-erofs@...ts.ozlabs.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] erofs: fix file handle encoding for 64-bit NIDs



On 2025/5/6 23:10, Gao Xiang wrote:
> Hi Hongbo,
> 
> On 2025/4/29 21:42, Hongbo Li wrote:
>> In erofs, the inode number has the location information of
>> files. The default encode_fh uses the ino32, this will lack
>> of some information when the file is too big. So we need
>> the internal helpers to encode filehandle.
> 
> EROFS uses NID to indicate the on-disk inode offset, which can
> exceed 32 bits. However, the default encode_fh uses the ino32,
> thus it doesn't work if the image is large than 128GiB.
> 
Thanks for helping me correct my description.

Here, an image larger than 128GiB won't trigger NID reversal. It 
requires a 128GiB file inside, and the NID of the second file may exceed 
U32 during formatting. So here can we change it to "However, the default 
encode_fh uses the ino32, thus it may not work if there exist a file 
which is large than 128GiB." ?

Thanks,
Hongbo

> Let's introduce our own helpers to encode file handles.
> 
>>
>> It is easy to reproduce test:
> 
> It's easy to reproduce:
> 
>>    1. prepare an erofs image with nid bigger than UINT_MAX
> 
>       1. Prepare an EROFS image with NIDs larger than U32_MAX
> 
>>    2. mount -t erofs foo.img /mnt/erofs
>>    3. set exportfs with configuration: /mnt/erofs *(rw,sync,
>>       no_root_squash)
>>    4. mount -t nfs $IP:/mnt/erofs /mnt/nfs
>>    5. md5sum /mnt/nfs/foo # foo is the file which nid bigger
>>       than UINT_MAX.
>> For overlayfs case, the under filesystem's file handle is
>> encoded in ovl_fb.fid, it is same as NFS's case.
>>
>> Fixes: 3e917cc305c6 ("erofs: make filesystem exportable")
>> Signed-off-by: Hongbo Li <lihongbo22@...wei.com>
>> ---
>> v2: 
>> https://lore.kernel.org/all/20250429074109.689075-1-lihongbo22@huawei.com/
>>    - Assign parent nid with correct value.
>>
>> v1: 
>> https://lore.kernel.org/all/20250429011139.686847-1-lihongbo22@huawei.com/
>>     - Encode generation into file handle and minor clean code.
>>     - Update the commiti's title.
>> ---
>>   fs/erofs/super.c | 54 +++++++++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 46 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index cadec6b1b554..28b3701165cc 100644
>> --- a/fs/erofs/super.c
>> +++ b/fs/erofs/super.c
>> @@ -511,24 +511,62 @@ static int erofs_fc_parse_param(struct 
>> fs_context *fc,
>>       return 0;
>>   }
>> -static struct inode *erofs_nfs_get_inode(struct super_block *sb,
>> -                     u64 ino, u32 generation)
>> +static int erofs_encode_fh(struct inode *inode, u32 *fh, int *max_len,
>> +               struct inode *parent)
>>   {
>> -    return erofs_iget(sb, ino);
>> +    int len = parent ? 6 : 3;
>> +    erofs_nid_t nid;
> 
> Just `erofs_nid_t nid = EROFS_I(inode)->nid;`?
> 
> I think the compiler will optimize out `if (*max_len < len)`.
> 
>> +    u32 generation;
> 
> It seems it's unnecessary to introduce `generation` variable here?
> 
>> +
>> +    if (*max_len < len) {
>> +        *max_len = len;
>> +        return FILEID_INVALID;
>> +    }
>> +
>> +    nid = EROFS_I(inode)->nid;
>> +    generation = inode->i_generation;
> 
> So drop these two lines.
> 
>> +    fh[0] = (u32)(nid >> 32);
>> +    fh[1] = (u32)(nid & 0xffffffff);
>> +    fh[2] = generation;
>> +
>> +    if (parent) {
>> +        nid = EROFS_I(parent)->nid;
>> +        generation = parent->i_generation;
>> +
>> +        fh[3] = (u32)(nid >> 32);
>> +        fh[4] = (u32)(nid & 0xffffffff);
>> +        fh[5] = generation;
> 
>          fh[5] = parent->i_generation;
> 
>> +    }
>> +
>> +    *max_len = len;
>> +    return parent ? FILEID_INO64_GEN_PARENT : FILEID_INO64_GEN;
>>   }
>>   static struct dentry *erofs_fh_to_dentry(struct super_block *sb,
>>           struct fid *fid, int fh_len, int fh_type)
>>   {
>> -    return generic_fh_to_dentry(sb, fid, fh_len, fh_type,
>> -                    erofs_nfs_get_inode);
>> +    erofs_nid_t nid;
>> +
>> +    if ((fh_type != FILEID_INO64_GEN &&
>> +         fh_type != FILEID_INO64_GEN_PARENT) || fh_len < 3)
>> +        return NULL;
>> +
>> +    nid = (u64) fid->raw[0] << 32;
>> +    nid |= (u64) fid->raw[1];
> 
> Unnecessary nid variable.
> 
>> +    return d_obtain_alias(erofs_iget(sb, nid));
> 
>      return d_obtain_alias(erofs_iget(sb,
>              ((u64)fid->raw[0] << 32) | fid->raw[1]));
> 
>>   }
>>   static struct dentry *erofs_fh_to_parent(struct super_block *sb,
>>           struct fid *fid, int fh_len, int fh_type)
>>   {
>> -    return generic_fh_to_parent(sb, fid, fh_len, fh_type,
>> -                    erofs_nfs_get_inode);
>> +    erofs_nid_t nid;
>> +
>> +    if (fh_type != FILEID_INO64_GEN_PARENT || fh_len < 6)
>> +        return NULL;
>> +
>> +    nid = (u64) fid->raw[3] << 32;
>> +    nid |= (u64) fid->raw[4];
> 
> Same here.
> 
> Thanks,
> Gao Xiang