Suppose the last dentry refering to an inode is being evicted.
It went through __dentry_kill(), into dentry_iput() and called ->d_iput().
At that stage it is unhashed, out of the list of children, out of the list
of aliases and does not refer to inode anymore. No locks are held and
it's about to call iput().
In the meanwhile, two lookups have found links to that inode. The
same in-core inode has already been picked by ll_iget() by both. Their
dentries are unhashed, so ll_lookup_it_finish() hits
alias = ll_splice_alias(inode, *de);
in both. inode is non-NULL, so
new = ll_find_alias(inode, de);
is done.
static struct dentry *ll_find_alias(struct inode *inode, struct dentry *dentry)
{
struct dentry *alias, *discon_alias, *invalid_alias;
if (hlist_empty(&inode->i_dentry))
return NULL;
discon_alias = invalid_alias = NULL;
ll_lock_dcache(inode);
ends up returning NULL for both (ll_lock_dcache == grab ->i_lock). So we
get NULL from ll_find_alias(), proceed to allocate ->d_fsdata for both
dentries, then do d_add(de, inode). See the problem?
We do not hold ->i_lock between ll_find_alias() and d_add(), so there's
nothing to guarantee that negative result will stay valid. Even had we kept
it (and with block allocation it would require some work), decision that
there's no aliases at all is made _outside_ of ->i_lock, so we have a race
there as well.
Moreover, ll_find_alias() itself is rather fishy - it accepts any alias that
happens to have DCACHE_DISCONNECTED and anything unhashed with exact match
to parent/name/hash. The former part is trouble - DCACHE_DISCONNECTED is
cleared *after* a detached subtree had been glued in. It's _not_ equivalent
to "it's a root of detached subtree".
IOW, that thing can return dentries that do have parent other than the one
we'd been looking for. Which means that d_move() done by ll_splice_alias()
after having found one is short on locking.
Another oddity: you have ll_d_init() called on the result of ll_find_alias(),
but that alias must have either been through ll_splice_alias() before or
through d_instantiate() in ll_lookup_it_finish(), or it wouldn't have been
positive. And the second variant required that it had been hashed at the
time, which means that it had been through ll_splice_alias() at some earlier
point. Once ->d_fsdata had been allocated and set, it's never cleared until
we free that dentry, so how the hell is it possible for that call of
ll_d_init() on result of ll_find_alias() to actually do anything at all?
Am I right assuming that it's basically trying to do d_splice_alias() with
a couple of twists - we are likely to find an exact unhashed alias (due to
promiscuous ->d_revalidate()?) we want to try and reuse even for
non-directories and it's likely enough to make unconditional allocation of
->d_fsdata a bad idea since dentry is likely to be replaced by alias?
If so, how about adding d_hash_exact_alias(dentry) that would try to find
and hash an exact unhashed alias, so that this thing would
* call d_hash_exact_alias(dentry), if found - just return it
* ll_d_init(dentry)
* return d_splice_alias() ?: dentry
Do you see any problems with that? Parent directory is locked, so no new
unhashed exact aliases should have a chance to appear and d_splice_alias()
would take care of everything else (correctness and detached ones).
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
