Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1677217 > unrolled thread

[PATCH] fs: ext4: inode->i_generation not assigned 0.

Back to article view | Back to linux.kernel

Contents

  [PATCH] fs: ext4: inode->i_generation not assigned 0. Kyungchan Koh <kkc6196@fb.com> - 2017-06-29 00:10 +0200
    Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. "Darrick J. Wong" <darrick.wong@oracle.com> - 2017-06-29 02:50 +0200
      Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. William Koh <kkc6196@fb.com> - 2017-06-29 03:00 +0200
    Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. Andreas Dilger <adilger@dilger.ca> - 2017-06-29 04:40 +0200
      Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. William Koh <kkc6196@fb.com> - 2017-06-29 06:40 +0200
        Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. "Darrick J. Wong" <darrick.wong@oracle.com> - 2017-06-29 07:10 +0200
          Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. William Koh <kkc6196@fb.com> - 2017-06-29 16:30 +0200
          Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. "J. Bruce Fields" <bfields@fieldses.org> - 2017-06-29 16:40 +0200
            Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. "Darrick J. Wong" <darrick.wong@oracle.com> - 2017-06-29 19:30 +0200
              Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. "J. Bruce Fields" <bfields@fieldses.org> - 2017-06-29 20:40 +0200
                Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. "J. Bruce Fields" <bfields@fieldses.org> - 2017-06-29 21:00 +0200

#1677217 — [PATCH] fs: ext4: inode->i_generation not assigned 0.

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.

Signed-off-by: Kyungchan Koh <kkc6196@fb.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

[toc] | [next] | [standalone]

#1677298

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@fb.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
> 

[toc] | [prev] | [next] | [standalone]

#1677300

On 6/28/17, 5:48 PM, "Darrick J. Wong" <darrick.wong@oracle.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@fb.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

[toc] | [prev] | [next] | [standalone]

#1677324

[Multipart message — attachments visible in raw view] — view raw

On Jun 28, 2017, at 4:06 PM, Kyungchan Koh <kkc6196@fb.com> 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.

I'd agree with this change to avoid assigning generation == 0 to real inodes.

Also, the separate question arises about whether we need to allow file handle
lookup with generation == 0?  That allows FID guessing easily, while requiring
a non-zero generation makes that a lot harder.

What are the cases where generation == 0 are used?

> 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.
> 
> Signed-off-by: Kyungchan Koh <kkc6196@fb.com>
> 
> 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)

This should be marked "unlikely()" since it happens at most once every 4B
file creations (though likely even less since it is unlikely that so many
files will be created in a single mount).

> +		inode->i_generation = sbi->s_next_generation++;
> +}
> +
> 
> 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	}
> 	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);
> 
> 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,
> 
> 	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);
> 


Cheers, Andreas

[toc] | [prev] | [next] | [standalone]

#1677378

On 6/28/17, 7:32 PM, "Andreas Dilger" <adilger@dilger.ca> wrote:

    On Jun 28, 2017, at 4:06 PM, Kyungchan Koh <kkc6196@fb.com> 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.
    
    I'd agree with this change to avoid assigning generation == 0 to real inodes.
    
    Also, the separate question arises about whether we need to allow file handle
    lookup with generation == 0?  That allows FID guessing easily, while requiring
    a non-zero generation makes that a lot harder.
    
    What are the cases where generation == 0 are used?

Honestly, I’m not too sure. I just noticed that generation 0 was a special
case from reading the code.

    > 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.
    > 
    > Signed-off-by: Kyungchan Koh <kkc6196@fb.com>
    > 
    > 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)
    
    This should be marked "unlikely()" since it happens at most once every 4B
    file creations (though likely even less since it is unlikely that so many
    files will be created in a single mount).

Got it.
    
    > +		inode->i_generation = sbi->s_next_generation++;
    > +}
    > +
    > 
    > 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	}
    > 	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);
    > 
    > 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,
    > 
    > 	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);
    > 
    
    
    Cheers, Andreas

This is applicable to many fs, including ext2, ext4, exofs, jfs, and f2fs.
Therefore, a shared helper in linux/fs.h will allow for easy changes
in all fs. Is there any reason that might be a bad idea?

Best,
Kyungchan Koh 
    
    
    
    

[toc] | [prev] | [next] | [standalone]

#1677386

[add linux-xfs to cc]

