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.
Signed-off-by: Ian Kent <[email protected]>
---
fs/autofs4/autofs_i.h | 18 ++++++++++++++++++
fs/autofs4/root.c | 16 ++++------------
2 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index 908e184..4c3f589 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -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);
diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index 91b1165..f39bf4b 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -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;
On Thu, Nov 15, 2012 at 8:15 PM, Ian Kent <[email protected]> wrote:
>
> +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
On Fri, 2012-11-16 at 07:45 -0800, Linus Torvalds wrote:
> On Thu, Nov 15, 2012 at 8:15 PM, Ian Kent <[email protected]> wrote:
> >
> > +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.
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
On Fri, Nov 16, 2012 at 8:36 AM, Ian Kent <[email protected]> wrote:
>
> 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?
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
On Fri, Nov 16, 2012 at 08:43:28AM -0800, Linus Torvalds wrote:
> On Fri, Nov 16, 2012 at 8:36 AM, Ian Kent <[email protected]> wrote:
> >
> > 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?
>
> 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?
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?
On Fri, 2012-11-16 at 17:34 +0000, Al Viro wrote:
> On Fri, Nov 16, 2012 at 08:43:28AM -0800, Linus Torvalds wrote:
> > On Fri, Nov 16, 2012 at 8:36 AM, Ian Kent <[email protected]> wrote:
> > >
> > > 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?
> >
> > 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?
>
> 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?
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