On Fri, Apr 23, 2021 at 07:13:51PM +0200, Christian Brauner <brauner@xxxxxxxxxx> wrote:
> +static void __cgroup_signal(struct cgroup *cgrp, int signr)
> +{
> + struct css_task_iter it;
> + struct task_struct *task;
> +
> + lockdep_assert_held(&cgroup_mutex);
> +
> + css_task_iter_start(&cgrp->self, 0, &it);
I think here you'd need CSS_TASK_ITER_PROCS here to avoid signalling
multithreaded processes multiple times? (OTOH, with commiting just to
SIGKILL this may be irrelevant.)
> +static void cgroup_signal(struct cgroup *cgrp, int signr)
> [...]
> + read_lock(&tasklist_lock);
(Thinking loudly.)
I wonder if it's possible to get rid of this? A similar check that
freezer does in cgroup_post_fork() but perhaps in cgroup_can_fork(). The
fork/clone would apparently fail for the soon-to-die parent but there's
already similar error path returning ENODEV (heh, the macabrous name
cgroup_is_dead() is already occupied).
This way the cgroup-killer wouldn't unncessarily preempt tasklist_lock
exclusive requestors system-wide.
> @@ -4846,6 +4916,11 @@ static struct cftype cgroup_base_files[] = {
> + {
> + .name = "cgroup.signal",
> + .flags = CFTYPE_NOT_ON_ROOT,
> + .write = cgroup_signal_write,
I think this shouldn't be visible in threaded cgroups (or return an
error when attempting to kill those).
Attachment:
signature.asc
Description: Digital signature
