From e509d6e9c1ab54af257d4ed95b30d41e3d786857 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Tue, 17 Sep 2019 22:16:58 -0400 Subject: [PATCH 1/4] autofs_clear_leaf_automount_flags(): use ino->count instead of ->d_subdirs We want to find out if the parent will become empty after we remove the victim of rmdir(). Checking if the victim is the only element of parent's ->d_subdirs is completely wrong - e.g. opening the parent will end up with a cursor added to its ->d_parent and fooling the check. We do maintain ino->count - 0 for anything removed, 1 + number of children for anything live. Which gives us precisely what we need for that check... Signed-off-by: Al Viro --- fs/autofs/root.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/fs/autofs/root.c b/fs/autofs/root.c index 29abafc0ce31..2065281ee8b1 100644 --- a/fs/autofs/root.c +++ b/fs/autofs/root.c @@ -660,7 +660,6 @@ static void autofs_set_leaf_automount_flags(struct dentry *dentry) static void autofs_clear_leaf_automount_flags(struct dentry *dentry) { - struct list_head *d_child; struct dentry *parent; /* flags for dentrys in the root are handled elsewhere */ @@ -673,10 +672,7 @@ static void autofs_clear_leaf_automount_flags(struct dentry *dentry) /* only consider parents below dentrys in the root */ if (IS_ROOT(parent->d_parent)) return; - d_child = &dentry->d_child; - /* Set parent managed if it's becoming empty */ - if (d_child->next == &parent->d_subdirs && - d_child->prev == &parent->d_subdirs) + if (atomic_read(&autofs_dentry_ino(parent)->count) == 2) managed_dentry_set_managed(parent); } From 41ca19740a0e772eff1f9ba67293649feb836662 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Tue, 17 Sep 2019 23:23:08 -0400 Subject: [PATCH 2/4] autofs: get rid of pointless checks around ->count handling * IS_ROOT can't be true for unlink or rmdir victim * any positive autofs dentry has non-NULL autofs_dentry_ino() * autofs symlink can't have ->count other than 1 * autofs empty directory can't have ->count other than 1 Signed-off-by: Al Viro --- fs/autofs/root.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/fs/autofs/root.c b/fs/autofs/root.c index 2065281ee8b1..7806b110e79d 100644 --- a/fs/autofs/root.c +++ b/fs/autofs/root.c @@ -571,8 +571,7 @@ static int autofs_dir_symlink(struct inode *dir, dget(dentry); atomic_inc(&ino->count); p_ino = autofs_dentry_ino(dentry->d_parent); - if (p_ino && !IS_ROOT(dentry)) - atomic_inc(&p_ino->count); + atomic_inc(&p_ino->count); dir->i_mtime = current_time(dir); @@ -610,11 +609,9 @@ static int autofs_dir_unlink(struct inode *dir, struct dentry *dentry) if (sbi->flags & AUTOFS_SBI_CATATONIC) return -EACCES; - if (atomic_dec_and_test(&ino->count)) { - p_ino = autofs_dentry_ino(dentry->d_parent); - if (p_ino && !IS_ROOT(dentry)) - atomic_dec(&p_ino->count); - } + atomic_dec(&ino->count); + p_ino = autofs_dentry_ino(dentry->d_parent); + atomic_dec(&p_ino->count); dput(ino->dentry); d_inode(dentry)->i_size = 0; @@ -706,11 +703,9 @@ static int autofs_dir_rmdir(struct inode *dir, struct dentry *dentry) if (sbi->version < 5) autofs_clear_leaf_automount_flags(dentry); - if (atomic_dec_and_test(&ino->count)) { - p_ino = autofs_dentry_ino(dentry->d_parent); - if (p_ino && dentry->d_parent != dentry) - atomic_dec(&p_ino->count); - } + atomic_dec(&ino->count); + p_ino = autofs_dentry_ino(dentry->d_parent); + atomic_dec(&p_ino->count); dput(ino->dentry); d_inode(dentry)->i_size = 0; clear_nlink(d_inode(dentry)); @@ -758,8 +753,7 @@ static int autofs_dir_mkdir(struct inode *dir, dget(dentry); atomic_inc(&ino->count); p_ino = autofs_dentry_ino(dentry->d_parent); - if (p_ino && !IS_ROOT(dentry)) - atomic_inc(&p_ino->count); + atomic_inc(&p_ino->count); inc_nlink(dir); dir->i_mtime = current_time(dir); From c3aed16680cd0c0e5abf1bfc0dc1338e6a41b29f Mon Sep 17 00:00:00 2001 From: Al Viro Date: Tue, 17 Sep 2019 23:28:08 -0400 Subject: [PATCH 3/4] autofs_dir_rmdir(): check ino->count for deciding whether it's empty... Signed-off-by: Al Viro --- fs/autofs/root.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/fs/autofs/root.c b/fs/autofs/root.c index 7806b110e79d..ae1d112b6f64 100644 --- a/fs/autofs/root.c +++ b/fs/autofs/root.c @@ -691,11 +691,10 @@ static int autofs_dir_rmdir(struct inode *dir, struct dentry *dentry) if (sbi->flags & AUTOFS_SBI_CATATONIC) return -EACCES; - spin_lock(&sbi->lookup_lock); - if (!simple_empty(dentry)) { - spin_unlock(&sbi->lookup_lock); + if (atomic_read(&ino->count) != 1) return -ENOTEMPTY; - } + + spin_lock(&sbi->lookup_lock); __autofs_add_expiring(dentry); d_drop(dentry); spin_unlock(&sbi->lookup_lock); From 850d71acd52cd331474116fbd60cf8b3f3ded93e Mon Sep 17 00:00:00 2001 From: Al Viro Date: Tue, 17 Sep 2019 23:31:27 -0400 Subject: [PATCH 4/4] autofs: don't bother with atomics for ino->count All writers are serialized on inode->i_rwsem. So are the readers outside of expire.c. And the readers in expire.c are in the code that really doesn't care about narrow races - it's looking for expiry candidates and its callers have to cope with the possibility of a good candidate becoming busy right under them. No point bothering with atomic operations - just use int and mark the non-serialized readers with READ_ONCE(). Signed-off-by: Al Viro --- fs/autofs/autofs_i.h | 2 +- fs/autofs/expire.c | 6 +++--- fs/autofs/root.c | 20 ++++++++++---------- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/fs/autofs/autofs_i.h b/fs/autofs/autofs_i.h index 8bcec8dcabb6..054f97b07754 100644 --- a/fs/autofs/autofs_i.h +++ b/fs/autofs/autofs_i.h @@ -63,7 +63,7 @@ struct autofs_info { struct autofs_sb_info *sbi; unsigned long last_used; - atomic_t count; + int count; kuid_t uid; kgid_t gid; diff --git a/fs/autofs/expire.c b/fs/autofs/expire.c index 2866fabf497f..31d616aa6fc9 100644 --- a/fs/autofs/expire.c +++ b/fs/autofs/expire.c @@ -211,7 +211,7 @@ static int autofs_tree_busy(struct vfsmount *mnt, } } else { struct autofs_info *ino = autofs_dentry_ino(p); - unsigned int ino_count = atomic_read(&ino->count); + unsigned int ino_count = READ_ONCE(ino->count); /* allow for dget above and top is already dgot */ if (p == top) @@ -379,7 +379,7 @@ static struct dentry *should_expire(struct dentry *dentry, /* Not a forced expire? */ if (!(how & AUTOFS_EXP_FORCED)) { /* ref-walk currently on this dentry? */ - ino_count = atomic_read(&ino->count) + 1; + ino_count = READ_ONCE(ino->count) + 1; if (d_count(dentry) > ino_count) return NULL; } @@ -396,7 +396,7 @@ static struct dentry *should_expire(struct dentry *dentry, /* Not a forced expire? */ if (!(how & AUTOFS_EXP_FORCED)) { /* ref-walk currently on this dentry? */ - ino_count = atomic_read(&ino->count) + 1; + ino_count = READ_ONCE(ino->count) + 1; if (d_count(dentry) > ino_count) return NULL; } diff --git a/fs/autofs/root.c b/fs/autofs/root.c index ae1d112b6f64..5aaa1732bf1e 100644 --- a/fs/autofs/root.c +++ b/fs/autofs/root.c @@ -569,9 +569,9 @@ static int autofs_dir_symlink(struct inode *dir, d_add(dentry, inode); dget(dentry); - atomic_inc(&ino->count); + ino->count++; p_ino = autofs_dentry_ino(dentry->d_parent); - atomic_inc(&p_ino->count); + p_ino->count++; dir->i_mtime = current_time(dir); @@ -609,9 +609,9 @@ static int autofs_dir_unlink(struct inode *dir, struct dentry *dentry) if (sbi->flags & AUTOFS_SBI_CATATONIC) return -EACCES; - atomic_dec(&ino->count); + ino->count--; p_ino = autofs_dentry_ino(dentry->d_parent); - atomic_dec(&p_ino->count); + p_ino->count--; dput(ino->dentry); d_inode(dentry)->i_size = 0; @@ -669,7 +669,7 @@ static void autofs_clear_leaf_automount_flags(struct dentry *dentry) /* only consider parents below dentrys in the root */ if (IS_ROOT(parent->d_parent)) return; - if (atomic_read(&autofs_dentry_ino(parent)->count) == 2) + if (autofs_dentry_ino(parent)->count == 2) managed_dentry_set_managed(parent); } @@ -691,7 +691,7 @@ static int autofs_dir_rmdir(struct inode *dir, struct dentry *dentry) if (sbi->flags & AUTOFS_SBI_CATATONIC) return -EACCES; - if (atomic_read(&ino->count) != 1) + if (ino->count != 1) return -ENOTEMPTY; spin_lock(&sbi->lookup_lock); @@ -702,9 +702,9 @@ static int autofs_dir_rmdir(struct inode *dir, struct dentry *dentry) if (sbi->version < 5) autofs_clear_leaf_automount_flags(dentry); - atomic_dec(&ino->count); + ino->count--; p_ino = autofs_dentry_ino(dentry->d_parent); - atomic_dec(&p_ino->count); + p_ino->count--; dput(ino->dentry); d_inode(dentry)->i_size = 0; clear_nlink(d_inode(dentry)); @@ -750,9 +750,9 @@ static int autofs_dir_mkdir(struct inode *dir, autofs_set_leaf_automount_flags(dentry); dget(dentry); - atomic_inc(&ino->count); + ino->count++; p_ino = autofs_dentry_ino(dentry->d_parent); - atomic_inc(&p_ino->count); + p_ino->count++; inc_nlink(dir); dir->i_mtime = current_time(dir);