Re: [RFC][PATCH 3/7] Add target_pid parameter to alloc_pidmap()

[Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>]

Serge E. Hallyn [serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org] wrote:
| Quoting sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org (sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
| > From: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
| > 
| > 
| > Signed-off-by: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
| > ---
| >  kernel/pid.c |   28 ++++++++++++++++++++++++++--
| >  1 files changed, 26 insertions(+), 2 deletions(-)
| > 
| > diff --git a/kernel/pid.c b/kernel/pid.c
| > index fd72ad9..93406c6 100644
| > --- a/kernel/pid.c
| > +++ b/kernel/pid.c
| > @@ -147,12 +147,36 @@ static int alloc_pidmap_page(struct pidmap *map)
| >  	return 0;
| >  }
| > 
| > -static int alloc_pidmap(struct pid_namespace *pid_ns)
| > +static int set_pidmap(struct pid_namespace *pid_ns, int pid)
| > +{
| > +	int offset;
| > +	struct pidmap *map;
| > +
| > +	if (pid >= pid_max)
| > +		return -EINVAL;
| > +
| > +	offset = pid & BITS_PER_PAGE_MASK;
| > +	map = &pid_ns->pidmap[pid/BITS_PER_PAGE];
| > +
| > +	if (alloc_pidmap_page(map))
| > +		return -ENOMEM;
| > +
| > +	if (test_and_set_bit(offset, map->page))
| > +		return -EBUSY;
| > +
| > +	atomic_dec(&map->nr_free);
| > +	return pid;
| > +}
| > +
| > +static int alloc_pidmap(struct pid_namespace *pid_ns, int target_pid)
| >  {
| >  	int i, offset, max_scan, pid, last = pid_ns->last_pid;
| >  	struct pidmap *map;
| >  	int rc = -EAGAIN;
| > 
| > +	if (target_pid)
| > +		return set_pidmap(pid_ns, target_pid);
| 
| I think this whole patchset is still NACKed until you tag
| pid_namespaces with a creator uid, and ensure that
| current_uid()==pid_ns->creator_uid() at each level where
| the caller is specifying a pid.

I currently have CAP_SYS_ADMIN check in clone_with_pids() and was
thinking that the tagging of pid namespaces can be done indpendent
of this patchset (as would integrating your patch of making pid_max
a property of pid-namespace).

| 
| I don't see that in this set.
| 
| OTOH, your approach of pulling alloc_pidmap_page() out of
| alloc_pidmap() and re-using it may be what Eric wanted to
| see.

Yes, I think the first few helper patches in the set would be needed/
useful to restart a process with a pid (not just for the clone-with-pids
syscall).

Sukadev