Re: [RFC] racy use of ->d_name

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

On Wed, Dec 14, 2016 at 02:35:39PM -0500, Mike Marshall wrote:

> I'm guessing this example is how I should protect simple usages
> like this:
> 
> diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c
> index 5355efb..05321f4 100644
> --- a/fs/orangefs/dcache.c
> +++ b/fs/orangefs/dcache.c
> @@ -30,9 +30,11 @@ static int orangefs_revalidate_lookup(struct dentry *dentry)
> 
>         new_op->upcall.req.lookup.sym_follow = ORANGEFS_LOOKUP_LINK_NO_FOLLOW;
>         new_op->upcall.req.lookup.parent_refn = parent->refn;
> +       spin_lock(&dentry->d_lock);
>         strncpy(new_op->upcall.req.lookup.d_name,
>                 dentry->d_name.name,
>                 ORANGEFS_NAME_MAX);
> +       spin_unlock(&dentry->d_lock);
> 
>         gossip_debug(GOSSIP_DCACHE_DEBUG,
>                      "%s:%s:%d interrupt flag [%d]\n",

That one is potentially nastier - ->d_revalidate() *can* race with
rename().  Next cycle we'll hopefully get to the point where this
kind of "we need to force lookup" will be handled outside of ->d_revalidate();
the main problem with doing so right now is that we would lose anything
mounted on that thing or anywhere under it and we will always get a new
dentry for non-directories.  AFAICS, your instance is safe with that ->d_lock,
but I would really prefer to have that done from ->lookup() rather than
duplicate between lookup and d_revalidate like now.  I have some bits and
pieces in that direction, but it was way too late in this cycle to be
4.10 material.

> It seems that in debug statements, icky looking things like
> file->f_path.dentry->d_name.name get replaced with "file" and "%pD".
> 
> I have a place where I use file->f_path.dentry->d_name.name in a
> strcmp, and the way I "fixed" it seems verbose and hackerly... what
> should I do instead?

debugfs has no rename, so there it's actually stable.