lkml.org 
[lkml]   [2013]   [Aug]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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.


    \
     
     \ /
      Last update: 2013-08-27 15:01    [W:4.055 / U:0.132 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site