Re: [RFC] weird use of dget_parent() in xfs_filestream_get_parent()

[Al Viro <viro@zeniv.linux.org.uk>]

On Mon, Mar 24, 2025 at 09:52:15PM +0000, Al Viro wrote:
> Function in question:
> 
> static struct xfs_inode *
> xfs_filestream_get_parent(
>         struct xfs_inode        *ip)
> {
>         struct inode            *inode = VFS_I(ip), *dir = NULL;
>         struct dentry           *dentry, *parent;
> 
>         dentry = d_find_alias(inode);
>         if (!dentry)
>                 goto out;
> 
>         parent = dget_parent(dentry);
>         if (!parent)
>                 goto out_dput;
> 
>         dir = igrab(d_inode(parent));
>         dput(parent);
> 
> out_dput:
>         dput(dentry);
> out:
>         return dir ? XFS_I(dir) : NULL;
> }
> 
> 1) dget_parent(dentry) never returns NULL; if you have IS_ROOT(dentry) you
> get an equivalent of dget(dentry).  What do you want returned in that
> case?
> 
> 2) why bother with dget_parent() in the first place?  What's wrong with
> 	spin_lock(&dentry->d_lock); // stabilizes ->d_parent
>         dir = igrab(dentry->d_parent->d_inode);
> 	spin_unlock(&dentry->d_lock);
> 
> or, if you intended NULL for root, 
> 	spin_lock(&dentry->d_lock); // stabilizes ->d_parent
> 	if (!IS_ROOT(dentry))
> 		dir = igrab(dentry->d_parent->d_inode);
> 	spin_unlock(&dentry->d_lock);
> 
> 
> Is there anything subtle I'm missing here?

For (2) there is - ->i_lock nests outside of ->d_lock, so igrab() under
any ->d_lock is not valid.

For (1) we provably can't get NULL - there are two returns in dget_parent():
                if (!read_seqcount_retry(&dentry->d_seq, seq))
                        return ret;
and
        spin_unlock(&ret->d_lock);
        return ret;
and neither can return a NULL - we'd oops immediately prior to getting
there.

The same goes for xrep_findparent_from_dcache() - in
        parent = dget_parent(dentry);
	if (!parent)
		goto out_dput;

	ASSERT(parent->d_sb == sc->ip->i_mount->m_super);

	pip = igrab(d_inode(parent));
	dput(parent);
we need to use dget_parent(), but we really can't get a NULL from
it.