linux-kernel - Re: [PATCH] fs: ext4: inode->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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <36EC19C1-8C8C-4B95-B263-8684BEDCE591@fb.com>
Date:   Thu, 29 Jun 2017 00:58:17 +0000
From:   William Koh <kkc6196@...com>
To:     "Darrick J. Wong" <darrick.wong@...cle.com>
CC:     "tytso@....edu" <tytso@....edu>,
        "adilger.kernel@...ger.ca" <adilger.kernel@...ger.ca>,
        "linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH] fs: ext4: inode->i_generation not assigned 0.

On 6/28/17, 5:48 PM, "Darrick J. Wong" <darrick.wong@...cle.com> wrote:

    On Wed, Jun 28, 2017 at 03:06:42PM -0700, Kyungchan Koh wrote:
    > In fs/ext4/super.c, the function ext4_nfs_get_inode takes as input
    > "generation" that can be used to specify the generation of the inode to
    > be returned. When 0 is given as input, then inodes of any generation can
    > be returned. Therefore, generation 0 is a special case that should be
    > avoided when assigning generation to inodes.
    > 
    > A new inline function, ext4_inode_set_gen, will take care of the
    > problem.  Now, inodes cannot have a generation of 0, so this patch fixes
    > the issue.
    
    Forgive my ignorance, but why is generation == 0 a special case?
    
    From a quick scan of the code it seems that filesystems hand out
    handles to NFS with parent_{ino,gen} set (or zeroed).  That implies that
    we have to check ino/gen for zeroes and garbage, but I don't see why
    you'd exempt gen == 0 from checking?
    
    (Really what I'm fishing for is whether or not there's some precedent
    for this that I don't know about.)
    
    --D
    
    > 
    > Signed-off-by: Kyungchan Koh <kkc6196@...com>
    > ---
    >  fs/ext4/ext4.h   | 8 ++++++++
    >  fs/ext4/ialloc.c | 2 +-
    >  fs/ext4/ioctl.c  | 4 ++--
    >  3 files changed, 11 insertions(+), 3 deletions(-)
    > 
    > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
    > index 3219154..74c6677 100644
    > --- a/fs/ext4/ext4.h
    > +++ b/fs/ext4/ext4.h
    > @@ -1549,6 +1549,14 @@ static inline int ext4_valid_inum(struct super_block *sb, unsigned long ino)
    >  		 ino <= le32_to_cpu(EXT4_SB(sb)->s_es->s_inodes_count));
    >  }
    >  
    > +static inline void ext4_inode_set_gen(struct inode *inode,
    > +				      struct ext4_sb_info *sbi)
    > +{
    > +	inode->i_generation = sbi->s_next_generation++;
    > +	if (!inode->i_generation)
    > +		inode->i_generation = sbi->s_next_generation++;
    > +}
    > +
    >  /*
    >   * Inode dynamic state flags
    >   */
    > diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
    > index 98ac2f1..d33f6f0 100644
    > --- a/fs/ext4/ialloc.c
    > +++ b/fs/ext4/ialloc.c
    > @@ -1072,7 +1072,7 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
    >  		goto out;
    >  	}
    >  	spin_lock(&sbi->s_next_gen_lock);
    > -	inode->i_generation = sbi->s_next_generation++;
    > +	ext4_inode_set_gen(inode, sbi);
    >  	spin_unlock(&sbi->s_next_gen_lock);
    >  
    >  	/* Precompute checksum seed for inode metadata */
    > diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
    > index 0c21e22..d52a467 100644
    > --- a/fs/ext4/ioctl.c
    > +++ b/fs/ext4/ioctl.c
    > @@ -160,8 +160,8 @@ static long swap_inode_boot_loader(struct super_block *sb,
    >  	inode->i_ctime = inode_bl->i_ctime = current_time(inode);
    >  
    >  	spin_lock(&sbi->s_next_gen_lock);
    > -	inode->i_generation = sbi->s_next_generation++;
    > -	inode_bl->i_generation = sbi->s_next_generation++;
    > +	ext4_inode_set_gen(inode, sbi);
    > +	ext4_inode_set_gen(inode_bl, sbi);
    >  	spin_unlock(&sbi->s_next_gen_lock);
    >  
    >  	ext4_discard_preallocations(inode);
    > -- 
    > 2.9.3
    > 
     

Generation == 0 seems to be a special case for many filesystems, not just ext4. For jfs, in jfs_nfs_get_inode, if the input generation is 0, then no inodes are returned. Such filesystems that seem to treat generation 0 as a special case nfs_get_inode that I have found so far are ext2, ext4, jfs,  exofs, and f2fs. Therefore, I was actually thinking about implementing a shared helper in linux/fs.h that has the prototype “static inline void inode_set_gen(struct inode *inode, unsigned int *generation)” that can be used for all filesystems. For example, for jfs, I can do “inode_set_gen(inode, &JFS_SBI(sb)->gengen);” or for extX, I can do “inode_set_gen(inode, &EXTX_SB(sb)->s_next_generation);”. This allows a cleaner change of adding a few lines of code to linux/fs.h and replacing one to a few lines for each filesystem. I am open to both options, if anyone has a strong preference for either option.

Best,
Kyungchan Koh