Received-SPF: pass (google.com: domain of linux-kernel+bounces-34199-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 22 Jan 2024 13:33:52 -0800
From: Kees Cook <keescook@chromium.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	linux-kernel@vger.kernel.org,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Christian Brauner <brauner@kernel.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Ajay Kaher <ajay.kaher@broadcom.com>
Subject: Re: [for-linus][PATCH 1/3] eventfs: Have the inodes all for files
 and directories all be the same
Message-ID: <202401221333.B3661AD71@keescook>
References: <20240117143548.595884070@goodmis.org>
 <20240117143810.531966508@goodmis.org>
 <CAMuHMdXKiorg-jiuKoZpfZyDJ3Ynrfb8=X+c7x0Eewxn-YRdCA@mail.gmail.com>
 <20240122100630.6a400dd3@gandalf.local.home>
 <CAMuHMdXD0weO4oku8g2du6fj-EzxGaF+0i=zrPScSXwphFAZgg@mail.gmail.com>
 <20240122114743.7e46b7cb@gandalf.local.home>
 <CAHk-=wiq5mr+wSb6pmtt7QqBhQo_xr7ip=yMwQ5ryWVwCyMhfg@mail.gmail.com>
 <CAHk-=wjGxVVKvxVf=NDnMhB3=eQ_NMiEY3onG1wRAjJepig=aw@mail.gmail.com>
 <CAHk-=wiLqJYT2GGSBhKuJS-Uq1DVq3S32oP0SwqQiATuBivxcg@mail.gmail.com>
 <20240122144443.0f9cf5b9@gandalf.local.home>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240122144443.0f9cf5b9@gandalf.local.home>

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