On Thu, Jun 29, 2017 at 04:37:14AM +0000, William Koh wrote:
> On 6/28/17, 7:32 PM, "Andreas Dilger" <adilger@dilger.ca> wrote:
> 
>     On Jun 28, 2017, at 4:06 PM, Kyungchan Koh <kkc6196@fb.com> 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.
>     
>     I'd agree with this change to avoid assigning generation == 0 to real inodes.
>     
>     Also, the separate question arises about whether we need to allow file handle
>     lookup with generation == 0?  That allows FID guessing easily, while requiring
>     a non-zero generation makes that a lot harder.
>     
>     What are the cases where generation == 0 are used?
> 
> Honestly, I’m not too sure. I just noticed that generation 0 was a special
> case from reading the code.
> 
>     > 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.
>     > 
>     > Signed-off-by: Kyungchan Koh <kkc6196@fb.com>
>     > 
>     > 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)
>     
>     This should be marked "unlikely()" since it happens at most once every 4B
>     file creations (though likely even less since it is unlikely that so many
>     files will be created in a single mount).
> 
> Got it.
>     
>     > +		inode->i_generation = sbi->s_next_generation++;
>     > +}
>     > +
>     > 
>     > 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	}
>     > 	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);
>     > 
>     > 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,
>     > 
>     > 	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);
>     > 
>     
>     
>     Cheers, Andreas
> 
> This is applicable to many fs, including ext2, ext4, exofs, jfs, and f2fs.
> Therefore, a shared helper in linux/fs.h will allow for easy changes
> in all fs. Is there any reason that might be a bad idea?

AFAICT, i_generation == 0 in XFS and btrfs is just as valid as any other
number.  There is no special casing of zero in either filesystem.

So now, my curiosity intrigued, I surveyed all the Linux filesystems
that can export to NFS.  I see that there are actually quite a few fs
(ext[2-4], exofs, efs, fat, jfs, f2fs, isofs, nilfs2, reiserfs, udf,
ufs) that treat zero as a special value meaning "ignore generation
check"; others (xfs, btrfs, fuse, ntfs, ocfs2) that don't consider zero
special and always require a match; and still others (affs, befs, ceph,
gfs2, jffs2, squashfs) that don't check at all.

That to mean strongly suggests that more research is necessary to figure
out why some of the filesystems that support i_generation reserve zero
as a special value to disable generation checks and why others always
require an exact match.  Until we can recapture why things are they way
they are, it doesn't make much sense to have a helper that only applies
to half the filesystems.

Granted, the contents of a file handle are generally left up to the
individual filesystem, and the behaviors are very different, so I also
don't see that much value in hoisting i_generation updates to the VFS
level.

I guess it wouldn't really matter if XFS stopped writing i_generation =
0 onto disk, but I'm too curious about this odd difference in behavior
to let it go just yet. :)

--D

> 
> Best,
> Kyungchan Koh 
>     
>     
>     
>     
> 

[toc] | [prev] | [next] | [standalone]

#1677801

