On Wed, Aug 07, 2019 at 04:26:10PM +0200, Oleg Nesterov wrote: > On 08/06, Adrian Reber wrote: > > > > +struct pid *alloc_pid(struct pid_namespace *ns, int set_tid) > > { > > struct pid *pid; > > enum pid_type type; > > @@ -186,12 +186,35 @@ struct pid *alloc_pid(struct pid_namespace *ns) > > 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); > > + if (set_tid) { > > + /* > > + * Also fail if a PID != 1 is requested > > + * and no PID 1 exists. > > + */ > > + if ((set_tid >= pid_max) || ((set_tid != 1) && > > + (idr_is_empty(&tmp->idr)))) { > > too many parentheses ;) this is purely cosmetic, up to you, but to me > > if (set_tid >= pid_max || > (set_tid != 1 && idr_is_empty(&tmp->idr))) { > > looks a bit more readable. > > > > + spin_unlock_irq(&pidmap_lock); > > + retval = -EINVAL; > > + goto out_free; > > This doesn't look right, you need idr_preload_end() before goto out_free. > > But I'd suggest to simply do > > nr = -EINVAL; > if (set_tid < pid_max && > (set_tid != 1 || idr_is_empty(&tmp->idr))) > nr = idr_alloc(&tmp->idr, NULL, set_tid, > set_tid + 1, GFP_ATOMIC); > > ... > > this is more robust.
That all sounds reasonable to me.