linux-kernel - Re: [PATCH -next V2] erofs: code clean up for function erofs_read

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: <89069fa4-7347-4364-8793-1ce705a00b92@huawei.com>
Date:   Thu, 9 Nov 2023 21:45:49 +0800
From:   Zizhi Wo <wozizhi@...wei.com>
To:     Gao Xiang <hsiangkao@...ux.alibaba.com>, <xiang@...nel.org>,
        <chao@...nel.org>
CC:     <linux-erofs@...ts.ozlabs.org>, <linux-kernel@...r.kernel.org>,
        <yangerkun@...wei.com>
Subject: Re: [PATCH -next V2] erofs: code clean up for function
 erofs_read_inode()



在 2023/11/9 21:14, Gao Xiang 写道:
> Hi,
> 
> On 2023/11/10 03:48, WoZ1zh1 wrote:
>> Because variables "die" and "copied" only appear in case
>> EROFS_INODE_LAYOUT_EXTENDED, move them from the outer space into this
>> case. Also, call "kfree(copied)" earlier to avoid double free in the
>> "error_out" branch. Some cleanups, no logic changes.
>>
>> Signed-off-by: WoZ1zh1 <wozizhi@...wei.com>
> 
> Please help use your real name here...
> 
>> ---
>>   fs/erofs/inode.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
>> index b8ad05b4509d..a388c93eec34 100644
>> --- a/fs/erofs/inode.c
>> +++ b/fs/erofs/inode.c
>> @@ -19,7 +19,6 @@ static void *erofs_read_inode(struct erofs_buf *buf,
>>       erofs_blk_t blkaddr, nblks = 0;
>>       void *kaddr;
>>       struct erofs_inode_compact *dic;
>> -    struct erofs_inode_extended *die, *copied = NULL;
>>       unsigned int ifmt;
>>       int err;
>> @@ -53,6 +52,8 @@ static void *erofs_read_inode(struct erofs_buf *buf,
>>       switch (erofs_inode_version(ifmt)) {
>>       case EROFS_INODE_LAYOUT_EXTENDED:
>> +        struct erofs_inode_extended *die, *copied = NULL;
> 
> Thanks for the patch, but in my own opinion:
> 
> 1) It doesn't simplify the code
OK, I'm sorry for the noise(;´༎ຶД༎ຶ`)
> 
> 2) We'd like to avoid defining variables like this (in the
>     switch block), and I even don't think this patch can compile.
I tested this patch with gcc-12.2.1 locally and it compiled
successfully. I'm not sure if this patch will fail in other environment
with different compiler...

> 3) The logic itself is also broken...

Sorry, but I just don't understand why the logic itself is broken, and
can you please explain more?

Thanks,
Zizhi Wo

> Thanks,
> Gao Xiang
> 
>> +
>>           vi->inode_isize = sizeof(struct erofs_inode_extended);
>>           /* check if the extended inode acrosses block boundary */
>>           if (*ofs + vi->inode_isize <= sb->s_blocksize) {
>> @@ -98,6 +99,7 @@ static void *erofs_read_inode(struct erofs_buf *buf,
>>               inode->i_rdev = 0;
>>               break;
>>           default:
>> +            kfree(copied);
>>               goto bogusimode;
>>           }
>>           i_uid_write(inode, le32_to_cpu(die->i_uid));
>> @@ -117,7 +119,6 @@ static void *erofs_read_inode(struct erofs_buf *buf,
>>               /* fill chunked inode summary info */
>>               vi->chunkformat = le16_to_cpu(die->i_u.c.format);
>>           kfree(copied);
>> -        copied = NULL;
>>           break;
>>       case EROFS_INODE_LAYOUT_COMPACT:
>>           vi->inode_isize = sizeof(struct erofs_inode_compact);
>> @@ -197,7 +198,6 @@ static void *erofs_read_inode(struct erofs_buf *buf,
>>       err = -EFSCORRUPTED;
>>   err_out:
>>       DBG_BUGON(1);
>> -    kfree(copied);
>>       erofs_put_metabuf(buf);
>>       return ERR_PTR(err);
>>   }
>