On 10/22, Gargi Sharma wrote:
>
> On Fri, Oct 20, 2017 at 6:21 PM, Andrei Vagin <[email protected]> wrote:
> > On Fri, Oct 20, 2017 at 05:06:47PM +0100, Gargi Sharma wrote:
> >> On Thu, Oct 19, 2017 at 5:18 PM, Oleg Nesterov <[email protected]> wrote:
> >> >
> >> > unsigned int last;
> >> > int err;
> >> >
> >> > tmp.data = &last;
> >> > err = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
> >> > if (!err)
> >> > idr_set_cursor(&pid_ns->idr, last + 1);
> >> > return err;
> >> I'm not sure entirely understand how this takes care of rolling over of
> >> PIDs?
> >> Can we ignore that? If yes, won't the tests for CRIU still break?
> >
> > Gargi, I don't understand what you mean. Could you elaborate? Do you
> > mean a case when idr_next is bigger than pid_max? I think this logic
> > remains the same what we had before switching to idr.
>
> When the PIDs are allocated, if the allocation exceeds pid_max wraps
> around and starts allocating PIDs starting from pid_min.
You misunderstood the problem introduced by your patch...
We do not care about pid_max overlap. The problem is that criu writes
to /proc/sys/kernel/ns_last_pid to control the pid number allocated by
the next fork().
So if you do "echo 100 > /proc/sys/kernel/ns_last_pid" the next fork()
should create the process with tid=101 (of course, if 101 is free).
After your patch the new task will have tid=100. See?
> > CRIU tests works with a following patch. It is slightly modified version
> > of Oleg's patch.
Andrei, could you write the changelog and send the fix to akpm? Feel free
to add my ack or sob.
Oleg.
> > diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> > index fea2c24..1c791b3 100644
> > --- a/kernel/pid_namespace.c
> > +++ b/kernel/pid_namespace.c
> > @@ -287,6 +287,7 @@ static int pid_ns_ctl_handler(struct ctl_table
> > *table, int write,
> > {
> > struct pid_namespace *pid_ns = task_active_pid_ns(current);
> > struct ctl_table tmp = *table;
> > + int ret;
> >
> > if (write && !ns_capable(pid_ns->user_ns, CAP_SYS_ADMIN))
> > return -EPERM;
> > @@ -298,7 +299,12 @@ static int pid_ns_ctl_handler(struct ctl_table
> > *table, int write,
> > */
> >
> > tmp.data = &pid_ns->idr.idr_next;
> > - return proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
> > + ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
> > + if (ret < 0)
> > + return ret;
> > +
> > + idr_set_cursor(&pid_ns->idr, pid_ns->idr.idr_next + 1);
> > + return 0;
> > }
> >
> > extern int pid_max;
> >
> >
> >>
> >> Thanks,
> >> Gargi
> >> >
> >> > Oleg.
> >> >
