Subject: Re: [solution] vnode refcount panic, perhaps due to kern/vfs_lookup.c:lookup()
To: Greg Troxel <firstname.lastname@example.org>
From: Bill Studenmund <email@example.com>
Date: 03/30/2003 16:14:06
On Fri, 28 Mar 2003, Greg Troxel wrote:
> No. It's coda's issue. From another part of vnode_if.src:
> # For operations other than VOP_LOOKUP which require a component name
> # parameter, the flags required for the initial namei() call are listed.
> # Additional flags may be added to the namei() call, but these are required.
> which I take to mean you may get other flags. If you don't want them, be
> I read it otherwise, saying that you have to call namei with some
> flags, but you can put other flags on the namei call if you have some
> reason. The issue I'm talking about is whether it is ok to have
> random flags, such as LOCKPARENT, set on random vnodeops . And if so,
> whether they have the described semantics (the notion of LOCKPARENT
> makes sense for VOP_SYMLINK), or are _required to be ignored_ by the
> vnodeop. Currently, flags like LOCKPARENT appear to be ignored
> (leaving VOP_LOOKUP aside as a special case).
The vnode op should behave as described in vnode_if.src. For symlink, it
is defined to return with the parent unlocked, not with the parent's state
as set by the absense of LOCKPARENT. VOP_LOOKUP is the only call AFAIK
that is defined to respect LOCKPARENT.
> ufs_symlink, for example, does not respect the LOCKPARENT flag; the
> vput is buried deep in ufs_makeinode. ufs_mkdir is similar.
> The top half of the structure is used exclusively for the
> pathname lookups using VOP_LOOKUP() and is initialised by the
> This is the place where it says that operations other than lookup have
> to ignore the flags.
> In the coda_symlink case, LOCKPARENT was on the namei call before
> VOP_SYMLINK, as required. coda_symlink should ignore LOCKPARENT, and
> thus can't just use the componentname to do a lookup. The bug is
> arguably in the new coda_symlink code, since VOP_LOOKUP is now called
> with the same componentname structure passed to VOP_SYMLINK.
I agree with the last part - it's coda_symlink's problem.
> The unintended consequences (leaving the parent locked) of reusing
> componentname like this violates the Principle of Least Astonishment,
> and I think it is unclean. Flags that are defined to be ignored
> shouldn't be set, and perhaps a #ifdef DIAGNOSTIC_EXTRA_PARANOID in
Uhm, that means they aren't being ignored. :-)
> vnode_if.c to check for them. This issue is quite common in
> kern/vfs_syscalls.c, though. Arguably it is just plain wrong to call
> lookup reusing a componentname struct one was passed.
> The LOCKPARENT issue is purely stylistic; with the &= ~LOCKPARENT in
> coda_symlink I am not aware of any incorrect behavior.
> Cool! Then this is probably the way to go!
> I think you are saying that you like the patch to coda_symlink, and
> that sys_symlink is correct. Committing the coda_symlink patch would
> be nice; the current code is certainly incorrect, although without the
> complication of cfs the bug seems not to be triggered.
I think that the coda_symlink change needs to happen, and that sys_symlink
is not wrong.