linux-kernel - Re: [GIT PULL] tracing: A few more fixes for 6.7

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: <CAHk-=wjmUr93TFgpZ6ew3t5XFO2Cyxb8wnRENCkWvrH8m9XEOA@mail.gmail.com>
Date: Thu, 21 Dec 2023 12:01:45 -0800
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Masami Hiramatsu <mhiramat@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, 
	Randy Dunlap <rdunlap@...radead.org>, Alexander Graf <graf@...zon.com>
Subject: Re: [GIT PULL] tracing: A few more fixes for 6.7

On Thu, 21 Dec 2023 at 11:27, Steven Rostedt <rostedt@...dmis.org> wrote:
>
> Luckily, that's easy to get to. All I need to do is:
>
> static void update_inode_attr(struct dentry *dentry, struct inode *inode,
>                               struct eventfs_attr *attr, umode_t mode)
> {
>         struct tracefs_fs_info *fsi = dentry->d_sb->s_fs_info;
>         struct tracefs_mount_opts *opts = &fsi->mount_opts;
>
>         /* Default the ownership to what it was mounted as */
>         inode->i_uid = opts->uid;
>         inode->i_gid = opts->gid;

I think you should add

>         inode->i_mode = mode;

to that "default setup", which not only makes things more consistent,
it also means that you can then remove it from here:

>         if (!attr) {
>                 inode->i_mode = mode;
>                 return;
>         }

.. and the 'else' side from here:

>         if (attr->mode & EVENTFS_SAVE_MODE)
>                 inode->i_mode = attr->mode & EVENTFS_MODE_MASK;
>         else
>                 inode->i_mode = mode;

and it all looks a lot more clear and obvious.

"Set things to default values, then if we have attr and the specific
fields are set in those attrs, update them".

Instead of having this odd "do one thing for git/uid, another for mode".

> > I still claim that the whole dynamic ftrace stuff was a huge mistake,
> > and that the real solution should always have been to just use one
> > single inode for every file (and use that 'attr' that you track and
> > the '->getattr()' callback to make them all *look* different to
> > users).
>
> Files now do not even have meta-data, and that saved 2 megs per trace
> instance. I only keep meta data for the directories. The files themselves
> are created via callback functions.

I bet that was basically *all* just the inodes.

The dentries take up very little space, and the fact that you didn't
keep the dentries around meant that you instead replaced them with
that 'struct eventfs_file' which probably takes up as much room as the
dentries ever did - and now when you use them, you obviously use
*more* memory since it duplicates the data in the dentries, including
the name etc.

So I bet you use *more* memory than if you just kept the dentry tree
around, and this dynamic creation has then caused a number of bugs and
a lot of extra complexity - things like having to re-implement your
own readdir() etc, much of which has been buggy.

And when you fix the resulting bugs, the end result is often
disgusting. I'm talking about things like commit ef36b4f92868
("eventfs: Remember what dentries were created on dir open"), which
does things like re-use file->private_data for two entirely different
things (is it a 'cursor' or a 'dlist'? Who can know? That thing makes
me gag).

Honestly, that was just one example of "that code does some truly ugly
things because the whole notion is mis-designed".

            Linus