On Tue, Jul 31, 2007 at 08:34:47PM +0900, Tejun Heo wrote:
> > If sysfs_mutex nested the other way things would be easier,
> > and we could grab all of the i_mutexes we wanted.  I wonder if we can
> > be annoying in sysfs_lookup and treat that as the lock inversion
> > case using mutex_trylock etc.  And have sysfs_mutex be on the
> > outside for the rest of the cases?
> 
> The problem with treating sysfs_lookup as inversion case is that vfs
> layer grabs i_mutex outside of sysfs_lookup.  Releasing i_mutex from
> inside sysfs_lookup would be a hacky layering violation.
> 
> Then again, the clean up which can come from the new sysfs_looukp_dentry
> is very significant.  I'll think about it a bit more.

How about something like this?  __sysfs_get_dentry() never creates any
dentry, it just looks up existing ones.  sysfs_get_dentry() calls
__sysfs_get_dentry() and if it fails, it builds a path string and look
up using regular vfs_path_lookup().  Once in the creation path,
sysfs_get_dentry() is allowed to fail, so allocating path buf is fine.

It still needs to retry when vfs_path_lookup() returns -ENOENT or the
wrong dentry but things are much simpler now.  It doesn't violate any
VFS locking rule while maintaining all the benefits of
sysfs_get_dentry() cleanup.

Something like LOOKUP_KERNEL is needed to ignore security checks;
otherwise, we'll need to resurrect lookup_one_len_kern() and open code
look up.

The patch is on top of all your patches and is in barely working form.

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 66d418a..0a6e036 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -81,20 +81,19 @@ void sysfs_unlink_sibling(struct sysfs_dirent *sd)
  *	Get dentry for @sd.
  *
  *	This function descends the sysfs dentry tree from the root
- *	populating it if necessary until it reaches the dentry for @sd.
+ *	until it reaches the dentry for @sd.
  *
  *	LOCKING:
- *	Kernel thread context (may sleep)
+ *	mutex_lock(sysfs_mutex)
  *
  *	RETURNS:
- *	Pointer to found dentry on success, ERR_PTR() value on error.
+ *	Pointer to found dentry on success, NULL if doesn't exist.
  */
-struct dentry *__sysfs_get_dentry(struct sysfs_dirent *sd, int create)
+struct dentry *__sysfs_get_dentry(struct sysfs_dirent *sd)
 {
 	struct sysfs_dirent *cur;
 	struct dentry *parent_dentry, *dentry;
 	struct qstr name;
-	struct inode *inode;
 
 	parent_dentry = NULL;
 	dentry = dget(sysfs_sb->s_root);
@@ -111,26 +110,8 @@ struct dentry *__sysfs_get_dentry(struct sysfs_dirent *sd, int create)
 		name.name = cur->s_name;
 		name.len = strlen(cur->s_name);
 		dentry = d_hash_and_lookup(parent_dentry, &name);
-		if (dentry)
-			continue;
-		if (!create)
-			goto out;
-		dentry = d_alloc(parent_dentry, &name);
-		if (!dentry) {
-			dentry = ERR_PTR(-ENOMEM);
-			goto out;
-		}
-		inode = sysfs_get_inode(cur);
-		if (!inode) {
-			dput(dentry);
-			dentry = ERR_PTR(-ENOMEM);
-			goto out;
-		}
-		d_instantiate(dentry, inode);
-		sysfs_attach_dentry(cur, dentry);
-	} while (cur != sd);
+	} while (dentry && cur != sd);
 
-out:
 	dput(parent_dentry);
 	return dentry;
 }
