* [PATCH v3] proc: Fix a dentry lock race between release_task and lookup
@ 2022-06-01 2:36 Zhihao Cheng
2022-06-01 5:44 ` kernel test robot
0 siblings, 1 reply; 2+ messages in thread
From: Zhihao Cheng @ 2022-06-01 2:36 UTC (permalink / raw)
To: ebiederm; +Cc: linux-kernel, linux-fsdevel, chengzhihao1, yukuai3, yi.zhang
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.
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.
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..f22fad3bf7f0 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1885,7 +1885,7 @@ void proc_pid_evict_inode(struct proc_inode *ei)
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 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,27 @@ struct inode *proc_pid_make_inode(struct super_block * sb,
return NULL;
}
+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)
{
@@ -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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v3] proc: Fix a dentry lock race between release_task and lookup
2022-06-01 2:36 [PATCH v3] proc: Fix a dentry lock race between release_task and lookup Zhihao Cheng
@ 2022-06-01 5:44 ` kernel test robot
0 siblings, 0 replies; 2+ messages in thread
From: kernel test robot @ 2022-06-01 5:44 UTC (permalink / raw)
To: Zhihao Cheng, ebiederm
Cc: llvm, kbuild-all, linux-kernel, linux-fsdevel, chengzhihao1,
yukuai3, yi.zhang
Hi Zhihao,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.18 next-20220601]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Zhihao-Cheng/proc-Fix-a-dentry-lock-race-between-release_task-and-lookup/20220601-102513
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 700170bf6b4d773e328fa54ebb70ba444007c702
config: x86_64-randconfig-a005 (https://download.01.org/0day-ci/archive/20220601/202206011314.dh4oSNjS-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project c825abd6b0198fb088d9752f556a70705bc99dfd)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/9dfec748ded82f145043dc08a944e1a9af7a640d
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Zhihao-Cheng/proc-Fix-a-dentry-lock-race-between-release_task-and-lookup/20220601-102513
git checkout 9dfec748ded82f145043dc08a944e1a9af7a640d
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash fs/proc/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> fs/proc/base.c:1929:15: warning: no previous prototype for function 'proc_pid_make_base_inode' [-Wmissing-prototypes]
struct inode *proc_pid_make_base_inode(struct super_block *sb,
^
fs/proc/base.c:1929:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
struct inode *proc_pid_make_base_inode(struct super_block *sb,
^
static
1 warning generated.
vim +/proc_pid_make_base_inode +1929 fs/proc/base.c
1928
> 1929 struct inode *proc_pid_make_base_inode(struct super_block *sb,
1930 struct task_struct *task, umode_t mode)
1931 {
1932 struct inode *inode;
1933 struct proc_inode *ei;
1934 struct pid *pid;
1935
1936 inode = proc_pid_make_inode(sb, task, mode);
1937 if (!inode)
1938 return NULL;
1939
1940 /* Let proc_flush_pid find this directory inode */
1941 ei = PROC_I(inode);
1942 pid = ei->pid;
1943 spin_lock(&pid->lock);
1944 hlist_add_head_rcu(&ei->sibling_inodes, &pid->inodes);
1945 spin_unlock(&pid->lock);
1946
1947 return inode;
1948 }
1949
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-06-01 5:44 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-01 2:36 [PATCH v3] proc: Fix a dentry lock race between release_task and lookup Zhihao Cheng
2022-06-01 5:44 ` kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.