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  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]
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

Powered by blists - more mailing lists