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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 23 Oct 2017 17:31:36 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     Gargi Sharma <gs051095@...il.com>
Cc:     Andrei Vagin <avagin@...tuozzo.com>, linux-kernel@...r.kernel.org,
        Rik van Riel <riel@...riel.com>,
        Julia Lawall <julia.lawall@...6.fr>,
        Andrew Morton <akpm@...ux-foundation.org>, mingo@...nel.org,
        pasha.tatashin@...cle.com, ktkhai@...tuozzo.com,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Christoph Hellwig <hch@...radead.org>, lkp@...el.com,
        tony.luck@...el.com
Subject: Re: [v6,1/2] pid: Replace pid bitmap implementation with IDR API

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