Received-SPF: pass (google.com: domain of linux-kernel+bounces-33669-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240117143548.595884070@goodmis.org> <20240117143810.531966508@goodmis.org>
 <CAMuHMdXKiorg-jiuKoZpfZyDJ3Ynrfb8=X+c7x0Eewxn-YRdCA@mail.gmail.com> <20240122100630.6a400dd3@gandalf.local.home>
In-Reply-To: <20240122100630.6a400dd3@gandalf.local.home>
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: Mon, 22 Jan 2024 17:23:29 +0100
Message-ID: <CAMuHMdXD0weO4oku8g2du6fj-EzxGaF+0i=zrPScSXwphFAZgg@mail.gmail.com>
Subject: Re: [for-linus][PATCH 1/3] eventfs: Have the inodes all for files and
 directories all be the same
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.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>, Linus Torvalds <torvalds@linux-foundation.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Steven,

On Mon, Jan 22, 2024 at 4:05=E2=80=AFPM Steven Rostedt <rostedt@goodmis.org=
> wrote:
> On Mon, 22 Jan 2024 11:38:52 +0100
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> > Hi Stephen,
>
> I don't know who "Stephen" is, but I'll reply to this message.

My apologies (too many different spellings in too many languages)...

> > On Wed, Jan 17, 2024 at 3:37=E2=80=AFPM Steven Rostedt <rostedt@goodmis=
org> wrote:
> > > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> > >
> > > The dentries and inodes are created in the readdir for the sole purpo=
se of
> > > getting a consistent inode number. Linus stated that is unnecessary, =
and
> > > that all inodes can have the same inode number. For a virtual file sy=
stem
> > > they are pretty meaningless.
> > >
> > > Instead use a single unique inode number for all files and one for al=
l
> > > directories.
> > >
> > > Link: https://lore.kernel.org/all/20240116133753.2808d45e@gandalf.loc=
al.home/
>
> Yeah, Linus wanted me to try this first and see if there's any regression=
s.
> Well, I guess you just answered that.
>
> The above link has me saying to Linus:
>
>   It was me being paranoid that using the same inode number would break u=
ser
>   space. If that is not a concern, then I'm happy to just make it either =
the
>   same, or maybe just hash the ei and name that it is associated with.
>
> > > Link: https://lore.kernel.org/linux-trace-kernel/20240116211353.41218=
0363@goodmis.org
> > >
> > > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > Cc: Christian Brauner <brauner@kernel.org>
> > > Cc: Al  Viro <viro@ZenIV.linux.org.uk>
> > > Cc: Ajay Kaher <ajay.kaher@broadcom.com>
> > > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> >
> > Thanks for your patch, which is now commit 53c41052ba312176 ("eventfs:
> > Have the inodes all for files and directories all be the same") in
> > v6.8-rc1, to which I have bisected the issue below.

> > This confuses "find".
> > Running "find /sys/" now prints lots of error messages to stderr:
> >
> > find: File system loop detected;
> > =E2=80=98/sys/kernel/debug/tracing/events/initcall/initcall_finish=E2=
=80=99 is part of
> > the same file system loop as
> > =E2=80=98/sys/kernel/debug/tracing/events/initcall=E2=80=99.
>
> So at a minimum, the directories need to have unique inode numbers.
>
>
> > find: File system loop detected;
> > =E2=80=98/sys/kernel/debug/tracing/events/initcall/initcall_start=E2=80=
=99 is part of
> > the same file system loop as
> > =E2=80=98/sys/kernel/debug/tracing/events/initcall=E2=80=99.
> > find: File system loop detected;
> > =E2=80=98/sys/kernel/debug/tracing/events/initcall/initcall_level=E2=80=
=99 is part of
> > the same file system loop as
> > =E2=80=98/sys/kernel/debug/tracing/events/initcall=E2=80=99.
> > [...]
>
> Does this fix it for you? It hashes the eventfs_inode data structure afte=
r
> adding some salt to it.
>
> Kees,
>
> I'm using the eventfs_inode pointer to create a unique value for the inod=
e.
> But it's being salted, hashed and then truncated. As it is very easy to
> read inodes (although by default, only root has access to read these
> inodes), the inode numbers themselves shouldn't be able to leak kernel
> addresses via the results of these inode numbers, would it?
>
> -- Steve

Gotya ;-)

>
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index 6795fda2af19..d54897b84596 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c

> @@ -36,6 +37,31 @@ static DEFINE_MUTEX(eventfs_mutex);
>  #define EVENTFS_FILE_INODE_INO         0x12c4e37
>  #define EVENTFS_DIR_INODE_INO          0x134b2f5
>
> +/* Used for making inode numbers */
> +static siphash_key_t inode_key;
> +
> +/* Copied from scripts/kconfig/symbol.c */
> +static unsigned strhash(const char *s)
> +{
> +       /* fnv32 hash */
> +       unsigned hash =3D 2166136261U;
> +       for (; *s; s++)
> +               hash =3D (hash ^ *s) * 0x01000193;
> +       return hash;
> +}
> +
> +/* Just try to make something consistent and unique */
> +static int eventfs_dir_ino(struct event_inode *ei, const char *name)

eventfs_inode

> +{
> +       unsigned long sip =3D (unsigned long)ei;
> +
> +       sip +=3D strhash(name) + EVENTFS_DIR_INODE_INO;
> +       sip =3D siphash_1u32((int)sip, &inode_key);
> +
> +       /* keep it positive */
> +       return sip & ((1U << 31) - 1);
> +}
> +
>  /*
>   * The eventfs_inode (ei) itself is protected by SRCU. It is released fr=
om
>   * its parent's list and will have is_freed set (under eventfs_mutex).

Thanks, find is happy now the directories have different inode numbers.
The files still have identical inodes numbers, I hope that doesn't cause
any issues...

Gr{oetje,eeting}s,

                        Geert

--=20
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k=
org

In personal conversations with technical people, I call myself a hacker. Bu=
t
when I'm talking to journalists I just say "programmer" or something like t=
hat.
                                -- Linus Torvalds