Re: [PATCH] configfs: Delay the fop setting to prevent noise generated by open

[Breno Leitao <leitao@debian.org>]

Hello Edward,

> The root cause is a race condition between open and mkdir. Add a revalidate
> for cached dentries to prevent open from accessing directories that are not
> yet ready.

Thanks for chasing this one. The analysis matches what I see: a lookup
holds the dentry of the half-built group, d_delete() can only unhash it,
and the BUG_ON() in create_default_group() fires.

The fix has a side effect, though:

> +	if (flags & LOOKUP_RCU)
> +		return -ECHILD;
> +
> +	if (!inode)
> +		return -ECHILD;
> +
> +	if (!configfs_dirent_is_ready(parent_sd))
> +		return -ECHILD;

-ECHILD only means "retry in ref-walk" when LOOKUP_RCU is set. In
ref-walk, lookup_fast() and lookup_dcache() hand it back as is, so it
reaches userspace, and mkdir() and open() now fail with ECHILD when they
race with a lookup.

With the syzbot setup (threads opening ports/1, ports/1/subsystems and
ports/1/ana_groups while mkdir(ports/1) runs under fail-nth), mkdir()
returned ECHILD in 2 to 11 of ~220 attempts per run, over 11 runs. The
hook also takes every configfs lookup out of RCU-walk.

Returning 0 instead would not help. The VFS would d_invalidate() the
dentry of a default group, and configfs_lookup() cannot recreate pinned
dentries.

I think the BUG_ON() itself is the bug. It assumes nobody else holds the
dentry of a default group while it is built, but a lookup can find it in
the dcache. The error path already copes with that: d_delete() unhashes
the busy dentry, the inode is S_DEAD, and lookup, open, mkdir and
symlink all refuse a group that is not ready. 

Thanks,
--breno