linux-kernel - Re: [PATCH 2/6] cgroup: Allocate cgroup_file_ctx for kernfs_open

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]

Date:   Fri, 10 Dec 2021 09:53:41 -0800
From:   Linus Torvalds <torvalds@...uxfoundation.org>
To:     Tejun Heo <tj@...nel.org>
Cc:     "Eric W. Biederman" <ebiederm@...ssion.com>,
        Michal Koutny <mkoutny@...e.com>, Jens Axboe <axboe@...nel.dk>,
        Kees Cook <keescook@...omium.org>,
        Oleg Nesterov <oleg@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jim Newsome <jnewsome@...project.org>,
        Alexey Gladkov <legion@...nel.org>,
        Andy Lutomirski <luto@...capital.net>,
        Jann Horn <jannh@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Security Officers <security@...nel.org>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH 2/6] cgroup: Allocate cgroup_file_ctx for kernfs_open_file->priv

On Thu, Dec 9, 2021 at 1:47 PM Tejun Heo <tj@...nel.org> wrote:
>
> of->priv is currently used by each interface file implementation to store
> private information. This patch collects the current two private data usages
> into struct cgroup_file_ctx which is allocated and freed by the common path.
> This allows generic private data which applies to multiple files, which will
> be used to in the following patch.

I'm not sure if it's worth it having that union just to make the
struct be 8 bytes instead of 16 (and later 16 bytes instead of 24),
when the real cost is that dynamic allocation overhead, and there's
likely only one or two active actual allocations at a time.

IOW, I'm not convinced there's any real memory savings, and making it
a union means that now you have to be very careful about who does
what..

And yes, people historically had to be very careful _anyway_, because
the "union" was kind of implicit in how there was just a shared 'void
*priv' thing that was either that iterator pointer or that psi trigger
pointer.

So in that sense this is a semantically minimal patch, but I think
that practically speaking we'd be better off without that possible
source of confusion, and just always have that cgroup proc file have a
full structure.

In fact, I think it means we could just make the thing *contain* the
iterator, instead of having a pointer to an iterator, and getting rid
of the now redundant dynamic alloc/free of the iterator pointer.

Wouldn't that simplify things? And might there not be some cgroup
pressure user that also wants to use the iterator interfaces? Maybe
not, my point is more that once we have an explicit struct allocation
for cgroup proc files, we might as well clarify and simplify the
code..

           Linus