linux-ext4 - Re: [PATCH] ext4: add ext4_sb_bread() to disambiguate ENOMEM cases

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20181122122155.GN9840@quack2.suse.cz>
Date:   Thu, 22 Nov 2018 13:21:55 +0100
From:   Jan Kara <jack@...e.cz>
To:     Theodore Ts'o <tytso@....edu>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>,
        stable@...nel.org
Subject: Re: [PATCH] ext4: add ext4_sb_bread() to disambiguate ENOMEM cases

On Sat 17-11-18 18:35:23, Theodore Ts'o wrote:
> Today, when sb_bread() returns NULL, this can either be because of an
> I/O error or because the system failed to allocate the buffer.  Since
> it's an old interface, changing would require changing many call
> sites.
> 
> So instead we create our own ext4_sb_bread(), which also allows us to
> set the REQ_META flag.
> 
> Also fixed a problem in the xattr code where a NULL return in a
> function could also mean that the xattr was not found, which could
> lead to the wrong error getting returned to userspace.
> 
> Fixes: ac27a0ec112a ("ext4: initial copy of files from ext3")
> Cc: stable@...nel.org
> Signed-off-by: Theodore Ts'o <tytso@....edu>

...

> +/*
> + * This works like sb_bread() except it uses ERR_PTR for error
> + * returns.  Currently with sb_bread it's impossible to distinguish
> + * between ENOMEM and EIO situations (since both result in a NULL
> + * return.
> + */
> +struct buffer_head *
> +ext4_sb_bread(struct super_block *sb, sector_t block)
> +{
> +	struct buffer_head *bh = sb_getblk(sb, block);
> +
> +	if (bh == NULL)
> +		return ERR_PTR(-ENOMEM);
> +	if (buffer_uptodate(bh))
> +		return bh;
> +	ll_rw_block(REQ_OP_READ, REQ_META | REQ_PRIO, 1, &bh);

Is there a reason for REQ_PRIO? I'm not sure all REQ_META reads are really
a priority ones...

> +	wait_on_buffer(bh);
> +	if (buffer_uptodate(bh))
> +		return bh;
> +	put_bh(bh);
> +	return ERR_PTR(-EIO);
> +}
> +
>  static int ext4_verify_csum_type(struct super_block *sb,
>  				 struct ext4_super_block *es)
>  {

...

> @@ -696,26 +695,23 @@ ext4_xattr_block_list(struct dentry *dentry, char *buffer, size_t buffer_size)
>  	ea_idebug(inode, "buffer=%p, buffer_size=%ld",
>  		  buffer, (long)buffer_size);
>  
> -	error = 0;
>  	if (!EXT4_I(inode)->i_file_acl)
> -		goto cleanup;
> +		return NULL;

NULL looks wrong here - should be 0 I guess...

Otherwise the patch looks good to me.

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