2.6.39-rc7 BUG in fs/namei.c:1362, bisected -- BKL, symlinks, and rename

2.6.39-rc7 BUG in fs/namei.c:1362, bisected -- BKL, symlinks, and rename

[Erez Zadok]

Easily reproducible bug found in v2.6.39-rc7-174-gddb503b. This line in fs/namei.c:nested_symlink() is triggered:

	BUG_ON(nd->depth >= MAX_NESTED_LINKS);

To reproduce: run racer on any f/s.  I narrowed it down to just 'rename' and 'symlink' ops (verified that other ops aren't involved).

Bisection narrowed the bug down to this one commit:

commit f74b9444192c60603020c61d7915b72893137edc
Merge: 7a63628 4ba8216
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Wed Mar 16 17:21:00 2011 -0700

    Merge branch 'config' of git://git.kernel.org/pub/scm/linux/kernel/git/arnd/bkl
    
    * 'config' of git://git.kernel.org/pub/scm/linux/kernel/git/arnd/bkl:
      BKL: That's all, folks
      fs/locks.c: Remove stale FIXME left over from BKL conversion
      ipx: remove the BKL
      appletalk: remove the BKL
      x25: remove the BKL
      ufs: remove the BKL
      hpfs: remove the BKL
      drivers: remove extraneous includes of smp_lock.h
      tracing: don't trace the BKL
      adfs: remove the big kernel lock

I'm happy to test a fix if someone has one for me.  I suspect that once BKL was removed, some lock might now be missing somewhere in the VFS that relates to rename/symlink/lookup.  I'll keep digging.

Cheers,
Erez.

Re: 2.6.39-rc7 BUG in fs/namei.c:1362, bisected -- BKL, symlinks, and rename

[Arnd Bergmann]

On Saturday 14 May 2011, Erez Zadok wrote:
> 
> Today 18:56:09
>    
> Easily reproducible bug found in v2.6.39-rc7-174-gddb503b. This line in fs/namei.c:nested_symlink() is triggered:
> 
>         BUG_ON(nd->depth >= MAX_NESTED_LINKS);
> 
> To reproduce: run racer on any f/s.  I narrowed it down to just 'rename' and 'symlink' ops (verified that other ops aren't involved).
> 
> Bisection narrowed the bug down to this one commit:

Hi Erez,

Thanks for the report and the work you put into bisecting it.

> commit f74b9444192c60603020c61d7915b72893137edc
> Merge: 7a63628 4ba8216
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Wed Mar 16 17:21:00 2011 -0700
> 
>     Merge branch 'config' of git://git.kernel.org/pub/scm/linux/kernel/git/arnd/bkl
>     
>     * 'config' of git://git.kernel.org/pub/scm/linux/kernel/git/arnd/bkl:
>       BKL: That's all, folks
>       fs/locks.c: Remove stale FIXME left over from BKL conversion
>       ipx: remove the BKL
>       appletalk: remove the BKL
>       x25: remove the BKL
>       ufs: remove the BKL
>       hpfs: remove the BKL
>       drivers: remove extraneous includes of smp_lock.h
>       tracing: don't trace the BKL
>       adfs: remove the big kernel lock
> 
> I'm happy to test a fix if someone has one for me.  I suspect that once BKL was removed, some lock might
> now be missing somewhere in the VFS that relates to rename/symlink/lookup.  I'll keep digging.

As the above commit is a merge commit, it seems an unlikely candidate for causing the bug. That would
indicate that something went wrong with Linus merging my patches, which I find less likely than
a bug in one of my patches.

Are you 100% sure that 4ba8216cd9 "BKL: That's all, folks" is actually working while the merged
commit f74b94441 is broken?

	Arnd

Re: 2.6.39-rc7 BUG in fs/namei.c:1362, bisected -- BKL, symlinks, and rename

[Erez Zadok]

Summary of this email: found real bad commit and have a proposed fix (below) which works for me.

On May 14, 2011, at 4:40 PM, Arnd Bergmann wrote:

> On Saturday 14 May 2011, Erez Zadok wrote:
>> 
>> Bisection narrowed the bug down to this one commit:
> 
>> commit f74b9444192c60603020c61d7915b72893137edc
>> Merge: 7a63628 4ba8216
>> Author: Linus Torvalds <torvalds@linux-foundation.org>
>> Date:   Wed Mar 16 17:21:00 2011 -0700
>> 
>>    Merge branch 'config' of git://git.kernel.org/pub/scm/linux/kernel/git/arnd/bkl
> 
> As the above commit is a merge commit, it seems an unlikely candidate for causing the bug. That would
> indicate that something went wrong with Linus merging my patches, which I find less likely than
> a bug in one of my patches.
> 
> Are you 100% sure that 4ba8216cd9 "BKL: That's all, folks" is actually working while the merged
> commit f74b94441 is broken?
> 
> 	Arnd

Arnd, you're right.  Maybe I messed up the bisection last night (swapped good/bad?).  Today I tested the code before the above commit, and it was still buggy, so I went further back and bisected twice more.  The first time gave me a patch related to symlink (getting warmer :-); I verified it manually by reversing that patch, and it still was wrong (either I made a mistake bisecting twice, or git-bisect can mess up some times).  Anyway, third bisection round was a charm.  This is the culprit:

----------------------------------------------------------
commit b356379a020bb7197603118bb1cbc903963aa198
Author: Al Viro <viro@zeniv.linux.org.uk>
Date:   Mon Mar 14 21:54:55 2011 -0400

    Turn resolution of trailing symlinks iterative everywhere
    
    The last remaining place (resolution of nested symlink) converted
    to the loop of the same kind we have in path_lookupat() and
    path_openat().
    
    Note that we still *do* have a recursion in pathname resolution;
    can't avoid it, really.  However, it's strictly for nested symlinks
    now - i.e. ones in the middle of a pathname.
    
    link_path_walk() has lost the tail now - it always walks everything
    except the last component.
    
    do_follow_link() renamed to nested_symlink() and moved down.
    
    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

:040000 040000 3dec373b0f0021cb4f57cbb129d01ddfcab5400b e11d1f347ce72b76a377692d858d29d8a06c0e8e M	fs
----------------------------------------------------------

I verified that the above commit was indeed causing the bug by going back and forth a bunch of times.

Next, I compared what the patch did.  The only thing I could think of that has semantically changed was the order of checks for symlink depth.  Old working code had this:

-       if (current->link_count >= MAX_NESTED_LINKS)
-               goto loop;
-       if (current->total_link_count >= 40)
-               goto loop;
-       BUG_ON(nd->depth >= MAX_NESTED_LINKS);

New buggy code had the order reversed:

+       BUG_ON(nd->depth >= MAX_NESTED_LINKS);
+       if (unlikely(current->link_count >= MAX_NESTED_LINKS)) {
+               path_put_conditional(path, nd);
+               path_put(&nd->path);
+               return -ELOOP;
+       }

So I tried to move the BUG_ON below the test for ELOOP, and this solved the bug for me.  I don't know if this is the right patch, Al, but I include it here for your review.

Cheers,
Erez.

——————————————————

VFS: move BUG_ON test for symlink nd->depth after current->link_count test

This solves a bug in nested_symlink (which was rewritten from
do_follow_link), and follows the order of depth tests that existed before.
The bug triggers a BUG_ON in fs/namei.c:1346, when running racer with
symlink and rename ops.

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
diff --git a/fs/namei.c b/fs/namei.c
index 017c3fa..7a93387 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1343,12 +1343,12 @@ static inline int nested_symlink(struct path *path, struct nameidata *nd)
 {
 	int res;
 
-	BUG_ON(nd->depth >= MAX_NESTED_LINKS);
 	if (unlikely(current->link_count >= MAX_NESTED_LINKS)) {
 		path_put_conditional(path, nd);
 		path_put(&nd->path);
 		return -ELOOP;
 	}
+	BUG_ON(nd->depth >= MAX_NESTED_LINKS);
 
 	nd->depth++;
 	current->link_count++;

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html