linux-kernel - Re: [PATCH 1/3] befs: remove off argument of befs_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: <20160727233708.GA3942@pc>
Date:	Thu, 28 Jul 2016 00:37:08 +0100
From:	Salah Triki <salah.triki@...il.com>
To:	Luis de Bethencourt <luisbg@....samsung.com>
Cc:	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
	viro@...iv.linux.org.uk, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 1/3] befs: remove off argument of befs_read_datastream

On Tue, Jul 12, 2016 at 12:02:48AM +0100, Luis de Bethencourt wrote:
> befs_read_datastream() is used to read the inode from the disk, off is
> meant to provide the offset of the data in the buffer head. But the only
> function using this argument already knows the starting offset of the node,
> so this argument isn't needed.
> 
> Signed-off-by: Luis de Bethencourt <luisbg@....samsung.com>
> ---
> Hi,
> 
> I know we are in release candidate 7 and maintainers are busy with important
> bugs and regressions. Just sending this now so it is in the queue when the
> merge window opens in two weeks.
> 
> befs_bt_read_node() is the only case where befs_read_datastream() was called
> with an off pointer, the rest had NULL.
> 
> befs_read_datastream() effectively did:
> block = pos >> BEFS_SB(sb)->block_shift;
> *off = pos - (block << BEFS_SB(sb)->block_shift);
> 
> Since we only use it for inodes, pos above is either 0 or 1204, the node size
> in BeFS by design. That shifted makes block equal 0. So off always ends up
> being the same as pos. We can use this directly in befs_bt_read_node().
> 
> Thank for the reviews,
> Luis
> 
>  fs/befs/btree.c      |  8 +++-----
>  fs/befs/datastream.c | 10 +++-------
>  fs/befs/datastream.h |  2 +-
>  3 files changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/befs/btree.c b/fs/befs/btree.c
> index 307645f9..3995d58 100644
> --- a/fs/befs/btree.c
> +++ b/fs/befs/btree.c
> @@ -142,7 +142,7 @@ befs_bt_read_super(struct super_block *sb, const befs_data_stream *ds,
>  
>  	befs_debug(sb, "---> %s", __func__);
>  
> -	bh = befs_read_datastream(sb, ds, 0, NULL);
> +	bh = befs_read_datastream(sb, ds, 0);
>  
>  	if (!bh) {
>  		befs_error(sb, "Couldn't read index header.");
> @@ -196,14 +196,12 @@ static int
>  befs_bt_read_node(struct super_block *sb, const befs_data_stream *ds,
>  		  struct befs_btree_node *node, befs_off_t node_off)
>  {
> -	uint off = 0;
> -
>  	befs_debug(sb, "---> %s", __func__);
>  
>  	if (node->bh)
>  		brelse(node->bh);
>  
> -	node->bh = befs_read_datastream(sb, ds, node_off, &off);
> +	node->bh = befs_read_datastream(sb, ds, node_off);
>  	if (!node->bh) {
>  		befs_error(sb, "%s failed to read "
>  			   "node at %llu", __func__, node_off);
> @@ -212,7 +210,7 @@ befs_bt_read_node(struct super_block *sb, const befs_data_stream *ds,
>  		return BEFS_ERR;
>  	}
>  	node->od_node =
> -	    (befs_btree_nodehead *) ((void *) node->bh->b_data + off);
> +	    (befs_btree_nodehead *) ((void *) node->bh->b_data + node_off);
>  
>  	befs_dump_index_node(sb, node->od_node);
>  
> diff --git a/fs/befs/datastream.c b/fs/befs/datastream.c
> index 26cc417..3c14c84 100644
> --- a/fs/befs/datastream.c
> +++ b/fs/befs/datastream.c
> @@ -39,14 +39,12 @@ static int befs_find_brun_dblindirect(struct super_block *sb,
>   * @sb: Filesystem superblock
>   * @ds: datastrem to find data with
>   * @pos: start of data
> - * @off: offset of data in buffer_head->b_data
>   *
> - * Returns pointer to buffer_head containing data starting with offset @off,
> - * if you don't need to know offset just set @off = NULL.
> + * Returns pointer to buffer_head containing data starting from pos.
>   */
>  struct buffer_head *
>  befs_read_datastream(struct super_block *sb, const befs_data_stream *ds,
> -		     befs_off_t pos, uint * off)
> +		     befs_off_t pos)
>  {
>  	struct buffer_head *bh;
>  	befs_block_run run;
> @@ -54,8 +52,6 @@ befs_read_datastream(struct super_block *sb, const befs_data_stream *ds,
>  
>  	befs_debug(sb, "---> %s %llu", __func__, pos);
>  	block = pos >> BEFS_SB(sb)->block_shift;
> -	if (off)
> -		*off = pos - (block << BEFS_SB(sb)->block_shift);
>  
>  	if (befs_fblock2brun(sb, ds, block, &run) != BEFS_OK) {
>  		befs_error(sb, "BeFS: Error finding disk addr of block %lu",
> @@ -131,7 +127,7 @@ befs_read_lsymlink(struct super_block *sb, const befs_data_stream *ds,
>  	befs_debug(sb, "---> %s length: %llu", __func__, len);
>  
>  	while (bytes_read < len) {
> -		bh = befs_read_datastream(sb, ds, bytes_read, NULL);
> +		bh = befs_read_datastream(sb, ds, bytes_read);
>  		if (!bh) {
>  			befs_error(sb, "BeFS: Error reading datastream block "
>  				   "starting from %llu", bytes_read);
> diff --git a/fs/befs/datastream.h b/fs/befs/datastream.h
> index 91ba820..76e1ab5 100644
> --- a/fs/befs/datastream.h
> +++ b/fs/befs/datastream.h
> @@ -5,7 +5,7 @@
>  
>  struct buffer_head *befs_read_datastream(struct super_block *sb,
>  					 const befs_data_stream *ds,
> -					 befs_off_t pos, uint * off);
> +					 befs_off_t pos);
>  
>  int befs_fblock2brun(struct super_block *sb, const befs_data_stream *data,
>  		     befs_blocknr_t fblock, befs_block_run * run);
> -- 
> 2.5.3
> 

[...]
> Since we only use it for inodes, pos above is either 0 or 1204, the node size
> in BeFS by design. 

pos is the byte address that corresponds to a FS block number

> block = pos >> BEFS_SB(sb)->block_shift;
> *off = pos - (block << BEFS_SB(sb)->block_shift);

the result of the second shift is equal to pos, so *off is always set to
zero.

Am I wrong ?

best regards
--
salah