[lkml]   [2010]   [Jun]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [RESEND PATCH] pid_ns: Fix proc_flush_task() accessing freed proc_mnt
    On 06/25, Louis Rilling wrote:
    > On 24/06/10 21:18 +0200, Oleg Nesterov wrote:
    > >
    > > and this adds the extra code to alloc/free pidmap.
    > Hopefully this is not much in alloc_pidmap() since we can expect that nr_pids
    > and last_pid are in the same cache line.

    This also adds atomic op. But I mostly dislike the pid-ns-specific
    complications itself (including the mount-after-the-first-alloc_pid
    dependancy), not the minor perfomance penalty.

    But see below...

    > > And, this subjective, yes, but it looks a bit strange that upid->nr
    > > has a reference to proc_mnt.
    > I presume that you wanted to say upid->ns.

    I meant ns->nr_pids ;)

    > This last point is what made me worry about your approach so far, although I did
    > not take time to spot the precise issues. Unfortunately I don't see what the
    > checks you added in proc_self_readlink(), proc_self_follow_link() and
    > proc_pid_lookup() buy. What does prevent destroy_pid_namespace() from running
    > concurrently? Maybe RCU could help in those cases?

    Very good point. And the strong argument against this approach.

    > Moreover, I think that proc_pid_readdir() should get some check too.

    Well, it checks ->reaper != NULL, that is why I don't verify ns.

    But yes, we have the same race (races) you pointed out here.

    > void pid_ns_release_proc(struct pid_namespace *ns)
    > {
    > struct inode *root_inode;
    > if (ns->proc_mnt) {
    > root_inode = ns->proc_mnt->mnt_sb->s_root->d_inode;
    > mutex_lock(&root_inode->i_mutex);
    > ns->proc_mnt->mnt_sb->s_fs_info = NULL;
    > PROC_I(root_inode)->pid = NULL;
    > mutex_unlock(&root_inode->i_mutex);
    > mntput(ns->proc_mnt);
    > }
    > }
    > This would also solve the issue for proc_pid_lookup() btw.

    Looks like you are right, but this doesn't help proc_self_readlink().

    I think we can fix all these problems, but I no longer think this
    approach can pretend to simplify the code. No, it will make the
    code more complex/ugly and potentially more buggy.

    Louis, thank you very much.


     \ /
      Last update: 2010-06-25 14:27    [W:0.044 / U:4.112 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site