Re: [PATCH -mm 5/7] add user namespace

[Kirill Korotaev <dev@sw.ru>]

>>Lets take a look at sys_setpriority() or any other function calling
>>find_user():
>>it can change the priority for all user or group processes like:
>>
>>do_each_thread_ve(g, p) {
>>   if (p->uid == who)
>>       error = set_one_prio(p, niceval, error);
>>} while_each_thread_ve(g, p);
> 
> 
> eh. this is openvz code ! thanks :)
it doesn't matter :)
2.6.17 code is:
                        do_each_thread(g, p)
                                if (p->uid == who)
                                        error = set_one_prio(p, niceval, error);
                        while_each_thread(g, p);

when introducing process namespaces we will have to isolate processes somehow and this loop, agree?
in this case 1 user-namespace can belong to 2 process-namespaces, agree?
how do you see this loop in the future making sure that above situation is handled correctly?
how many other such places do we have?

>>which essentially means that user-namespace becomes coupled with
>>process-namespace. Sure, we can check in every such place for
>> p->nsproxy->user_ns == current->nsproxy->user_ns
>>condition. But this a way IMHO leading to kernel full of security
>>crap which is hardly maintainable.
> 
> 
> only 4 syscalls use find_user() : sys_setpriority, sys_getpriority,
> sys_ioprio_set, sys_ioprio_get and they use it very simply to check if a
> user_struct exists for a given uid. So, it should be OK. But please see the
> attached patch.
the problem is not in find_user() actually. but in uid comparison inside
some kind of process iteration loop.
In this case you select processes p which belong to both namespaces simultenously:
i.e. processes p which belong both to user-namespace U and process-namespace P.

I hope I was more clear this time :)

>>Another example of not so evident coupling here:
>>user structure maintains number of processes/opened
>>files/sigpending/locked_shm etc.
>>if a single user can belong to different proccess/ipc/... namespaces
>>all these becomes unusable.
> 
> 
> this is the purpose of execns.
> 
> user namespace can't be unshared through the unshare syscall().
why? we do it fine in OpenVZ.

> they can
> only be unshared through execns() which flushes the previous image of the
> process. However, the execns patch still needs to close files without the
> close-on-exec flag. I didn't do it yet. lazy me :)

Kirill