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]

Date:   Thu, 9 Nov 2023 21:14:18 +0800
From:   Gao Xiang <hsiangkao@...ux.alibaba.com>
To:     WoZ1zh1 <wozizhi@...wei.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()

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

2) We'd like to avoid defining variables like this (in the
    switch block), and I even don't think this patch can compile.

3) The logic itself is also broken...

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);
>   }