Serge E. Hallyn wrote:
> Quoting Oren Laadan (orenl@xxxxxxxxxxxxxxx):
>> The two issues are related, and this is intentional. The idea
>> was that when you use CLONE_NEWPID, you imply a new nesting level
>> and a pid==1 there. So you are not supposed to specify the pid
>> of that new level.
>>
>> IOW, the parent specify the pid's of the child from the _parent's_
>> level and up (of the desired depth). CLONE_NEWPID creates a new
>> pidns level below the parent's, and that is not covered in the
>> array of pids.
>>
>> By allocation an extra slot and forcing it to be 0, we ensure that
>> the case of CLONE_NEWPID is covered correctly. Clearly, if this
>> flag isn't set, then the extra slot is redundant (but doesn't hurt).
>
> Ok, I see - I guess I don't mind those semantics. So:
>
>>>> + j = knum_pids - unum_pids;
>>> j = 1, so we copy the 3 pids in the right place.
>>>
>>>> + rc = copy_from_user(&target_pids[j], pid_set.target_pids, size);
>>>> + if (rc) {
>>>> + rc = -EFAULT;
>>>> + goto out_free;
>>>> + }
>>>> +
>>>> + return target_pids;
>>>
>>> For the second one, we have a parent task
>>>
>>> level no | pid
>>> 0 5009
>>> 1 1000
>>> 2 49
>>>
>>> calling clone_with_pid with CLONE_NEWPID and {1001,50,1} to produce:
>>>
>>> level no | pid
>>> 0 5010
>>> 1 1001
>>> 2 50
>>> 3 1
>>>
>>> So the numbers in your code become:
>>>
>>>> + unum_pids = pid_set.num_pids;
>>> unum_pids = 3
>> This is a "bug" of the parent. The parent should specify the pids
>> from the parent's level only and up, and not include the new level
>> below that will be created. (After all, it will have to be 1).
>>
>> So unum_pids = 3 will not do what you want; instead it will try to
>> create the process:
>>
>> 0 1001
>> 1 50
>> 2 1
>> 3 1
>>
>> And will fail, of course, because pid==1 at level 2 is already
>> taken.
>>
>> Instead, parent should say use {1001, 50}.
>
> Ok, but then we have the task:
>
> level no | pid
> 0 5009
> 1 1000
> 2 49
>
> calling clone(CLONE_NEWPID) with unum_pids = 2, so
>
>>>> + knum_pids = task_pid(current)->level + 1;
>>> knum_pids = 2 + 1 = 3
>>>
>>>> + target_pids = kzalloc((knum_pids + 1) * sizeof(pid_t), GFP_KERNEL);
>>> target_pids gets room for 4 pids
>>>
>>>> + j = knum_pids - unum_pids;
>
> j = 3 - 2 = 1, so we copy 1001 into pid[1] and
> 50 into pid[2], with 0 in pid[0] and pid[3]
>
> Looks good. Thanks for indulging me :)
>
> Acked-by: Serge Hallyn <serue@xxxxxxxxxx>
>
> One last thought - should there be an explicit check to make sure that
> if CLONE_NEWPID, then at the end pid[knum_pids+1] = 0? Or is that
> there and I just missed it?
the wonders of kzalloc() ...
Oren.
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
