Re: [v6,1/2] pid: Replace pid bitmap implementation with IDR API

[Oleg Nesterov <oleg@redhat.com>]

On 10/22, Gargi Sharma wrote:
>
> On Fri, Oct 20, 2017 at 6:21 PM, Andrei Vagin <avagin@virtuozzo.com> 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 <oleg@redhat.com> 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.
> >> >