lkml.org 
[lkml]   [2017]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/2] pid_ns: Introduce ioctl to set vector of ns_last_pid's on ns hierarhy
    On 04/27, Kirill Tkhai wrote:
    >
    > On 27.04.2017 19:12, Oleg Nesterov wrote:
    > > On 04/26, Kirill Tkhai wrote:
    > >>
    > >> On 26.04.2017 18:53, Oleg Nesterov wrote:
    > >>>
    > >>>> +static long set_last_pid_vec(struct pid_namespace *pid_ns,
    > >>>> + struct pidns_ioc_req *req)
    > >>>> +{
    > >>>> + char *str, *p;
    > >>>> + int ret = 0;
    > >>>> + pid_t pid;
    > >>>> +
    > >>>> + read_lock(&tasklist_lock);
    > >>>> + if (!pid_ns->child_reaper)
    > >>>> + ret = -EINVAL;
    > >>>> + read_unlock(&tasklist_lock);
    > >>>> + if (ret)
    > >>>> + return ret;
    > >>>
    > >>> why do you need to check ->child_reaper under tasklist_lock? this looks pointless.
    > >>>
    > >>> In fact I do not understand how it is possible to hit pid_ns->child_reaper == NULL,
    > >>> there must be at least one task in this namespace, otherwise you can't open a file
    > >>> which has f_op == ns_file_operations, no?
    > >>
    > >> Sure, it's impossible to pick a pid_ns, if there is no the pid_ns's tasks. I added
    > >> it under impression of
    > >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=dfda351c729733a401981e8738ce497eaffcaa00
    > >> but here it's completely wrong. It will be removed in v2.
    > >
    > > Hmm. But if I read this commit correctly then we really need to check
    > > pid_ns->child_reaper != NULL ?
    > >
    > > Currently we can't pick an "empty" pid_ns. But after the commit above a task
    > > can do sys_unshare(CLONE_NEWPID), another (or the same) task can open its
    > > /proc/$pid/ns/pid_for_children and call ns_ioctl() before the 1st alloc_pid() ?
    >
    > Another task can't open /proc/$pid/ns/pid_for_children before the 1st alloc_pid(),
    > because pid_for_children is available to open only after the 1st alloc_pid().
    > So, it's impossible to call ioctl() on it.

    Ah, OK, I didn't notice the ns->child_reaper check in pidns_for_children_get().

    But note that it doesn't need tasklist_lock too.

    Oleg.

    \
     
     \ /
      Last update: 2017-04-27 18:23    [W:6.606 / U:0.536 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site