Messages in this thread | | | Date | Mon, 11 Nov 2019 16:40:28 +0100 | From | Adrian Reber <> | Subject | Re: [PATCH v7 1/2] fork: extend clone3() to support setting a PID |
| |
On Mon, Nov 11, 2019 at 04:25:15PM +0100, Oleg Nesterov wrote: > On 11/11, Adrian Reber wrote: > > > > v7: > > - changed set_tid to be an array to set the PID of a process > > in multiple nested PID namespaces at the same time as discussed > > at LPC 2019 (container MC) > > cough... iirc you convinced me this is not needed when we discussed > the previous version ;) Nevermind, probably my memory fools me.
You are right. You suggested the same thing and we didn't listen ;)
> So far I only have some cosmetic nits,
Thanks for the quick review. I will try to apply your suggestions.
> > @@ -175,6 +187,18 @@ struct pid *alloc_pid(struct pid_namespace *ns) > > > > for (i = ns->level; i >= 0; i--) { > > int pid_min = 1; > > + int t_pos = 0; > ^^^^^ > > I won't insist, but I'd suggest to cache set_tid[t_pos] instead to make > the code a bit more simple. > > > @@ -186,12 +210,24 @@ struct pid *alloc_pid(struct pid_namespace *ns) > > if (idr_get_cursor(&tmp->idr) > RESERVED_PIDS) > > pid_min = RESERVED_PIDS; > > You can probably move this code into the "else" branch below. > > IOW, something like > > > for (i = ns->level; i >= 0; i--) { > int xxx = 0; > > if (set_tid_size) { > int pos = ns->level - i; > > xxx = set_tid[pos]; > if (xxx < 1 || xxx >= pid_max) > return ERR_PTR(-EINVAL); > /* Also fail if a PID != 1 is requested and no PID 1 exists */ > if (xxx != 1 && !tmp->child_reaper) > return ERR_PTR(-EINVAL); > if (!ns_capable(tmp->user_ns, CAP_SYS_ADMIN)) > return ERR_PTR(-EPERM); > set_tid_size--; > } > > idr_preload(GFP_KERNEL); > spin_lock_irq(&pidmap_lock); > > if (xxx) { > nr = idr_alloc(&tmp->idr, NULL, xxx, xxx + 1, > GFP_ATOMIC); > /* > * If ENOSPC is returned it means that the PID is > * alreay in use. Return EEXIST in that case. > */ > if (nr == -ENOSPC) > nr = -EEXIST; > } else { > int pid_min = 1; > /* > * init really needs pid 1, but after reaching the > * maximum wrap back to RESERVED_PIDS > */ > if (idr_get_cursor(&tmp->idr) > RESERVED_PIDS) > pid_min = RESERVED_PIDS; > /* > * Store a null pointer so find_pid_ns does not find > * a partially initialized PID (see below). > */ > nr = idr_alloc_cyclic(&tmp->idr, NULL, pid_min, > pid_max, GFP_ATOMIC); > } > > ... > > This way only the "if (set_tid_size)" block has to play with set_tid_size/set_tid. > > note also that this way we can easily allow set_tid[some_level] == 0, we can > simply do > > - if (xxx < 1 || xxx >= pid_max) > + if (xxx < 0 || xxx >= pid_max) > > although I don't think this is really useful.
Yes. I explicitly didn't allow 0 as a PID as I didn't thought it would be useful (or maybe even valid).
Adrian
| |