linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Oleg Drokin <oleg.drokin@intel.com>
Cc: linux-fsdevel@vger.kernel.org, Andreas Dilger <andreas.dilger@intel.com>
Subject: [RFC] lustre treatment of dentry->d_name
Date: Tue, 21 Oct 2014 02:13:46 +0100	[thread overview]
Message-ID: <20141021011346.GP7996@ZenIV.linux.org.uk> (raw)

	a) what protects ->d_name in ll_intent_file_open()?  It copies
->d_name.name and ->d_name.len into local variables and proceeds to
use those; what's to guarantee that dentry won't get hit with d_move()
halfway through that?  None of the locks that would give an exclusion
against d_move() appear to be held...

	b) what stabilizes *dentryp->d_name in do_statahead_enter()?

	c) what stabilizes fdentry->d_parent->d_name in llog_lvfs_destroy()?

Unless I'm missing something subtle, all three can race with d_move(),
with obvious unpleasant results.  The next bunch doesn't (the callers
are holding ->i_mutex on parents), but it's also bloody odd - why are we
playing these games in llite/namei.c?

static int ll_mkdir(struct inode *dir, struct dentry *dentry, ll_umode_t mode)
{
        return ll_mkdir_generic(dir, &dentry->d_name, mode, dentry);
}
static int ll_mkdir_generic(struct inode *dir, struct qstr *name,
                            int mode, struct dentry *dchild)

{
        int err;

        CDEBUG(D_VFSTRACE, "VFS Op:name=%.*s,dir=%lu/%u(%p)\n",
               name->len, name->name, dir->i_ino, dir->i_generation, dir);

        if (!IS_POSIXACL(dir) || !exp_connect_umask(ll_i2mdexp(dir)))
                mode &= ~current_umask();
        mode = (mode & (S_IRWXUGO|S_ISVTX)) | S_IFDIR;
        err = ll_new_node(dir, name, NULL, mode, 0, dchild, LUSTRE_OPC_MKDIR);

        if (!err)
                ll_stats_ops_tally(ll_i2sbi(dir), LPROC_LL_MKDIR, 1);

        return err;
}
and that's the only caller of ll_mkdir_generic().  What's the point of
passing this qstr separately, when you are passing dentry itself anyway?
Both to ll_mkdir_generic() *and* to ll_new_node().  And AFAICS all callers
of ll_new_node() have the second argument equal to address of ->d_name of
the 6th one...  I thought you guys had some stack footprint problems?

While we are at it, where is ll_new_inode() ever called with NULL dchild?
Similar to that, where is ll_d_mountpoint() ever called with NULL dchild,
why do you have
	if (unlikely(dchild))
in there when it's true on every call and why does it exist in the first
place?  All its callers are reachable only from vfs_{unlink,rmdir,rename}
and we *do* d_mountpoint() checks there.

Comments?

             reply	other threads:[~2014-10-21  1:13 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-10-21  1:13 Al Viro [this message]
2014-10-21  2:55 ` [RFC] lustre treatment of dentry->d_name Al Viro
2014-10-21  3:55   ` Drokin, Oleg
2014-10-21  3:46 ` Drokin, Oleg
2014-10-21  4:02   ` Al Viro
2014-10-21 13:34     ` Drokin, Oleg
2014-10-21 21:17       ` Al Viro
2014-10-22  1:48         ` Drokin, Oleg
2014-10-22  2:50           ` Al Viro
2014-10-22  9:30             ` Drokin, Oleg
2014-10-21 19:30     ` Al Viro
2014-10-22  1:49       ` Drokin, Oleg
2014-10-21 20:07   ` Al Viro
2014-10-22  1:53     ` Drokin, Oleg

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20141021011346.GP7996@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=andreas.dilger@intel.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=oleg.drokin@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).