autofs4 - use simple_empty() for empty directory check
Subsystems (2)
PATCH 1/1 autofs4 - use simple_empty() for empty directory check
From: Ian Kent <[email protected]>
For direct (and offset) mounts, if an automounted mount is manually umounted the trigger mount dentry can appear non-empty causing it to not trigger mounts. This can also happen if there is a file handle leak in a user space automounting application.
It happens because, when the ioctl control file handle is opened on the mount, a cursor dentry is created which causes list_empty() to see the dentry as non-empty. Since there is a case where listing the directory of these dentrys is needed, the use of dcache_dir_*() functions for .open() and .release() is needed.
Consequently simple_empty() must be used instead of list_empty() when checking for an empty directory.
---
fs/autofs4/autofs_i.h | 18 ++++++++++++++++++ fs/autofs4/root.c | 16 ++++------------ 2 files changed, 22 insertions(+), 12 deletions(-)
fs/autofs4/autofs_i.h +18 -0
@@ -301,6 +301,24 @@ static inline int simple_positive(struct dentry *dentry)return dentry->d_inode && !d_unhashed (dentry); } +static inline int __simple_empty (struct dentry *dentry) +{ +struct dentry *child; +int ret =0 ; + +list_for_each_entry (child, &dentry->d_subdirs, d_u.d_child) { +spin_lock_nested (&child->d_lock,DENTRY_D_LOCK_NESTED ); +if (simple_positive (child)) { +spin_unlock (&child->d_lock); + goto out; + } +spin_unlock (&child->d_lock); + } + ret =1 ; +out: +return ret; +} +static inline void __autofs4_add_expiring (struct dentry *dentry) {struct autofs_sb_info *sbi =autofs4_sbi (dentry->d_sb);
fs/autofs4/root.c +4 -12
@@ -124,13 +124,10 @@ static int autofs4_dir_open(struct inode *inode, struct file *file)* it. */ spin_lock (&sbi->lookup_lock); -spin_lock (&dentry->d_lock); -if (!d_mountpoint (dentry) &&list_empty (&dentry->d_subdirs)) { -spin_unlock (&dentry->d_lock); +if (!d_mountpoint (dentry) &&simple_empty (dentry)) {spin_unlock (&sbi->lookup_lock);return -ENOENT ; } -spin_unlock (&dentry->d_lock);spin_unlock (&sbi->lookup_lock); out:
@@ -382,12 +379,8 @@ static struct vfsmount *autofs4_d_automount(struct path *path)if (have_submounts (dentry)) goto done; }else { -spin_lock (&dentry->d_lock); -if (!list_empty (&dentry->d_subdirs)) { -spin_unlock (&dentry->d_lock); +if (!simple_empty (dentry)) goto done; - } -spin_unlock (&dentry->d_lock); } ino->flags |=AUTOFS_INF_PENDING ;spin_unlock (&sbi->fs_lock);
@@ -413,8 +406,7 @@ done:* the follow. */ spin_lock (&dentry->d_lock); -if ((!d_mountpoint (dentry) && - !list_empty (&dentry->d_subdirs)) || +if ((!d_mountpoint (dentry) && !__simple_empty (dentry)) || (dentry->d_inode &&S_ISLNK (dentry->d_inode->i_mode)))__managed_dentry_clear_automount (dentry);spin_unlock (&dentry->d_lock);
@@ -673,7 +665,7 @@ static int autofs4_dir_rmdir(struct inode *dir, struct dentry *dentry)spin_lock (&sbi->lookup_lock);spin_lock (&dentry->d_lock); -if (!list_empty (&dentry->d_subdirs)) { +if (!__simple_empty (dentry)) {spin_unlock (&dentry->d_lock);spin_unlock (&sbi->lookup_lock);return -ENOTEMPTY ;
fs/autofs4/autofs_i.h static inline int simple_positive(struct dentry *dentry) · line 305
Quoted 3 lines · Ian Kent
fs/autofs4/autofs_i.h +2
-0+static inline int __simple_empty (struct dentry *dentry) +{
This seems completely bogus.
It's a duplicate of the existing fs/libfs.c "simple_empty()" function, but without taking the outer lock.
That kind of code duplication - and doing it in a totally different file, and making it look autofs-specific - seems entirely wrong.
Linus
Quoted 7 lines · Linus Torvalds
fs/autofs4/autofs_i.h static inline int simple_positive(struct dentry *dentry) · line 305
Quoted 3 lines · Ian Kent
fs/autofs4/autofs_i.h +2
-0+static inline int __simple_empty (struct dentry *dentry) +{This seems completely bogus.
Quoted 5 lines
It's a duplicate of the existing
fs/libfs.c"simple_empty()" function, but without taking the outer lock.That kind of code duplication - and doing it in a totally different file, and making it look autofs-specific - seems entirely wrong.
Sure, are you recommending I alter the fs/libfs.c functions to add a function that doesn't have the outer lock, and have simple_empty() call that, then use it in autofs?
Or are recommending I rework the autofs code?
Ian
Quoted 4 lines · Ian Kent
Sure, are you recommending I alter the
fs/libfs.cfunctions to add a function that doesn't have the outer lock, and havesimple_empty()call that, then use it in autofs?
Yup. That's the standard pattern, although usually we *strive* to make the unlocked versions be static to the internal code, and then use them there for the various helpers. In your case that seems impossible, since you do depend on holding the d_lock in the caller after the tests. But at least we don't have to duplicate the code and have it in two unrelated places.
Al? Comments?
Linus
Al Viro
maintainerQuoted 6 lines · Linus Torvalds
Quoted 4 lines · Ian Kent
Sure, are you recommending I alter the
fs/libfs.cfunctions to add a function that doesn't have the outer lock, and havesimple_empty()call that, then use it in autofs?
Quoted 8 lines
Yup. That's the standard pattern, although usually we *strive* to make the unlocked versions be static to the internal code, and then use them there for the various helpers. In your case that seems impossible, since you do depend on holding the
d_lockin the caller after the tests. But at least we don't have to duplicate the code and have it in two unrelated places.Al? Comments?
The thing is, I'm not convinced we really need ->d_lock held downstream. E.g. __autofs4_add_expiring() ought to be OK with just sbi->lookup_lock. Not sure about the situation in autofs4_d_automount() - the thing is messy as hell ;-/
Ian, do we really need that __simple_empty() variant in either caller? What is getting protected by ->d_lock after it and do we really need ->d_lock continuously held for that?
Quoted 16 lines · Al Viro
Quoted 6 lines · Linus Torvalds
Quoted 4 lines · Ian Kent
Sure, are you recommending I alter the
fs/libfs.cfunctions to add a function that doesn't have the outer lock, and havesimple_empty()call that, then use it in autofs?Quoted 8 lines
Yup. That's the standard pattern, although usually we *strive* to make the unlocked versions be static to the internal code, and then use them there for the various helpers. In your case that seems impossible, since you do depend on holding the
d_lockin the caller after the tests. But at least we don't have to duplicate the code and have it in two unrelated places.Al? Comments?
Quoted 8 lines
The thing is, I'm not convinced we really need ->d_lock held downstream. E.g.
__autofs4_add_expiring()ought to be OK with justsbi->lookup_lock. Not sure about the situation inautofs4_d_automount()- the thing is messy as hell ;-/Ian, do we really need that
__simple_empty()variant in either caller? What is getting protected by ->d_lock after it and do we really need ->d_lock continuously held for that?
Yeah, I've thought about that a few times now but haven't gone so far as to change it.
I'll have another look.
Ian