On 6/28/17, 9:59 PM, "Darrick J. Wong" <darrick.wong@oracle.com> wrote:

    [add linux-xfs to cc]
    
    On Thu, Jun 29, 2017 at 04:37:14AM +0000, William Koh wrote:
    > On 6/28/17, 7:32 PM, "Andreas Dilger" <adilger@dilger.ca> wrote:
    > 
    >     On Jun 28, 2017, at 4:06 PM, Kyungchan Koh <kkc6196@fb.com> 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.
    >     
    >     I'd agree with this change to avoid assigning generation == 0 to real inodes.
    >     
    >     Also, the separate question arises about whether we need to allow file handle
    >     lookup with generation == 0?  That allows FID guessing easily, while requiring
    >     a non-zero generation makes that a lot harder.
    >     
    >     What are the cases where generation == 0 are used?
    > 
    > Honestly, I’m not too sure. I just noticed that generation 0 was a special
    > case from reading the code.
    > 
    >     > 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.
    >     > 
    >     > Signed-off-by: Kyungchan Koh <kkc6196@fb.com>
    >     > 
    >     > 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)
    >     
    >     This should be marked "unlikely()" since it happens at most once every 4B
    >     file creations (though likely even less since it is unlikely that so many
    >     files will be created in a single mount).
    > 
    > Got it.
    >     
    >     > +		inode->i_generation = sbi->s_next_generation++;
    >     > +}
    >     > +
    >     > 
    >     > 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	}
    >     > 	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);
    >     > 
    >     > 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,
    >     > 
    >     > 	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);
    >     > 
    >     
    >     
    >     Cheers, Andreas
    > 
    > This is applicable to many fs, including ext2, ext4, exofs, jfs, and f2fs.
    > Therefore, a shared helper in linux/fs.h will allow for easy changes
    > in all fs. Is there any reason that might be a bad idea?
    
    AFAICT, i_generation == 0 in XFS and btrfs is just as valid as any other
    number.  There is no special casing of zero in either filesystem.
    
    So now, my curiosity intrigued, I surveyed all the Linux filesystems
    that can export to NFS.  I see that there are actually quite a few fs
    (ext[2-4], exofs, efs, fat, jfs, f2fs, isofs, nilfs2, reiserfs, udf,
    ufs) that treat zero as a special value meaning "ignore generation
    check"; others (xfs, btrfs, fuse, ntfs, ocfs2) that don't consider zero
    special and always require a match; and still others (affs, befs, ceph,
    gfs2, jffs2, squashfs) that don't check at all.
    
    That to mean strongly suggests that more research is necessary to figure
    out why some of the filesystems that support i_generation reserve zero
    as a special value to disable generation checks and why others always
    require an exact match.  Until we can recapture why things are they way
    they are, it doesn't make much sense to have a helper that only applies
    to half the filesystems.
    
    Granted, the contents of a file handle are generally left up to the
    individual filesystem, and the behaviors are very different, so I also
    don't see that much value in hoisting i_generation updates to the VFS
    level.
    
    I guess it wouldn't really matter if XFS stopped writing i_generation =
    0 onto disk, but I'm too curious about this odd difference in behavior
    to let it go just yet. :)
    
    --D

That makes sense. I’ll try to also look into this matter and send a
newer patch with the most optimal fix to this issue.

-Kyungchan Koh
    
    > 
    > Best,
    > Kyungchan Koh 
    >     
    >     
    >     
    >     
    > 
    

[toc] | [prev] | [next] | [standalone]

#1677824

On Wed, Jun 28, 2017 at 09:59:40PM -0700, Darrick J. Wong wrote:
> AFAICT, i_generation == 0 in XFS and btrfs is just as valid as any other
> number.  There is no special casing of zero in either filesystem.
> 
> So now, my curiosity intrigued, I surveyed all the Linux filesystems
> that can export to NFS.  I see that there are actually quite a few fs
> (ext[2-4], exofs, efs, fat, jfs, f2fs, isofs, nilfs2, reiserfs, udf,
> ufs) that treat zero as a special value meaning "ignore generation
> check"; others (xfs, btrfs, fuse, ntfs, ocfs2) that don't consider zero
> special and always require a match; and still others (affs, befs, ceph,
> gfs2, jffs2, squashfs) that don't check at all.
> 
> That to mean strongly suggests that more research is necessary to figure
> out why some of the filesystems that support i_generation reserve zero
> as a special value to disable generation checks and why others always
> require an exact match.  Until we can recapture why things are they way
> they are, it doesn't make much sense to have a helper that only applies
> to half the filesystems.

From a quick look at ext2; correct me if I got anything wrong:

	- it looks like this is *only* used by NFS fh->inode lookups,
	  there's not some other internal use for this special case.
	- filehandles are never encoded with i_generation 0, so will
	  never be returned to clients.

So, this could only ever be used by an NFS client.  But the only NFS
client that could ever use it would be a non-standard client that knew
this special feature of these particular filesystems.

Sounds like at most a slightly useful tool for malicious clients
attempting filehandle-guessing attacks.

I'm probably missing something.

--b.

[toc] | [prev] | [next] | [standalone]

#1677992

