linux-ext4 - Re: [PATCH] ext2: propagate errors up to ext2_find

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] [day] [month] [year] [list]

Message-ID: <20200602091351.GD19165@quack2.suse.cz>
Date:   Tue, 2 Jun 2020 11:13:51 +0200
From:   Jan Kara <jack@...e.cz>
To:     "zhangyi (F)" <yi.zhang@...wei.com>
Cc:     linux-ext4@...r.kernel.org, jack@...e.cz
Subject: Re: [PATCH] ext2: propagate errors up to ext2_find_entry()'s callers

On Mon 01-06-20 21:42:22, zhangyi (F) wrote:
> The same to commit <36de928641ee4> (ext4: propagate errors up to
> ext4_find_entry()'s callers') in ext4, also return error instead of NULL
> pointer in case of some error happens in ext2_find_entry() (e.g. -ENOMEM
> or -EIO). This could avoid a negative dentry cache entry installed even
> it failed to read directory block due to IO error.
> 
> Signed-off-by: zhangyi (F) <yi.zhang@...wei.com>
> ---
>  fs/ext2/dir.c   | 62 +++++++++++++++++++++++++------------------------
>  fs/ext2/ext2.h  |  3 ++-
>  fs/ext2/namei.c | 28 ++++++++++++++++++----
>  3 files changed, 58 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/ext2/dir.c b/fs/ext2/dir.c
> index 13318e255ebf..1c3ab60890b1 100644
> --- a/fs/ext2/dir.c
> +++ b/fs/ext2/dir.c
> @@ -347,8 +347,7 @@ struct ext2_dir_entry_2 *ext2_find_entry (struct inode *dir,
>  	unsigned long npages = dir_pages(dir);
>  	struct page *page = NULL;
>  	struct ext2_inode_info *ei = EXT2_I(dir);
> -	ext2_dirent * de;
> -	int dir_has_error = 0;
> +	ext2_dirent *de, *ret = NULL;

I don't think you need additional 'ret' variable and it does not improve
the readability of the code either... You can just use 'de' all the time.

Otherwise the patch looks good. I'd also note that all callers of
ext2_find_entry() except for ext2_inode_by_name() transform de == NULL to
-ENOENT so it would be a good followup cleanup to return -ENOENT directly
from ext2_find_entry(). Also ext2_inode_by_name() could just pass -ENOENT
further since only ext2_lookup() needs to actually transform this -ENOENT
to calling d_splice_alias() with NULL inode.

Thanks for the patch!

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR