[PATCH] autofs4 - fix get_next_positive_subdir()

[PATCH] autofs4 - fix get_next_positive_subdir()

[Ian Kent]

From: Ian Kent <raven@themaw.net>

The locking for the list traversal in get_next_positive_subdir() is
wrong, so fix it.

Signed-off-by: Ian Kent <raven@themaw.net>
---

 fs/autofs4/expire.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c
index 1feb68e..20cc9fd 100644
--- a/fs/autofs4/expire.c
+++ b/fs/autofs4/expire.c
@@ -97,9 +97,9 @@ static struct dentry *get_next_positive_subdir(struct dentry *prev,
 	struct dentry *p, *q;
 
 	spin_lock(&sbi->lookup_lock);
+	spin_lock(&root->d_lock);
 
 	if (prev == NULL) {
-		spin_lock(&root->d_lock);
 		prev = dget_dlock(root);
 		next = prev->d_subdirs.next;
 		p = prev;
@@ -107,12 +107,13 @@ static struct dentry *get_next_positive_subdir(struct dentry *prev,
 	}
 
 	p = prev;
-	spin_lock(&p->d_lock);
+	spin_lock_nested(&p->d_lock, DENTRY_D_LOCK_NESTED);
 again:
 	next = p->d_u.d_child.next;
 start:
 	if (next == &root->d_subdirs) {
 		spin_unlock(&p->d_lock);
+		spin_unlock(&root->d_lock);
 		spin_unlock(&sbi->lookup_lock);
 		dput(prev);
 		return NULL;
@@ -121,8 +122,8 @@ start:
 	q = list_entry(next, struct dentry, d_u.d_child);
 
 	spin_lock_nested(&q->d_lock, DENTRY_D_LOCK_NESTED);
-	/* Negative dentry - try next */
-	if (!simple_positive(q)) {
+	/* Negative dentry or already gone - try next */
+	if (q->d_count == 0 || !simple_positive(q)) {
 		spin_unlock(&p->d_lock);
 		lock_set_subclass(&q->d_lock.dep_map, 0, _RET_IP_);
 		p = q;
@@ -131,6 +132,7 @@ start:
 	dget_dlock(q);
 	spin_unlock(&q->d_lock);
 	spin_unlock(&p->d_lock);
+	spin_unlock(&root->d_lock);
 	spin_unlock(&sbi->lookup_lock);
 
 	dput(prev);

Re: [PATCH] autofs4 - fix get_next_positive_subdir()

[Linus Torvalds]

On Tue, May 22, 2012 at 7:49 PM, Ian Kent <ikent@redhat.com> wrote:
>
> The locking for the list traversal in get_next_positive_subdir() is
> wrong, so fix it.

As an explanation, this kind of thing is totally useless. It doesn't
actually give any information at all. It's like saying "change
locking"

What happened, and why? Why is the new nested spinlock ok and won't
deadlock against other nested users? Wazzup?

                 Linus

Re: [PATCH] autofs4 - fix get_next_positive_subdir()

[Ian Kent]

On Tue, 2012-05-22 at 19:53 -0700, Linus Torvalds wrote:
> On Tue, May 22, 2012 at 7:49 PM, Ian Kent <ikent@redhat.com> wrote:
> >
> > The locking for the list traversal in get_next_positive_subdir() is
> > wrong, so fix it.
> 
> As an explanation, this kind of thing is totally useless. It doesn't
> actually give any information at all. It's like saying "change
> locking"

OK, I'll fix that and re-submit.

Ian

Re: [PATCH] autofs4 - fix get_next_positive_subdir()

[Ian Kent]

On Tue, 2012-05-22 at 19:53 -0700, Linus Torvalds wrote:
> On Tue, May 22, 2012 at 7:49 PM, Ian Kent <ikent@redhat.com> wrote:
> >
> > The locking for the list traversal in get_next_positive_subdir() is
> > wrong, so fix it.
> 
> As an explanation, this kind of thing is totally useless. It doesn't
> actually give any information at all. It's like saying "change
> locking"
> 
> What happened, and why? Why is the new nested spinlock ok and won't
> deadlock against other nested users? Wazzup?

It's good that you questioned this Linus.

Looking again at dput() I think the traversal still isn't quite right.

For a start the test for d_count 0 or positive and hashed can never be
true since the point of the change was to take the d_lock of the
d_subdirs dentry for the traversal.

I'll need to work on this some more, thanks.
Ian

Re: [PATCH] autofs4 - fix get_next_positive_subdir()

[Ian Kent]

On Wed, 2012-05-23 at 15:22 +0800, Ian Kent wrote:
> On Tue, 2012-05-22 at 19:53 -0700, Linus Torvalds wrote:
> > On Tue, May 22, 2012 at 7:49 PM, Ian Kent <ikent@redhat.com> wrote:
> > >
> > > The locking for the list traversal in get_next_positive_subdir() is
> > > wrong, so fix it.
> > 
> > As an explanation, this kind of thing is totally useless. It doesn't
> > actually give any information at all. It's like saying "change
> > locking"
> > 
> > What happened, and why? Why is the new nested spinlock ok and won't
> > deadlock against other nested users? Wazzup?
> 
> It's good that you questioned this Linus.
> 
> Looking again at dput() I think the traversal still isn't quite right.
> 
> For a start the test for d_count 0 or positive and hashed can never be

Correction, that second check is actually !(positive and hashed) in the
code.

> true since the point of the change was to take the d_lock of the
> d_subdirs dentry for the traversal.
> 
> I'll need to work on this some more, thanks.
> Ian

Re: [PATCH] autofs4 - fix get_next_positive_subdir()

[Ian Kent]

On Wed, 2012-05-23 at 15:30 +0800, Ian Kent wrote:
> On Wed, 2012-05-23 at 15:22 +0800, Ian Kent wrote:
> > On Tue, 2012-05-22 at 19:53 -0700, Linus Torvalds wrote:
> > > On Tue, May 22, 2012 at 7:49 PM, Ian Kent <ikent@redhat.com> wrote:
> > > >
> > > > The locking for the list traversal in get_next_positive_subdir() is
> > > > wrong, so fix it.
> > > 
> > > As an explanation, this kind of thing is totally useless. It doesn't
> > > actually give any information at all. It's like saying "change
> > > locking"
> > > 
> > > What happened, and why? Why is the new nested spinlock ok and won't
> > > deadlock against other nested users? Wazzup?
> > 
> > It's good that you questioned this Linus.
> > 
> > Looking again at dput() I think the traversal still isn't quite right.
> > 
> > For a start the test for d_count 0 or positive and hashed can never be
> 
> Correction, that second check is actually !(positive and hashed) in the
> code.
> 
> > true since the point of the change was to take the d_lock of the
> > d_subdirs dentry for the traversal.

That's not right, ignore this.

> > 
> > I'll need to work on this some more, thanks.
> > Ian
> 

[PATCH] autofs4 - fix get_next_positive_subdir()

[Ian Kent]

From: Ian Kent <raven@themaw.net>

Following a report of a crash during an automount expire I found that
the locking in fs/autofs4/expire.c:get_next_positive_subdir() was wrong.
Not only is the locking wrong but the function is more complex than it
needs to be.

The function is meant to calculate (and dget) the next entry in the list
of directories contained in the root of an autofs mount point (an autofs
indirect mount to be precise). The main problem was that the d_lock of
the owner of the list was not being taken when walking the list, which
lead to list corruption under load. The only other lock that needs to
be taken is against the next dentry candidate so it can be checked for
usability.

Signed-off-by: Ian Kent <raven@themaw.net>
---

 fs/autofs4/expire.c |   31 +++++++++++++------------------
 1 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c
index 1feb68e..8c0e56d 100644
--- a/fs/autofs4/expire.c
+++ b/fs/autofs4/expire.c
@@ -94,25 +94,21 @@ static struct dentry *get_next_positive_subdir(struct dentry *prev,
 {
 	struct autofs_sb_info *sbi = autofs4_sbi(root->d_sb);
 	struct list_head *next;
-	struct dentry *p, *q;
+	struct dentry *q;
 
 	spin_lock(&sbi->lookup_lock);
+	spin_lock(&root->d_lock);
 
-	if (prev == NULL) {
-		spin_lock(&root->d_lock);
+	if (prev)
+		next = prev->d_u.d_child.next;
+	else {
 		prev = dget_dlock(root);
 		next = prev->d_subdirs.next;
-		p = prev;
-		goto start;
 	}
 
-	p = prev;
-	spin_lock(&p->d_lock);
-again:
-	next = p->d_u.d_child.next;
-start:
+cont:
 	if (next == &root->d_subdirs) {
-		spin_unlock(&p->d_lock);
+		spin_unlock(&root->d_lock);
 		spin_unlock(&sbi->lookup_lock);
 		dput(prev);
 		return NULL;
@@ -121,16 +117,15 @@ start:
 	q = list_entry(next, struct dentry, d_u.d_child);
 
 	spin_lock_nested(&q->d_lock, DENTRY_D_LOCK_NESTED);
-	/* Negative dentry - try next */
-	if (!simple_positive(q)) {
-		spin_unlock(&p->d_lock);
-		lock_set_subclass(&q->d_lock.dep_map, 0, _RET_IP_);
-		p = q;
-		goto again;
+	/* Already gone or negative dentry (under construction) - try next */
+	if (q->d_count == 0 || !simple_positive(q)) {
+		spin_unlock(&q->d_lock);
+		next = q->d_u.d_child.next;
+		goto cont;
 	}
 	dget_dlock(q);
 	spin_unlock(&q->d_lock);
-	spin_unlock(&p->d_lock);
+	spin_unlock(&root->d_lock);
 	spin_unlock(&sbi->lookup_lock);
 
 	dput(prev);