RE: [RFC] weird stuff in exfat_lookup()

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

Hi!
> > [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>

The patch looks good to me and seems to be working as intended.
Thank you for your efforts!

Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com>

> > ---
> > 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 */