linux-kernel - Re: [PATCH 5/5] union: hybrid union filesystem prototype

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100901214238.GC15849@shell>
Date:	Wed, 1 Sep 2010 17:42:38 -0400
From:	Valerie Aurora <vaurora@...hat.com>
To:	Miklos Szeredi <miklos@...redi.hu>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	neilb@...e.de, viro@...iv.linux.org.uk, jblunck@...e.de,
	hch@...radead.org
Subject: Re: [PATCH 5/5] union: hybrid union filesystem prototype

On Thu, Aug 26, 2010 at 08:33:45PM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@...e.cz>
> 
> This union filesystem is a hybrid of entirely filesystem based
> (unionfs, aufs) and entierly VFS based (union mounts) solutions.

This is elegant and readable code.  I am still reviewing it but have a
few comments now.

> +static int union_upper_create(struct dentry *dentry, struct iattr *attr,
> +			      dev_t rdev, const char *link, struct path *src)
> +{
> +	int err;
> +	int attr_update = ATTR_UID | ATTR_GID | ATTR_ATIME_SET | ATTR_MTIME_SET;
> +	struct dentry *parent = dget_parent(dentry);
> +	struct union_entry *ue = dentry->d_fsdata;
> +	struct union_entry *pue = parent->d_fsdata;
> +	struct inode *upperdir = pue->upperpath.dentry->d_inode;
> +	struct dentry *newdentry;
> +	struct path newpath;
> +
> +	mutex_lock_nested(&upperdir->i_mutex, I_MUTEX_PARENT);
> +
> +	/*
> +	 * Using upper filesystem locking to protect against copy up
> +	 * racing with rename (rename means the copy up was already
> +	 * successful).
> +	 */
> +	err = -EEXIST;
> +	if (dentry->d_parent != parent)
> +		goto out_unlock;
> +
> +	newdentry = union_lookup_create(ue, pue, &dentry->d_name);
> +	err = PTR_ERR(newdentry);
> +	if (IS_ERR(newdentry))
> +		goto out_unlock;
> +
> +	newpath.dentry = newdentry;
> +	newpath.mnt = pue->upperpath.mnt;
> +
> +	switch (attr->ia_mode & S_IFMT) {
> +	case S_IFREG:
> +		if (src)
> +			WARN_ON(!(attr->ia_valid & ATTR_SIZE));
> +		else
> +			WARN_ON((attr->ia_valid & ATTR_SIZE));
> +
> +		err = vfs_create(upperdir, newdentry, attr->ia_mode, NULL);

Passing a NULL namiedata pointer to vfs_create() is a convenient
temporary hack, but unfortunately NFS, ceph, etc. still use the
nameidata passed to vfs_create() and other ops.

The way union mounts gets a valid nameidata is by doing the create in
the VFS before calling file system ops that may trigger a copyup,
while we still have the original nameidata.  This is one of the major
reasons union mounts lives in the VFS.

A lot of my conversations about union mounts with Al go like this:

Al: "Rewrite it this way."
Val: "But then how do we get the nameidata?"
Al: "Arrrrrrrrrrrrrggggh."

Can you think of a way to construct a good nameidata for these
implicit copyups?  That might be a solution.

-VAL
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/