linux-kernel - Re: [PATCH v6 2/3] cgroups: allow a cgroup subsystem to reject a fork

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: <CAOviyahTq1K6zBRfmXfepabiQ7xU8ZoNdq0jPd3nP4Q-0H1DDQ@mail.gmail.com>
Date:	Fri, 27 Mar 2015 01:38:54 +1100
From:	Aleksa Sarai <cyphar@...har.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	lizefan@...wei.com, mingo@...hat.com, peterz@...radead.org,
	richard@....at,
	Frédéric Weisbecker <fweisbec@...il.com>,
	linux-kernel@...r.kernel.org, cgroups@...r.kernel.org
Subject: Re: [PATCH v6 2/3] cgroups: allow a cgroup subsystem to reject a fork

Hi Tejun,

Here's a bit more of a follow-up on two of your points:

Since

> Do we really need a separate callback for this?  Can't we just add
> @old_css param to ss->fork() which is NULL if it didn't change since
> can_fork()?

and

> Did you explore the idea of passing an opaque pointer from can_fork to
> post_fork?  If so, why did you choose this direction instead of that
> one?

Are quite similar (they essentially both result in each other, since if you use
void pointers for state you can't check if the association changed without
doing completely separate accounting which wouldn't be used by the callbacks),
I'll respond to both in the same go:

The issue I can see with passing around an opaque pointer to fork() is that you
have a random private (void **) argument that is completely useless if you
don't use can_fork(). This is why I think we should call the reapply_fork()
callback if the association changes [we could call it something else if you
like, since reapply_fork() is a pids-specific name -- what about switch_fork(),
reassoc_fork(), re_fork() or something to show that it's a callback if the
association changes?] (the subsystem can decide if they want to ignore it / if
they don't want to touch it) and we deal with pinning / dropping the ref of the
css_set for the current task inside the cgroup_* callbacks. That way, we don't
start messing around with post-fork() callbacks that aren't related to the new
conditional stuff.

I mean, if you want to have a random, completely unused and essentially
vestigial argument to ss->fork() [if you don't use the new can_fork() callbacks
(and actually care about storing private data)] then I can just write that. It
just looks like a weird callback API imho.

In fact, it seems even more single purpose than just pinning the ref and
dealing with it for the general case of an association switching (because now
you have a single-purpose parameter to the general fork() call, as opposed to
doing some ref pinning and accounting of the current css_set for (currently)
only the pids subsystem in the cgroup core. Since we know  that pids will need
to pin the ref, then we're not doing any extra work in  doing it for the
general cgroup fork callback -- and we get to both separate "association
changed" logic from "post fork" logic in the callbacks, as well as not blindly
trusting the cgroup subsystem to properly deal with a changing association.

--
Aleksa Sarai (cyphar)
www.cyphar.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/