@@ -138,10 +119,52 @@ out:
 struct dentry *sysfs_get_dentry(struct sysfs_dirent *sd)
 {
 	struct dentry *dentry;
+	struct nameidata nd;
+	char *path_buf;
+	int len, rc;
+	
+	mutex_lock(&sysfs_mutex);
+	dentry = __sysfs_get_dentry(sd);
+	mutex_unlock(&sysfs_mutex);
+
+	if (dentry)
+		return dentry;
+
+	/* Look up failed.  This means that the dentry associated with
+	 * @sd currently doesn't exist and we're allowed to fail.
+	 * Let's allocate path buffer and look up like a sane person.
+	 */
+	path_buf = kmalloc(PATH_MAX, GFP_KERNEL);
+	if (!path_buf)
+		return ERR_PTR(-ENOMEM);
 
+ retry:
 	mutex_lock(&sysfs_mutex);
-	dentry = __sysfs_get_dentry(sd, 1);
+	/* XXX - clean up */
+	len = object_path_length(sd);
+	BUG_ON(len > PATH_MAX);
+	path_buf[len - 1] = '\0';
+	fill_object_path(sd, path_buf, len);
 	mutex_unlock(&sysfs_mutex);
+
+	/* XXX - we need a flag to override security check or need to
+	 * open code lookup.  The former is far better, IMHO.
+	 */
+	rc = vfs_path_lookup(sysfs_mount->mnt_root, sysfs_mount,
+			     path_buf + 1, 0, &nd);
+	dentry = nd.dentry;
+
+	/* renamed/moved beneath us? */
+	if (rc == -ENOENT)
+		goto retry;
+	if (rc == 0 && dentry->d_fsdata != sd) {
+		dput(dentry);
+		goto retry;
+	}
+	if (rc && rc != -ENOENT)
+		dentry = ERR_PTR(rc);
+
+	kfree(path_buf);
 	return dentry;
 }
 
@@ -512,7 +535,7 @@ static void sysfs_drop_dentry(struct sysfs_dirent *sd)
 
 	/* Remove a dentry for a sd from the dcache if present */
 	mutex_lock(&sysfs_mutex);
-	dentry = __sysfs_get_dentry(sd, 0);
+	dentry = __sysfs_get_dentry(sd);
 	if (IS_ERR(dentry))
 		dentry = NULL;
 	if (dentry) {
@@ -700,11 +723,6 @@ static struct dentry * sysfs_lookup(struct inode *dir, struct dentry *dentry,
 
 	mutex_lock(&sysfs_mutex);
 
-	/* Guard against races with sysfs_get_dentry */
-	result = d_hash_and_lookup(dentry->d_parent, &dentry->d_name);
-	if (result)
-		goto out;
-
 	sd = sysfs_find_dirent(parent_sd, dentry->d_name.name);
 
 	/* no such entry */
diff --git a/fs/sysfs/mount.c b/fs/sysfs/mount.c
index 0ad731b..3f652be 100644
--- a/fs/sysfs/mount.c
+++ b/fs/sysfs/mount.c
@@ -15,7 +15,7 @@
 /* Random magic number */
 #define SYSFS_MAGIC 0x62656572
 
-static struct vfsmount *sysfs_mount;
+struct vfsmount *sysfs_mount;
 struct super_block * sysfs_sb = NULL;
 struct kmem_cache *sysfs_dir_cachep;
 
diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index 2b542dc..336823c 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -21,7 +21,7 @@ static int object_depth(struct sysfs_dirent *sd)
 	return depth;
 }
 
-static int object_path_length(struct sysfs_dirent * sd)
+int object_path_length(struct sysfs_dirent * sd)
 {
 	int length = 1;
 
@@ -31,7 +31,7 @@ static int object_path_length(struct sysfs_dirent * sd)
 	return length;
 }
 
-static void fill_object_path(struct sysfs_dirent *sd, char *buffer, int length)
+void fill_object_path(struct sysfs_dirent *sd, char *buffer, int length)
 {
 	--length;
 	for (; sd->s_parent; sd = sd->s_parent) {
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index 265a16a..86704ef 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -51,6 +51,7 @@ struct sysfs_addrm_cxt {
 };
 
 extern struct sysfs_dirent sysfs_root;
+extern struct vfsmount *sysfs_mount;
 extern struct kmem_cache *sysfs_dir_cachep;
 
 extern struct dentry *sysfs_get_dentry(struct sysfs_dirent *sd);
@@ -89,6 +90,9 @@ extern void sysfs_remove_subdir(struct sysfs_dirent *sd);
 
 extern int sysfs_setattr(struct dentry *dentry, struct iattr *iattr);
 
+extern int object_path_length(struct sysfs_dirent * sd);
+extern void fill_object_path(struct sysfs_dirent *sd, char *buffer, int length);
+
 extern spinlock_t sysfs_assoc_lock;
 extern struct mutex sysfs_mutex;
 extern struct super_block * sysfs_sb;
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers