Fix ea-in-inode default ACL creation

Fix ea-in-inode default ACL creation

[Andreas Gruenbacher]

Hello,

here is another nastiness.

When a new inode is created, ext3_new_inode sets the EXT3_STATE_NEW
flag, which tells ext3_do_update_inode to zero out the inode before
filling in the inode's data. When a file is created in a directory with
a default acl, the new inode inherits the directory's default acl; this
generates attributes. The attributes are created before
ext3_do_update_inode is called to write out the inode. In case of
in-inode attributes, the new inode's attributes are written, and then
zeroed out again by ext3_do_update_inode. Bad thing.

Fix this by recognizing the EXT3_STATE_NEW case in
ext3_xattr_set_handle, and zeroing out the inode there already when
necessary.

Signed-off-by: Andreas Gruenbacher <agruen@suse.de>

Index: linux-2.6.11-latest/fs/ext3/xattr.c
===================================================================
--- linux-2.6.11-latest.orig/fs/ext3/xattr.c
+++ linux-2.6.11-latest/fs/ext3/xattr.c
@@ -954,6 +954,13 @@ ext3_xattr_set_handle(handle_t *handle, 
 	error = ext3_get_inode_loc(inode, &is.iloc);
 	if (error)
 		goto cleanup;
+
+	if (EXT3_I(inode)->i_state & EXT3_STATE_NEW) {
+		struct ext3_inode *raw_inode = ext3_raw_inode(&is.iloc);
+		memset(raw_inode, 0, EXT3_SB(inode->i_sb)->s_inode_size);
+		EXT3_I(inode)->i_state &= ~EXT3_STATE_NEW;
+	}
+
 	error = ext3_xattr_ibody_find(inode, &i, &is);
 	if (error)
 		goto cleanup;


-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX GMBH

Re: Fix ea-in-inode default ACL creation

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1800 bytes --]

On Thu, 20 Jan 2005 19:22:25 +0100, Andreas Gruenbacher said:

> When a new inode is created, ext3_new_inode sets the EXT3_STATE_NEW
> flag, which tells ext3_do_update_inode to zero out the inode before
> filling in the inode's data. When a file is created in a directory with
> a default acl, the new inode inherits the directory's default acl; this
> generates attributes. The attributes are created before
> ext3_do_update_inode is called to write out the inode. In case of
> in-inode attributes, the new inode's attributes are written, and then
> zeroed out again by ext3_do_update_inode. Bad thing.
> 
> Fix this by recognizing the EXT3_STATE_NEW case in
> ext3_xattr_set_handle, and zeroing out the inode there already when
> necessary.
> 
> Signed-off-by: Andreas Gruenbacher <agruen@suse.de>
> 
> Index: linux-2.6.11-latest/fs/ext3/xattr.c
> ===================================================================
> --- linux-2.6.11-latest.orig/fs/ext3/xattr.c
> +++ linux-2.6.11-latest/fs/ext3/xattr.c
> @@ -954,6 +954,13 @@ ext3_xattr_set_handle(handle_t *handle, 
>  	error = ext3_get_inode_loc(inode, &is.iloc);
>  	if (error)
>  		goto cleanup;
> +
> +	if (EXT3_I(inode)->i_state & EXT3_STATE_NEW) {
> +		struct ext3_inode *raw_inode = ext3_raw_inode(&is.iloc);
> +		memset(raw_inode, 0, EXT3_SB(inode->i_sb)->s_inode_size);
> +		EXT3_I(inode)->i_state &= ~EXT3_STATE_NEW;
> +	}
> +
>  	error = ext3_xattr_ibody_find(inode, &i, &is);
>  	if (error)
>  		goto cleanup;

Maybe I'm a total idiot, but I'm failing to see how adding *another* zero
operation (although quite likely needed at that point) is going to help the
fact that we zero something out after we've stored data we want to keep in it.
Is there a missing hunk that *removes* the too-late memset-to-zero in
ext3_do_update_inode?


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: Fix ea-in-inode default ACL creation

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1244 bytes --]

