Re: [PATCH 11/12] pid: rewrite task helper functions avoiding task->pid and task->tgid

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Aug 26, 2013 at 10:37:22PM -0400, Richard Guy Briggs wrote:
> On Fri, Aug 23, 2013 at 08:36:21AM +0200, Peter Zijlstra wrote:
> > Except that's not the case, with namespaces there's a clear hierarchy
> > and the task_struct::pid is the one true value aka. root namespace.
> 
> Peter, I agonized over the access efficiency of dropping this one or the
> duplicate in task_struct::pids and this one was far easier to drop in
> terms of somehow always forcing
> task->pids[PIDTYPE_PID].pid->numbers[0].nr to point to task->pid.

You mean there's more than 1 site that sets task_struct::pid? I thought
we only assign that thing once in fork.c someplace.

> It should be possible to audit the kernel to make certain task->pid is
> only ever written at the time of task creation and copied to its own
> task->pids[PIDTYPE_PID].pid->numbers[0].nr at the time of task creation
> so that the two values are consistent.  Continuously auditing the kernel
> so this is the case would be a bit more of a challenge.

I know there's people running scripts over git commits to catch abuse,
if this is scriptable that might be doable.

> Would it be reasonable to suggest task_struct::pid only be accessed by
> the existing inlined task_pid_nr() converted to const?

Sure that works for me, alternatively what's wrong with making
task_struct::pid itself a const pid_t ? Then assignment will need an
ugly cast to even work.

> The goal is to gain assurance that any PIDs referred to in audit logs
> are indisputable.
> 
> Would you be alright with doing away with task_struct::tgid?

So I don't particularly use that one much -- if at all. So no I don't
mind it too much.

> > Furthermore idle threads really are special and it doesn't make sense to
> > address them in any but the root namespace, doubly so because only
> > kernel space does this.
> 
> If that is the case, and the above holds true, then we don't need the
> second hunk, I agree.

It should be the case -- not entirely sure it is the case, but in any
case pid-0 should be 'special' by all accounts.

> > As for the init thread, that function is called is_global_init() for
> > crying out loud, what numb nut doesn't get that that's supposed to be
> > using the root namespace?
> 
> A process in another pid namespace?  If that's never going to happen,
> then drop it.

That'd be a bug I suppose, you want the 'global' init when using that
function. I don't use this function, never have. So I really don't know
_that_ much about it. It just seems to me that the name really implies
it wants the root init process and not any other.

> > Seriously, you namespace guys should stop messing things up and
> > confusing yourselves and others.
> 
> "you namespace guys"?  I'm not a namespace guy.  I'm a rusty kernel
> network security guy taking on audit, trying to prepare it for moving
> into a more and more namespace-enabled place of
> containerization/light-virtualization.

Well, you let yourself in with 'those' people ;-)

> We aren't ready for it yet, but there is demand to run additional auditd
> daemons in other pid namespaces and some of this work is trying to move
> in that direction.

So afaict that's 'simply' writing the 'right' pid to your logger, right?
Your additional concern that the pid space isn't corrupted sounds a bit
superfluous to me, we don't typically muck about with pids, stuff would
horribly break if we did that.

There's a few special cases, like the idle threads having pid-0 and
'simple' people like myself prefer to use task_struct::pid for debugging
when we run our simple kernels without all this namespace stuff enabled.

The entire task->pids[PIDTYPE_PID].pid->numbers[0].nr thing just seems
increddibly unwieldy and double dereferences, even if the lines are
'hot' are worse than single derefs.