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:   Mon, 13 Dec 2021 09:56:26 -1000
From:   Tejun Heo <tj@...nel.org>
To:     Linus Torvalds <torvalds@...uxfoundation.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

Hello,

On Mon, Dec 13, 2021 at 11:29:36AM -0800, Linus Torvalds wrote:
> Looking at the patch, I mostly like the end result, although it
> highlights how odd that __cgroup_procs_start() thing is.
> 
> I wonder if that 'started' flag could be removed, and the
> css_task_iter_start() thing could be done at allocation time?
> 
> But I may just be confused. This is not a NAK, just a "that pattern
> looks slightly odd to me".

Yeah, it's a bit odd. So...

css_task_iters are stateful and a bit expensive to initialize, so we don't
want to do it from generic open path. However, we can add an open method for
cgroup_files and then do it from there.

However, while these files don't allow random seeking, they do allow seeking
to 0 to re-read the whole content, so the iteration start path needs to
handle both the initial read and subsequent repeating reads. While this can
be handled by starting iter on open and then always restarting it on
seq_start, that isn't ideal given the prior point. So, if we want to handle
it in the seq_start path, we need to distinguish the initial read and
re-reads anyway.

This may be neater if we start the iter from open and restart from seek so
that the seq_start method doesn't have to distinguish the two. However,
kernfs doesn't forward the seek event downwards (yet anyway).

So, there's a bit of awkwardness but it at least isn't gratuitous given the
surroundings.

Thanks.

-- 
tejun