Messages in this thread | | | Subject | Re: [PATCH v4] proc: Fix a dentry lock race between release_task and lookup | From | Zhihao Cheng <> | Date | Wed, 13 Jul 2022 15:24:50 +0800 |
| |
在 2022/7/12 22:16, Brian Foster 写道: > On Wed, Jun 01, 2022 at 02:23:32PM +0800, Zhihao Cheng wrote: >> 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 >> > > Curious... can this same sort of thing happen with /proc/<tgid>/task if > that dir similarly has a lot of dentries? >
Yes. It could happend too. There will be many dentries under /proc/<tgid>/task when there are many tasks under same thread group.
We must put /proc/<tgid>/task into pid->inodes, because we have to handle single thread exiting situation: Any one of threads should invalidate its /proc/<tgid>/task/<pid> dentry before begin released. You may refer to the function proc_flush_task_mnt() before commit 7bc3e6e55acf06 ("proc: Use a list of inodes to flush from proc").
> ... >> 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> >> --- >> v1->v2: Add new helper proc_pid_make_base_inode that performs the extra >> work of adding to the pid->list. >> v2->v3: Add performance regression in commit message. >> v3->v4: Make proc_pid_make_base_inode() static >> fs/proc/base.c | 34 ++++++++++++++++++++++++++-------- >> 1 file changed, 26 insertions(+), 8 deletions(-) >> >> diff --git a/fs/proc/base.c b/fs/proc/base.c >> index c1031843cc6a..d884933950fd 100644 >> --- a/fs/proc/base.c >> +++ b/fs/proc/base.c > ... >> @@ -1931,6 +1926,27 @@ struct inode *proc_pid_make_inode(struct super_block * sb, >> return NULL; >> } >> >> +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; >> +} >> + > > Somewhat related to the question above.. it would be nice if this > wrapper had a line or two comment above it that explained when it should > or shouldn't be used over the underlying function (for example, why or > why not include /proc/<tgid>/task?). Otherwise the patch overall seems > reasonable to me.. >
Thanks for advice, I will add some notes in v5. > Brian > >> int pid_getattr(struct user_namespace *mnt_userns, const struct path *path, >> struct kstat *stat, u32 request_mask, unsigned int query_flags) >> { >> @@ -3350,7 +3366,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); >> >> @@ -3649,7 +3666,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); >> >> -- >> 2.31.1 >> > > . >
| |