RE: [RFC] weird stuff in exfat_lookup()

["Sungjong Seo" <sj1557.seo@samsung.com>]

> [with apologies for very late reply]
That's okay, no problem.

> 
> On Thu, Mar 13, 2025 at 09:39:39PM +0900, Sungjong Seo wrote:
> > - thread1: while(1) { mkdir(A) and rmdir(A) }
> > - thread2: while(1) { stat(A) }
> >
> > This is due to the characteristics of exfat allowing negative dentry and
> > considering CI in d_revalidate. As mentioned in the comment,
> > unhashed-positive dentry can exist in a situation where mkdir
> > and stat are competing, and it can be dropped, but exfat_lookup has
> > been implemented to reuse(rehash) this dentry.
> 
> That's an interesting scenario, but I still don't see why would we bother.
> 
> Note that in your example we don't need to even look for aliases - it's
> a directory inode, so d_splice_alias() would do the right thing, no matter
> what.  And for non-directories you
> 	* already have d_move(alias, dentry) there, which would do the
> right
> thing as well and
> 	* won't get an unhashed alias from d_find_alias() to start with.
> 
> Frankly, I would skip the entire "look for aliases" thing in case of
> directory inodes - just let d_splice_alias() handle it.  That has
> another fun benefit - exfat_d_anon_disconn() check becomes completely
> pointless.  By the time we call it we have already verified that
> alias->d_parent == dentry->d_parent, so the only way to get
> exfat_d_anon_disconn(alias) to be true is to have IS_ROOT(alias),
> i.e. alias->d_parent == alias and thus alias == dentry->d_parent
> and at the very least the inode is a directory one.  We obvously
> want to have it fail with -ELOOP in such case and d_splice_alias()
> does just that, so if we bypass the entire "look for an alias"
> thing for directories, the check becomes identical to
> 	if (alias && alias->d_parent == dentry->d_parent)
> since the last term in the current variant (!exfat_d_anon_disconn(...))
> can be dropped, along with the helper itself.
> 
> Does anybody have a problem with patch below?

Seems right!
In fact, exFAT does not support symlinks, shortname aliases, or export_ops.
So, all it has to do here is handle case insensitivity for the same name.
Later, it seems we might also need to check the exfat_d_revalidate().

I greatly appreciate your meticulous review of the exFAT lookup operation
and the patches you provided. I will check if there are any issues with
the existing scenario along with the patch review. However, I am currently
super busy with my main job, so it would take some time.

Thanks!

> 
> [PATCH] simplify exfat_lookup()
> 
> 1) d_splice_alias() handles ERR_PTR() for inode just fine
> 2) no need to even look for existing aliases in case of directory inodes;
> just punt to d_splice_alias(), it'll do the right thing
> 3) no need to bother with 'd_unhashed(alias)' case - d_find_alias()
> would've returned that only in case of a directory, and d_splice_alias()
> will handle that just fine on its own.
> 4) exfat_d_anon_disconn() is entirely pointless now - we only get to
> evaluating it in case dentry->d_parent == alias->d_parent and
> alias being a non-directory.  But in that case IS_ROOT(alias) can't
> possibly be true - that would've reqiured alias == alias->d_parent,
> i.e alias == dentry->d_parent and dentry->d_parent is guaranteed to
> be a directory.  So exfat_d_anon_disconn() would always return false
> when it's called, which makes && !exfat_d_anon_disconn(alias)
> a no-op.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
> index 670116ae9ec8..8fac39f2bcb3 100644
> --- a/fs/exfat/namei.c
> +++ b/fs/exfat/namei.c
> @@ -711,71 +711,44 @@ static int exfat_find(struct inode *dir, const
> struct qstr *qname,
>  	return 0;
>  }
> 
> -static int exfat_d_anon_disconn(struct dentry *dentry)
> -{
> -	return IS_ROOT(dentry) && (dentry->d_flags & DCACHE_DISCONNECTED);
> -}
> -
>  static struct dentry *exfat_lookup(struct inode *dir, struct dentry
> *dentry,
>  		unsigned int flags)
>  {
>  	struct super_block *sb = dir->i_sb;
> -	struct inode *inode;
> +	struct inode *inode = NULL;
>  	struct dentry *alias;
>  	struct exfat_dir_entry info;
>  	int err;
>  	loff_t i_pos;
> -	mode_t i_mode;
> 
>  	mutex_lock(&EXFAT_SB(sb)->s_lock);
>  	err = exfat_find(dir, &dentry->d_name, &info);
>  	if (err) {
> -		if (err == -ENOENT) {
> -			inode = NULL;
> -			goto out;
> -		}
> -		goto unlock;
> +		if (unlikely(err != -ENOENT))
> +			inode = ERR_PTR(err);
> +		goto out;
>  	}
> 
>  	i_pos = exfat_make_i_pos(&info);
>  	inode = exfat_build_inode(sb, &info, i_pos);
> -	err = PTR_ERR_OR_ZERO(inode);
> -	if (err)
> -		goto unlock;
> +	if (IS_ERR(inode) || S_ISDIR(inode->i_mode))
> +		goto out;
> 
> -	i_mode = inode->i_mode;
>  	alias = d_find_alias(inode);
> 
>  	/*
>  	 * Checking "alias->d_parent == dentry->d_parent" to make sure
>  	 * FS is not corrupted (especially double linked dir).
>  	 */
> -	if (alias && alias->d_parent == dentry->d_parent &&
> -			!exfat_d_anon_disconn(alias)) {
> -
> +	if (alias && alias->d_parent == dentry->d_parent) {
>  		/*
> -		 * Unhashed alias is able to exist because of revalidate()
> -		 * called by lookup_fast. You can easily make this status
> -		 * by calling create and lookup concurrently
> -		 * In such case, we reuse an alias instead of new dentry
> +		 * This inode has non anonymous-DCACHE_DISCONNECTED
> +		 * dentry. This means, the user did ->lookup() by an
> +		 * another name (longname vs 8.3 alias of it) in past.
> +		 *
> +		 * Switch to new one for reason of locality if possible.
>  		 */
> -		if (d_unhashed(alias)) {
> -			WARN_ON(alias->d_name.hash_len !=
> -				dentry->d_name.hash_len);
> -			exfat_info(sb, "rehashed a dentry(%p) in read
lookup",
> -				   alias);
> -			d_drop(dentry);
> -			d_rehash(alias);
> -		} else if (!S_ISDIR(i_mode)) {
> -			/*
> -			 * This inode has non anonymous-DCACHE_DISCONNECTED
> -			 * dentry. This means, the user did ->lookup() by an
> -			 * another name (longname vs 8.3 alias of it) in
past.
> -			 *
> -			 * Switch to new one for reason of locality if
> possible.
> -			 */
> -			d_move(alias, dentry);
> -		}
> +		d_move(alias, dentry);
>  		iput(inode);
>  		mutex_unlock(&EXFAT_SB(sb)->s_lock);
>  		return alias;
> @@ -787,9 +760,6 @@ static struct dentry *exfat_lookup(struct inode *dir,
> struct dentry *dentry,
>  		exfat_d_version_set(dentry, inode_query_iversion(dir));
> 
>  	return d_splice_alias(inode, dentry);
> -unlock:
> -	mutex_unlock(&EXFAT_SB(sb)->s_lock);
> -	return ERR_PTR(err);
>  }
> 
>  /* remove an entry, BUT don't truncate */