On Thu, Jun 29, 2017 at 10:35:51AM -0400, J. Bruce Fields wrote:
> On Wed, Jun 28, 2017 at 09:59:40PM -0700, Darrick J. Wong wrote:
> > AFAICT, i_generation == 0 in XFS and btrfs is just as valid as any other
> > number.  There is no special casing of zero in either filesystem.
> > 
> > So now, my curiosity intrigued, I surveyed all the Linux filesystems
> > that can export to NFS.  I see that there are actually quite a few fs
> > (ext[2-4], exofs, efs, fat, jfs, f2fs, isofs, nilfs2, reiserfs, udf,
> > ufs) that treat zero as a special value meaning "ignore generation
> > check"; others (xfs, btrfs, fuse, ntfs, ocfs2) that don't consider zero
> > special and always require a match; and still others (affs, befs, ceph,
> > gfs2, jffs2, squashfs) that don't check at all.
> > 
> > That to mean strongly suggests that more research is necessary to figure
> > out why some of the filesystems that support i_generation reserve zero
> > as a special value to disable generation checks and why others always
> > require an exact match.  Until we can recapture why things are they way
> > they are, it doesn't make much sense to have a helper that only applies
> > to half the filesystems.
> 
> From a quick look at ext2; correct me if I got anything wrong:
> 
> 	- it looks like this is *only* used by NFS fh->inode lookups,
> 	  there's not some other internal use for this special case.
> 	- filehandles are never encoded with i_generation 0, so will
> 	  never be returned to clients.
> 
> So, this could only ever be used by an NFS client.  But the only NFS
> client that could ever use it would be a non-standard client that knew
> this special feature of these particular filesystems.
> 
> Sounds like at most a slightly useful tool for malicious clients
> attempting filehandle-guessing attacks.
> 
> I'm probably missing something.

Was there ever a version of NFS (or more generally callers of the
exportfs code) that couldn't deal with i_generation in the file handle,
and therefore we invented this generation hack to work around the loss
of the generation information?

There's a comment in xfs_fs_encode_fh about not supporting 64bit inodes
with subtree_check (which seems to require one ino/gen pair for the file
and a second pair for the file's parent) on NFSv2 because v2 doesn't
provide enough space for all the file handle information, but that's the
furthest I got with lazy-mining the git history. :)

--D

> 
> --b.

[toc] | [prev] | [next] | [standalone]

#1678048

On Thu, Jun 29, 2017 at 10:25:28AM -0700, Darrick J. Wong wrote:
> Was there ever a version of NFS (or more generally callers of the
> exportfs code) that couldn't deal with i_generation in the file handle,
> and therefore we invented this generation hack to work around the loss
> of the generation information?
> 
> There's a comment in xfs_fs_encode_fh about not supporting 64bit inodes
> with subtree_check (which seems to require one ino/gen pair for the file
> and a second pair for the file's parent) on NFSv2 because v2 doesn't
> provide enough space for all the file handle information, but that's the
> furthest I got with lazy-mining the git history. :)

There's a comment in fs/ext4/super.c:ext4_nfs_get_inode

	* Currently we don't know the generation for parent directory, so
	* a generation of 0 means "accept any"

But I don't see that used.

It was used once upon a time; I see it actually used in old 2.5 code in
nfsd_get_dentry.  Hm.

--b.

[toc] | [prev] | [next] | [standalone]

#1678068

On Thu, Jun 29, 2017 at 02:30:53PM -0400, J. Bruce Fields wrote:
> On Thu, Jun 29, 2017 at 10:25:28AM -0700, Darrick J. Wong wrote:
> > Was there ever a version of NFS (or more generally callers of the
> > exportfs code) that couldn't deal with i_generation in the file handle,
> > and therefore we invented this generation hack to work around the loss
> > of the generation information?
> > 
> > There's a comment in xfs_fs_encode_fh about not supporting 64bit inodes
> > with subtree_check (which seems to require one ino/gen pair for the file
> > and a second pair for the file's parent) on NFSv2 because v2 doesn't
> > provide enough space for all the file handle information, but that's the
> > furthest I got with lazy-mining the git history. :)
> 
> There's a comment in fs/ext4/super.c:ext4_nfs_get_inode
> 
> 	* Currently we don't know the generation for parent directory, so
> 	* a generation of 0 means "accept any"
> 
> But I don't see that used.
> 
> It was used once upon a time; I see it actually used in old 2.5 code in
> nfsd_get_dentry.  Hm.

Oh, maybe it's here in fs/libfs.c:generic_fh_to_parent:

	switch (fh_type) {
	case FILEID_INO32_GEN_PARENT:
		inode = get_inode(sb, fid->i32.parent_ino,
        			  (fh_len > 3 ? fid->i32.parent_gen : 0));
		break;
	}

I'm not sure under what conditions that filehandle encoding is used.

--b.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web