Sender: Tejun Heo <htejun@gmail.com>
Date:   Mon, 13 Dec 2021 09:56:26 -1000
From:   Tejun Heo <tj@kernel.org>
To:     Linus Torvalds <torvalds@linuxfoundation.org>
Cc:     "Eric W. Biederman" <ebiederm@xmission.com>,
        Michal Koutny <mkoutny@suse.com>, Jens Axboe <axboe@kernel.dk>,
        Kees Cook <keescook@chromium.org>,
        Oleg Nesterov <oleg@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Jim Newsome <jnewsome@torproject.org>,
        Alexey Gladkov <legion@kernel.org>,
        Andy Lutomirski <luto@amacapital.net>,
        Jann Horn <jannh@google.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Security Officers <security@kernel.org>,
        Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH 2/6] cgroup: Allocate cgroup_file_ctx for
 kernfs_open_file->priv
Message-ID: <Ybelaqt8uiyN+DkW@slm.duckdns.org>
References: <20211213191833.916632-1-tj@kernel.org>
 <20211213191833.916632-3-tj@kernel.org>
 <CAHk-=wgQQhQakEgKeCcDp==wk__pu32=k-1m63prWx+XjzyhHg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHk-=wgQQhQakEgKeCcDp==wk__pu32=k-1m63prWx+XjzyhHg@mail.gmail.com>
Precedence: bulk

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