Commit d919a1e7 authored Jul 13, 2022 by Zhihao Cheng Committed by Andrew Morton Jul 17, 2022

proc: fix a dentry lock race between release_task and lookup

Commit 7bc3e6e5 ("proc: Use a list of inodes to flush from proc")
moved proc_flush_task() behind __exit_signal().  Then, process systemd can
take long period high cpu usage during releasing task in following
concurrent processes:

  systemd                                 ps
kernel_waitid                 stat(/proc/tgid)
  do_wait                       filename_lookup
    wait_consider_task            lookup_fast
      release_task
        __exit_signal
          __unhash_process
            detach_pid
              __change_pid // remove task->pid_links
                                     d_revalidate -> pid_revalidate  // 0
                                     d_invalidate(/proc/tgid)
                                       shrink_dcache_parent(/proc/tgid)
                                         d_walk(/proc/tgid)
                                           spin_lock_nested(/proc/tgid/fd)
                                           // iterating opened fd
        proc_flush_pid                                    |
           d_invalidate (/proc/tgid/fd)                   |
              shrink_dcache_parent(/proc/tgid/fd)         |
                shrink_dentry_list(subdirs)               ↓
                  shrink_lock_dentry(/proc/tgid/fd) --> race on dentry lock

Function d_invalidate() will remove dentry from hash firstly, but why does
proc_flush_pid() process dentry '/proc/tgid/fd' before dentry
'/proc/tgid'?  That's because proc_pid_make_inode() adds proc inode in
reverse order by invoking hlist_add_head_rcu().  But proc should not add
any inodes under '/proc/tgid' except '/proc/tgid/task/pid', fix it by
adding inode into 'pid->inodes' only if the inode is /proc/tgid or
/proc/tgid/task/pid.

Performance regression:
Create 200 tasks, each task open one file for 50,000 times. Kill all
tasks when opened files exceed 10,000,000 (cat /proc/sys/fs/file-nr).

Before fix:
$ time killall -wq aa
  real    4m40.946s   # During this period, we can see 'ps' and 'systemd'
			taking high cpu usage.

After fix:
$ time killall -wq aa
  real    1m20.732s   # During this period, we can see 'systemd' taking
			high cpu usage.

Link: https://lkml.kernel.org/r/20220713130029.4133533-1-chengzhihao1@huawei.com
Fixes: 7bc3e6e5 ("proc: Use a list of inodes to flush from proc")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216054


Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Suggested-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Baoquan He <bhe@redhat.com>
Cc: Kalesh Singh <kaleshsingh@google.com>
Cc: Yu Kuai <yukuai3@huawei.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

parent 7ffe4e90

fs/proc/base.c

+38 −8

Original line number	Diff line number	Diff line
		@@ -1914,11 +1914,6 @@ struct inode proc_pid_make_inode(struct super_block sb,

		/* Let the pid remember us for quick removal */
		ei->pid = pid;
		if (S_ISDIR(mode)) {
		spin_lock(&pid->lock);
		hlist_add_head_rcu(&ei->sibling_inodes, &pid->inodes);
		spin_unlock(&pid->lock);
		}

		task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid);
		security_task_to_inode(task, inode);
		@@ -1931,6 +1926,39 @@ struct inode proc_pid_make_inode(struct super_block sb,
		return NULL;
		}

		/*
		* Generating an inode and adding it into @pid->inodes, so that task will
		* invalidate inode's dentry before being released.
		*
		* This helper is used for creating dir-type entries under '/proc' and
		* '/proc/<tgid>/task'. Other entries(eg. fd, stat) under '/proc/<tgid>'
		* can be released by invalidating '/proc/<tgid>' dentry.
		* In theory, dentries under '/proc/<tgid>/task' can also be released by
		* invalidating '/proc/<tgid>' dentry, we reserve it to handle single
		* thread exiting situation: Any one of threads should invalidate its
		* '/proc/<tgid>/task/<pid>' dentry before released.
		*/
		static struct inode proc_pid_make_base_inode(struct super_block sb,
		struct task_struct *task, umode_t mode)
		{
		struct inode *inode;
		struct proc_inode *ei;
		struct pid *pid;

		inode = proc_pid_make_inode(sb, task, mode);
		if (!inode)
		return NULL;

		/* Let proc_flush_pid find this directory inode */
		ei = PROC_I(inode);
		pid = ei->pid;
		spin_lock(&pid->lock);
		hlist_add_head_rcu(&ei->sibling_inodes, &pid->inodes);
		spin_unlock(&pid->lock);

		return inode;
		}

		int pid_getattr(struct user_namespace mnt_userns, const struct path path,
		struct kstat *stat, u32 request_mask, unsigned int query_flags)
		{
		@@ -3369,7 +3397,8 @@ static struct dentry proc_pid_instantiate(struct dentry dentry,
		{
		struct inode *inode;

		inode = proc_pid_make_inode(dentry->d_sb, task, S_IFDIR \| S_IRUGO \| S_IXUGO);
		inode = proc_pid_make_base_inode(dentry->d_sb, task,
		S_IFDIR \| S_IRUGO \| S_IXUGO);
		if (!inode)
		return ERR_PTR(-ENOENT);

		@@ -3671,7 +3700,8 @@ static struct dentry proc_task_instantiate(struct dentry dentry,
		struct task_struct task, const void ptr)
		{
		struct inode *inode;
		inode = proc_pid_make_inode(dentry->d_sb, task, S_IFDIR \| S_IRUGO \| S_IXUGO);
		inode = proc_pid_make_base_inode(dentry->d_sb, task,
		S_IFDIR \| S_IRUGO \| S_IXUGO);
		if (!inode)
		return ERR_PTR(-ENOENT);