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: <4A02FC3A.9000806@redhat.com>
Date:	Thu, 07 May 2009 10:20:26 -0500
From:	Eric Sandeen <sandeen@...hat.com>
To:	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>
CC:	cmm@...ibm.com, tytso@....edu, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 1/3] ext4: Properly initialize the buffer_head state

Aneesh Kumar K.V wrote:
> These buffer_heads are allocated on stack and are
> used only to make get_blocks calls. So we can set the
> b_state to 0
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@...ux.vnet.ibm.com>

I'd noticed this too, thanks for fixing up.

> ---
>  fs/ext4/extents.c |    1 +
>  fs/ext4/inode.c   |    2 +-
>  fs/mpage.c        |    2 +-
>  3 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index e963870..10b3028 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3141,6 +3141,7 @@ long ext4_fallocate(struct inode *inode, int mode, loff_t offset, loff_t len)
>  			ret = PTR_ERR(handle);
>  			break;
>  		}
> +		map_bh.b_state = 0;
>  		ret = ext4_get_blocks_wrap(handle, inode, block,
>  					  max_blocks, &map_bh,
>  					  EXT4_CREATE_UNINITIALIZED_EXT, 0, 0);
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 43884e3..c3cd00f 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2104,7 +2104,7 @@ static int mpage_da_map_blocks(struct mpage_da_data *mpd)
>  	if ((mpd->b_state  & (1 << BH_Mapped)) &&
>  	    !(mpd->b_state & (1 << BH_Delay)))
>  		return 0;
> -	new.b_state = mpd->b_state;
> +	new.b_state = 0;

hm can you explain why we want 0 rather than mpd->b_state?  The others
are obvious, b_state was largely uninitialized, but this is changing
what looked like a different intentional initialization.  Can you update
the changelog to say why it's wrong?

While we're at it could we name this something other than "new?"

If it's a mapping bh, maybe "map_bh" like normal? :)

>  	new.b_blocknr = 0;
>  	new.b_size = mpd->b_size;
>  	next = mpd->b_blocknr;
> diff --git a/fs/mpage.c b/fs/mpage.c
> index 680ba60..cd98409 100644
> --- a/fs/mpage.c
> +++ b/fs/mpage.c
> @@ -412,7 +412,7 @@ int mpage_readpage(struct page *page, get_block_t get_block)
>  	struct buffer_head map_bh;
>  	unsigned long first_logical_block = 0;
>  
> -	clear_buffer_mapped(&map_bh);
> +	map_bh.b_state = 0;
>  	bio = do_mpage_readpage(bio, page, 1, &last_block_in_bio,
>  			&map_bh, &first_logical_block, get_block);
>  	if (bio)

the rest looks good to me; there are places in the core kernel that
don't initialize state and just clear flags they "know" they'll care
about... it always struck me as messy, clearing state is much much better.

-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