Re: BUG_ON(nd->inode->i_op->follow_link);

[Linus Torvalds]

On Thu, Mar 7, 2013 at 11:35 AM, Dave Jones <davej@xxxxxxxxxx> wrote:
> On Thu, Mar 07, 2013 at 09:30:56AM -0800, Linus Torvalds wrote:
>  >
>  > So I think we should change that BUG_ON() into a
>  >
>  >         if (WARN_ON_ONCE(nd->inode != parent->d_inode))
>  >                 return -ESTALE;
>
> Curiously, the machine wasn't dead after hitting that.
> Oh wait, it locks up that one CPU, leaving the others running right ?
> That would explain it, it's got a few cores..

No, it's simpler than that - we normally don't hold any extra locks
while walking pathnames.

The keyword being *normally*.

When we do things like renames etc, we often have other locks, and
then a BUG_ON() in that place is just deadly. But in your particular
case I think killing the process ends up just doing the right thing.

So a BUG_ON() can happen to do the right thing. In fact, back in the
BKL days it was pretty common, since the exit path could also undo the
BKL. It's just that these days it's not unlikely that we hold some
other lock or whatever, and then BUG_ON() can end up really screwing
things up.

>  > Hmm. Nothing looks all that odd in that trace. Do you have any idea
>  > what the path was? This being trinity, I'm assuming you're doing some
>  > kind of targeted testing. sysfs or proc, perhaps? Or some particular
>  > concurrency test with random system calls/pathnames? Not that I see
>  > how it could happen anyway, but maybe it could give some hint about
>  > what triggered this.
>
> Basically, see the summary of a bunch of bugs I reported to Greg last night
> in sysfs: https://lkml.org/lkml/2013/3/7/21
> It sounds like it's just trinity finding old bugs for the first time,
> though I've not actually tested yet on an older kernel.

If this is fairly repeatable, I really think it would be interesting
to see the names involved. Especially for sysfs, there are a *lot* of
random files that have odd semantics, and it depends on the file. Same
is (to a slightly lesser degree) true of /proc (which does have many
of the same issues, but tends to be more tested just for having been
around for longer - but then proc does have some issues all its own)

So for example, if you can re-create the one in nd_jump_link(), it
would be lovely if you replaced the BUG_ON() with just an if(), and
made it print out the old and the new path dentry names (ok, that
means saving the old path and doing the path_put on it afterwards).

Something like

+    const char *oldname = nd->path.dentry->d_name.name;   /* Yeah,
this remembers the name pointer over the put_path(), not strictly
right */
+    const char *newname = path->dentry->d_name.name;
      ...
-    BUG_ON(nd->inode->i_op->follow_link);
+    if (WARN_ON(nd->inode->i_op->follow_link)) {
+        printk("old=%s new=%d\n", oldname, newname);
+    }

or other..

> I suspect it's the addition of this..
> http://git.codemonkey.org.uk/?p=trinity.git;a=commitdiff;h=fd46c22e967a613de73d7e51a9715717d954ec45
> Which adds a bunch of negative dentry lookups when it hits a mangled pathname.

Yeah, it would have been nicer if we could bisect this to something,
or limit it to the changes in -rc1, but...

      Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/