vclean() vs namei cache

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

Hi,
looking again at the vnode locking code I found a possible reuse of a
cached vnode which is being cleaned out, from the namei cache.

vclean() is called with a reference and the interlock held.
cache_lookup() will gets a 'struct namecache' from its cache which
holds vnode pointers. these pointers are not accounted in
v_usecount.

Now if you're unlucky, a cache_lookup() will find a namecache entry,
it'll do a vtryget() on the vnode pointed to by this entry
and this is where the race is:
vtryget works unlocked, so the interlock doesn't come in play here.
vtryget:
 1 - checks for (VI_XLOCK | VI_FREEING) and if present, return
     false
 2 - checks if v_usecount is 0 or has VC_XLOCK; if not it
     bumps v_usecount (atomically) and returns true. Otherwise
     returns false.

thread 1 could be calling vclean() and so holding a reference, without
having set VI_XLOCK yet.
thread 2 is doing step 1 and succeeds.
thread 1 sets VI_XLOCK, checks v_usecount and sets active to 0
thread 2 is doing step 2 and succeds.
And so cache_lookup() returns a vnode which is about to be cleaned.

Does anyone see what is preventing this from happening ?
I wonder if we should just drop vtryget(), or set VC_XLOCK in vrelel
in addition to VI_INACTNOW (and probably in vflush too) ?
VC_XLOCK costs 2 atomic add in vrelel() when this is the last reference,
while using vget() in place of vtryget() costs a v_interlock mutex
grab/release (which should be a CAS and a write) for every cache_lookup()
which already has a reference.
Not sure which one is cheaper.

Of course cache_lookup_raw() has the same issue.

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--