On Jan 20, 2005  13:56 -0500, Valdis.Kletnieks@vt.edu wrote:
> On Thu, 20 Jan 2005 19:22:25 +0100, Andreas Gruenbacher said:
> > ===================================================================
> > --- linux-2.6.11-latest.orig/fs/ext3/xattr.c
> > +++ linux-2.6.11-latest/fs/ext3/xattr.c
> > @@ -954,6 +954,13 @@ ext3_xattr_set_handle(handle_t *handle, 
> > +	if (EXT3_I(inode)->i_state & EXT3_STATE_NEW) {
> > +		struct ext3_inode *raw_inode = ext3_raw_inode(&is.iloc);
> > +		memset(raw_inode, 0, EXT3_SB(inode->i_sb)->s_inode_size);
> > +		EXT3_I(inode)->i_state &= ~EXT3_STATE_NEW;
> > +	}
> 
> Maybe I'm a total idiot, but I'm failing to see how adding *another* zero
> operation (although quite likely needed at that point) is going to help the
> fact that we zero something out after we've stored data we want to keep in it.
> Is there a missing hunk that *removes* the too-late memset-to-zero in
> ext3_do_update_inode?

Yes, as you can see above the EXT3_STATE_NEW flag is cleared so the later
check in ext3_new_inode() will not again zero the inode

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://members.shaw.ca/adilger/             http://members.shaw.ca/golinux/


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Fix ea-in-inode default ACL creation

[Andreas Gruenbacher]

On Thu, 2005-01-20 at 19:56, Valdis.Kletnieks@vt.edu wrote:
> [...] I'm failing to see how adding *another* zero operation [...] is going to help the
> fact [...]

It's an ancient kernel hackers trick:  ;)
> +		EXT3_I(inode)->i_state &= ~EXT3_STATE_NEW;

Regards,
-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX GMBH

Re: Fix ea-in-inode default ACL creation

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 548 bytes --]

On Thu, 20 Jan 2005 20:09:24 +0100, Andreas Gruenbacher said:
> On Thu, 2005-01-20 at 19:56, Valdis.Kletnieks@vt.edu wrote:
> > [...] I'm failing to see how adding *another* zero operation [...] is going to help the
> > fact [...]
> 
> It's an ancient kernel hackers trick:  ;)
> > +		EXT3_I(inode)->i_state &= ~EXT3_STATE_NEW;

Damn. I saw every *other* line of the patch but that one. Literally.

(Make note to self - if a cold is making your eyes water and itch too
much to wear your contacts, you probably can't see well enough to read code ;)

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: Fix ea-in-inode default ACL creation

[Stephen C. Tweedie]

Hi,

On Thu, 2005-01-20 at 18:22, Andreas Gruenbacher wrote:

> When a new inode is created, ext3_new_inode sets the EXT3_STATE_NEW
> flag, which tells ext3_do_update_inode to zero out the inode before
> filling in the inode's data. When a file is created in a directory with
> a default acl, the new inode inherits the directory's default acl; this
> generates attributes. The attributes are created before
> ext3_do_update_inode is called to write out the inode. In case of
> in-inode attributes, the new inode's attributes are written, and then
> zeroed out again by ext3_do_update_inode. Bad thing.
> 
> Fix this by recognizing the EXT3_STATE_NEW case in
> ext3_xattr_set_handle, and zeroing out the inode there already when
> necessary.

Ugh.  It feels horrible to have to do this, but we _do_ want to clear
the raw inode, and we only want to do it once, and we have to do it on
first access to the on-disk structures.  I can't see an easy way round
it that doesn't add more overhead.

Looks reasonable to me.

--Stephen

Re: Fix ea-in-inode default ACL creation

[Andreas Gruenbacher]

On Fri, 2005-01-21 at 22:36, Stephen C. Tweedie wrote:
> Ugh.  It feels horrible to have to do this, but we _do_ want to clear
> the raw inode, and we only want to do it once, and we have to do it on
> first access to the on-disk structures.  I can't see an easy way round
> it that doesn't add more overhead.

Well, we could split EXT3_STATE_NEW into a "GOOD_OLD_NEW" flag for the
first 128 bytes and a "BIG_INODE_NEW" flag for the rest, and only
initialize the rest in the xattr code when necessary. Not any better it
I suppose. Note that this change has no effect except with default ACLs
anyway.

-- Andreas.

Re: Fix ea-in-inode default ACL creation

[Stephen C. Tweedie]

Hi,

On Fri, 2005-01-21 at 21:48, Andreas Gruenbacher wrote:

> Well, we could split EXT3_STATE_NEW into a "GOOD_OLD_NEW" flag for the
> first 128 bytes and a "BIG_INODE_NEW" flag for the rest, and only
> initialize the rest in the xattr code when necessary. Not any better it
> I suppose. 

Agreed. :-)

--Stephen