Re: [for-linus][PATCH 1/3] eventfs: Have the inodes all for files and directories all be the same

[Kees Cook <keescook@chromium.org>]

On Mon, Jan 22, 2024 at 02:44:43PM -0500, Steven Rostedt wrote:
> On Mon, 22 Jan 2024 10:19:12 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > On Mon, 22 Jan 2024 at 09:39, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > > Actually, why not juist add an inode number to your data structures,
> > > at least for directories? And just do a static increment on it as they
> > > get registered?
> > >
> > > That avoids the whole issue with possibly leaking kernel address data.  
> > 
> > The 'nlink = 1' thing doesn't seem to make 'find' any happier for this
> > case, sadly.
> > 
> > But the inode number in the 'struct eventfs_inode' looks trivial. And
> > doesn't even grow that structure on 64-bit architectures at least,
> > because the struct is already 64-bit aligned, and had only one 32-bit
> > entry at the end.
> > 
> > On 32-bit architectures the structure size grows, but I'm not sure the
> > allocation size grows. Our kmalloc() is quantized at odd numbers.
> > 
> > IOW, this trivial patch seems to be much safer than worrying about
> > some pointer exposure.
> 
> I originally wanted to avoid the addition of the 4 bytes, but your comment
> about it not making a difference on 64bit due to alignment makes sense.
> 
> Slightly different version below.
> 
> -- Steve
> 
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index 6795fda2af19..6b211522a13e 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -34,7 +34,15 @@ static DEFINE_MUTEX(eventfs_mutex);
>  
>  /* Choose something "unique" ;-) */
>  #define EVENTFS_FILE_INODE_INO		0x12c4e37
> -#define EVENTFS_DIR_INODE_INO		0x134b2f5
> +
> +/* Just try to make something consistent and unique */
> +static int eventfs_dir_ino(struct eventfs_inode *ei)
> +{
> +	if (!ei->ino)
> +		ei->ino = get_next_ino();
> +
> +	return ei->ino;
> +}
>  
>  /*
>   * The eventfs_inode (ei) itself is protected by SRCU. It is released from
> @@ -396,7 +404,7 @@ static struct dentry *create_dir(struct eventfs_inode *ei, struct dentry *parent
>  	inode->i_fop = &eventfs_file_operations;
>  
>  	/* All directories will have the same inode number */
> -	inode->i_ino = EVENTFS_DIR_INODE_INO;
> +	inode->i_ino = eventfs_dir_ino(ei);
>  
>  	ti = get_tracefs(inode);
>  	ti->flags |= TRACEFS_EVENT_INODE;
> @@ -802,7 +810,7 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx)
>  
>  		name = ei_child->name;
>  
> -		ino = EVENTFS_DIR_INODE_INO;
> +		ino = eventfs_dir_ino(ei_child);
>  
>  		if (!dir_emit(ctx, name, strlen(name), ino, DT_DIR))
>  			goto out_dec;
> diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
> index 12b7d0150ae9..1a574d306ea9 100644
> --- a/fs/tracefs/internal.h
> +++ b/fs/tracefs/internal.h
> @@ -64,6 +64,7 @@ struct eventfs_inode {
>  		struct llist_node	llist;
>  		struct rcu_head		rcu;
>  	};
> +	unsigned int			ino;
>  	unsigned int			is_freed:1;
>  	unsigned int			is_events:1;
>  	unsigned int			nr_entries:30;

I like it! :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook