| From: Zhihao Cheng <chengzhihao1@huawei.com> |
| Subject: proc: fix a dentry lock race between release_task and lookup |
| Date: Wed, 13 Jul 2022 21:00:29 +0800 |
| |
| Commit 7bc3e6e55acf06 ("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: 7bc3e6e55acf06 ("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> |
| --- |
| |
| fs/proc/base.c | 46 ++++++++++++++++++++++++++++++++++++++-------- |
| 1 file changed, 38 insertions(+), 8 deletions(-) |
| |
| --- a/fs/proc/base.c~proc-fix-a-dentry-lock-race-between-release_task-and-lookup |
| +++ a/fs/proc/base.c |
| @@ -1885,7 +1885,7 @@ void proc_pid_evict_inode(struct proc_in |
| put_pid(pid); |
| } |
| |
| -struct inode *proc_pid_make_inode(struct super_block * sb, |
| +struct inode *proc_pid_make_inode(struct super_block *sb, |
| struct task_struct *task, umode_t mode) |
| { |
| struct inode * inode; |
| @@ -1914,11 +1914,6 @@ struct inode *proc_pid_make_inode(struct |
| |
| /* 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 @@ out_unlock: |
| 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_instantia |
| { |
| 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_instanti |
| 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); |
| |
| _ |
