Messages in this thread | | | Date | Tue, 27 Aug 2013 14:11:33 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH 11/12] pid: rewrite task helper functions avoiding task->pid and task->tgid |
| |
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.
| |