linux-ext4 - Re: [RFC PATCH] ext4: don't split xattr inode refcounts across i_ctime and i

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: <1515524731.3571.16.camel@kernel.org>
Date:   Tue, 09 Jan 2018 14:05:31 -0500
From:   Jeff Layton <jlayton@...nel.org>
To:     "Darrick J. Wong" <darrick.wong@...cle.com>, tahsin@...gle.com
Cc:     adilger.kernel@...ger.ca, tytso@....edu,
        linux-ext4@...r.kernel.org, dhowells@...hat.com
Subject: Re: [RFC PATCH] ext4: don't split xattr inode refcounts across
 i_ctime and i_version fields

On Tue, 2018-01-09 at 10:52 -0800, Darrick J. Wong wrote:
> On Tue, Jan 09, 2018 at 09:47:01AM -0500, Jeff Layton wrote:
> > From: Jeff Layton <jlayton@...hat.com>
> > 
> > This patch is based on top of the i_version rework that I'm flogging
> > upstream. It's just a cleanup of some nastiness I noticed while in there.
> > 
> > This code uses the i_ctime and i_version fields to each store half of
> > a refcount. I suspect it was done this way long ago when the i_version
> 
> Way long ago == 2017-06-22 :)
> 

(cc'ing Tahsin)

Hah, ok. I was thinking this was legacy code, but I guess not! I should
have probably done some git archaeology first.

> The new 64k xattr value feature in ext4 uses hidden inodes to store attr
> values that don't fit in a single block.  Since the inode can be shared
> by multiple xattr keys and isn't exported via NFS (I hope...), they use
> a 64-bit refcount mashed into i_ctime and i_version.  That's what this
> gobbledygook is for.
> 
> https://ext4.wiki.kernel.org/index.php/Ext4_Disk_Layout#Large_Extended_Attribute_Values
> 
> --D
> 

Ahh, many thanks...

My main question was: why split the refcount across fields like this? If
it's necessary now for backward compatibility then so be it, but it's
weird and not 100% clear why it's being done that way.


> > field was a 32 bits, and was never changed when the field was converted
> > to a 64 bit value.
> > 
> > Change the code to just store the refcount in the i_version field rather
> > than splitting it across both fields.
> > 
> > Cc: David Howells <dhowells@...hat.com>
> > Signed-off-by: Jeff Layton <jlayton@...hat.com>
> > ---
> >  fs/ext4/xattr.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> > index 63656dbafdc4..6ea78dd367ca 100644
> > --- a/fs/ext4/xattr.c
> > +++ b/fs/ext4/xattr.c
> > @@ -294,14 +294,12 @@ ext4_xattr_inode_hash(struct ext4_sb_info *sbi, const void *buffer, size_t size)
> >  
> >  static u64 ext4_xattr_inode_get_ref(struct inode *ea_inode)
> >  {
> > -	return ((u64)ea_inode->i_ctime.tv_sec << 32) |
> > -		(u32) inode_peek_iversion_raw(ea_inode);
> > +	return inode_peek_iversion_raw(ea_inode);
> >  }
> >  
> >  static void ext4_xattr_inode_set_ref(struct inode *ea_inode, u64 ref_count)
> >  {
> > -	ea_inode->i_ctime.tv_sec = (u32)(ref_count >> 32);
> > -	inode_set_iversion_raw(ea_inode, ref_count & 0xffffffff);
> > +	inode_set_iversion_raw(ea_inode, ref_count);
> >  }
> >  
> >  static u32 ext4_xattr_inode_get_hash(struct inode *ea_inode)
> > -- 
> > 2.14.3
> > 

-- 
Jeff Layton <jlayton@...nel.org>