Sukadev Bhattiprolu <sukadev@xxxxxxxxxxxxxxxxxx> writes: > Eric W. Biederman [ebiederm@xxxxxxxxxxxx] wrote: > | > Could you clarify ? How is the call to alloc_pidmap() from clone3() different > | > from the call from clone() itself ? > | > | I think it is totally inappropriate to assign pids in a pid namespace > | where there are user space processes already running. > > Honestly, I don't understand why it is inappropriate or how this differs > from normal clone() - which also assigns pids in own and ancestor pid > namespaces. The fact we can specify which pids we want. I won't claim it is as exploitable as NULL pointer deferences have been but it has that kind of feel to it. > | > | How we handle a clone extension depends critically on if we want to > | > | create a processes for restart in user space or kernel space. > | > | > | > | Could some one give me or point me at a strong case for creating the > | > | processes for restart in user space? > | > > | > There has been a lot of discussion on this with reference to the > | > Checkpoint/Restart patchset. See http://lkml.org/lkml/2009/4/13/401 > | > for instance. > | > | Just read it. Thank you. > > Sorry. I should have mentioned the reason here. (Like you mention below), > flexibility is the main reason. > > | Now I am certain clone_with_pids() is not useful functionality to be > | exporting to userspace. > | > | The only real argument in favor of doing this in user space is greater > | flexibility. I can see checkpointing/restoring a single thread process > | without a pid namespace. Anything more and you are just asking for > | trouble. > | > | A design that weakens security. Increases maintenance costs. All for > | an unreliable result seems like a bad one to me. > | > | > | The pid assignment code is currently ugly. I asked that we just pass > | > | in the min max pid pids that already exist into the core pid > | > | assignment function and a constrained min/max that only admits a > | > | single pid when we are allocating a struct pid for restart. That was > | > | not done and now we have a weird abortion with unnecessary special cases. > | > > | > I did post a version of the patch attemptint to implement that. As > | > pointed out in: > | > > | > http://lkml.org/lkml/2009/8/17/445 > | > > | > we would need more checks in alloc_pidmap() to cover cases like min or max > | > being invalid or min being greater than max or max being greater than pid_max > | > etc. Those checks also made the code ugly (imo). > | > | If you need more checks you are doing it wrong. The code already has min > | and max values, and even a start value. I was just strongly suggesting > | we generalize where we get the values from, and then we have not special > | cases. > > Well, if alloc_pidmap(pid_ns, min, max) does not have to check the > parameters passed in (ie assumes that callers pass it in correctly) > it might be simple. But when user specifies the pid, the > > min == max == user's target pid > > so we will need to check the values either here or in callers. Agreed. When you are talking about the target pid. That code path needs the extra check. > Yes the code already has values and a start value. But these are > controlled by alloc_pidmap() and not passed in from the user space. I was only thinking passed in from someplace else in kernel/pid.c > alloc_pidmap() needs to assign the next available pid or a specific > target pid. Generalizing it to alloc a pid in a range seemed be a > bit of an over kill for currently known usages. alloc_pidmap in assigning the next available pid is allocating a pid in a range. > I will post a version of the patch outside this patchset with min > and max parameters and we can see if it can be optimized/beautified. Thanks, Eric _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/containers
