Skip to contentSkip to conversation

sysfs tagged directories

PATCH RESEND In review Benjamin Thery 12 participants 48 messages May 6, 2008 – May 14, 2008

Subsystems (10)
lore
Share & tools
lore
#0 lore raw

cover sysfs tagged directories

This is still the same port of Eric Biederman's patchset to implement tagged directories in sysfs that was discussed a few days ago.

This time it applies on top of 2.6.26-rc1,
which includes a fix from Daniel Lezcano to fix net device renaming for sysfs (and of course all the network namespaces stuff that was committed in net-2.6.26).

This patchset still contains the patch from Serge Hallyn that implements tagging for user namespaces.

It also contains a patch from Daniel Lezcano which allows to have net devices with the same name in two different network namespaces.

Here is what I said when I sent the patchset a few days ago:

Quoted 27 lines

I re-post this patchset as most of network namespace work has been merged in net-2.6.26 and the issue is it is currently easy to test because of this missing piece.
(today sysfs must be disabled to test netns).

This feature is needed for network namespaces where we have to be able to have multiple network devices with the same name in different network namespaces.

On top of that, the final patch from Serge Hallyn also implements tagging for user namespaces.

Here is the announcement Eric wrote back in December to introduce his patchset:

"
Now that we have network namespace support merged it is time to revisit the sysfs support so we can remove the dependency on !SYSFS. [...]
The bulk of the patches are the changes to allow multiple sysfs superblocks.

Then comes the tagged directory sysfs support which uses information captured at mount time to decide which object with which tag will appear in a directory.

Quoted 6 lines

Then the support for renaming and deleting objects where the source may be ambiguous because of tagging.

Then finally the network namespace support so it is clear how all of this tied together.
"

Regards,
Benjamin

#1 lore raw

PATCH 1/11 sysfs: Support for preventing unmounts.

sysfs: Support for preventing unmounts.

To support mounting multiple instances of sysfs occassionally I need to walk through all of the currently present sysfs super blocks.

To allow this iteration this patch adds sysfs_grab_supers and sysfs_release_supers. While a piece of code is in a section surrounded by these no more sysfs super blocks will be either created or destroyed.

---
 fs/sysfs/mount.c |   79 +++++++++++++++++++++++++++++++++++++++++++++++++------
 fs/sysfs/sysfs.h |   10 ++++++
 2 files changed, 81 insertions(+), 8 deletions(-)
fs/sysfs/mount.c +71 -8
Index: linux-vanilla/fs/sysfs/mount.c
===================================================================
@@ -41,47 +41,110 @@ struct sysfs_dirent sysfs_root = {
 
 static int sysfs_fill_super(struct super_block *sb, void *data, int silent)
 {
-	struct inode *inode;
-	struct dentry *root;
+	struct sysfs_super_info *info = NULL;
+	struct inode *inode = NULL;
+	struct dentry *root = NULL;
+	int error;
 
 	sb->s_blocksize = PAGE_CACHE_SIZE;
 	sb->s_blocksize_bits = PAGE_CACHE_SHIFT;
 	sb->s_magic = SYSFS_MAGIC;
 	sb->s_op = &sysfs_ops;
 	sb->s_time_gran = 1;
-	sysfs_sb = sb;
+	if (!sysfs_sb)
+		sysfs_sb = sb;
+
+	error = -ENOMEM;
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		goto out_err;
 
 	/* get root inode, initialize and unlock it */
+	error = -ENOMEM;
 	inode = sysfs_get_inode(&sysfs_root);
 	if (!inode) {
 		pr_debug("sysfs: could not get root inode\n");
-		return -ENOMEM;
+		goto out_err;
 	}
 
 	/* instantiate and link root dentry */
+	error = -ENOMEM;
 	root = d_alloc_root(inode);
 	if (!root) {
 		pr_debug("%s: could not get root dentry!\n",__func__);
-		iput(inode);
-		return -ENOMEM;
+		goto out_err;
 	}
 	root->d_fsdata = &sysfs_root;
 	sb->s_root = root;
+	sb->s_fs_info = info;
 	return 0;
+
+out_err:
+	dput(root);
+	iput(inode);
+	kfree(info);
+	if (sysfs_sb == sb)
+		sysfs_sb = NULL;
+	return error;
 }
 
 static int sysfs_get_sb(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *data, struct vfsmount *mnt)
 {
-	return get_sb_single(fs_type, flags, data, sysfs_fill_super, mnt);
+	int rc;
+	mutex_lock(&sysfs_rename_mutex);
+	rc = get_sb_single(fs_type, flags, data, sysfs_fill_super, mnt);
+	mutex_unlock(&sysfs_rename_mutex);
+	return rc;
 }
 
-static struct file_system_type sysfs_fs_type = {
+struct file_system_type sysfs_fs_type = {
 	.name		= "sysfs",
 	.get_sb		= sysfs_get_sb,
 	.kill_sb	= kill_anon_super,
 };
 
+void sysfs_grab_supers(void)
+{
+	/* must hold sysfs_rename_mutex */
+	struct super_block *sb;
+	/* Loop until I have taken s_umount on all sysfs superblocks */
+restart:
+	spin_lock(&sb_lock);
+	list_for_each_entry(sb, &sysfs_fs_type.fs_supers, s_instances) {
+		if (sysfs_info(sb)->grabbed)
+			continue;
+		/* Wait for unmount activity to complete. */
+		if (sb->s_count < S_BIAS) {
+			sb->s_count += 1;
+			spin_unlock(&sb_lock);
+			down_read(&sb->s_umount);
+			drop_super(sb);
+			goto restart;
+		}
+		atomic_inc(&sb->s_active);
+		sysfs_info(sb)->grabbed = 1;
+	}
+	spin_unlock(&sb_lock);
+}
+
+void sysfs_release_supers(void)
+{
+	/* must hold sysfs_rename_mutex */
+	struct super_block *sb;
+restart:
+	spin_lock(&sb_lock);
+	list_for_each_entry(sb, &sysfs_fs_type.fs_supers, s_instances) {
+		if (!sysfs_info(sb)->grabbed)
+			continue;
+		sysfs_info(sb)->grabbed = 0;
+		spin_unlock(&sb_lock);
+		deactivate_super(sb);
+		goto restart;
+	}
+	spin_unlock(&sb_lock);
+}
+
 int __init sysfs_init(void)
 {
 	int err = -ENOMEM;
fs/sysfs/sysfs.h +10 -0
Index: linux-vanilla/fs/sysfs/sysfs.h
===================================================================
@@ -85,12 +85,22 @@ struct sysfs_addrm_cxt {
 	int			cnt;
 };
 
+struct sysfs_super_info {
+	int	grabbed;
+};
+
+#define sysfs_info(SB) ((struct sysfs_super_info *)(SB)->s_fs_info)
+
 /*
  * mount.c
  */
 extern struct sysfs_dirent sysfs_root;
 extern struct super_block *sysfs_sb;
 extern struct kmem_cache *sysfs_dir_cachep;
+extern struct file_system_type sysfs_fs_type;
+
+void sysfs_grab_supers(void);
+void sysfs_release_supers(void);
 
 /*
  * dir.c
#2 lore raw

PATCH 2/11 sysfs: sysfs_get_dentry add a sb parameter

In reply to Benjamin Thery

sysfs: sysfs_get_dentry add a sb parameter

In preparation for multiple mounts of sysfs add a superblock parameter to sysfs_get_dentry.

---
 fs/sysfs/dir.c   |   12 +++++++-----
 fs/sysfs/file.c  |    2 +-
 fs/sysfs/sysfs.h |    3 ++-
 3 files changed, 10 insertions(+), 7 deletions(-)
fs/sysfs/dir.c +7 -5
Index: linux-vanilla/fs/sysfs/dir.c
===================================================================
@@ -85,6 +85,7 @@ static void sysfs_unlink_sibling(struct 
 
 /**
  *	sysfs_get_dentry - get dentry for the given sysfs_dirent
+ *	@sb: superblock of the dentry to return
  *	@sd: sysfs_dirent of interest
  *
  *	Get dentry for @sd.  Dentry is looked up if currently not
@@ -97,9 +98,10 @@ static void sysfs_unlink_sibling(struct 
  *	RETURNS:
  *	Pointer to found dentry on success, ERR_PTR() value on error.
  */
-struct dentry *sysfs_get_dentry(struct sysfs_dirent *sd)
+struct dentry *sysfs_get_dentry(struct super_block *sb,
+				struct sysfs_dirent *sd)
 {
-	struct dentry *dentry = dget(sysfs_sb->s_root);
+	struct dentry *dentry = dget(sb->s_root);
 
 	while (dentry->d_fsdata != sd) {
 		struct sysfs_dirent *cur;
@@ -781,7 +783,7 @@ int sysfs_rename_dir(struct kobject * ko
 		goto out;	/* nothing to rename */
 
 	/* get the original dentry */
-	old_dentry = sysfs_get_dentry(sd);
+	old_dentry = sysfs_get_dentry(sysfs_sb, sd);
 	if (IS_ERR(old_dentry)) {
 		error = PTR_ERR(old_dentry);
 		old_dentry = NULL;
@@ -849,7 +851,7 @@ int sysfs_move_dir(struct kobject *kobj,
 		goto out;	/* nothing to move */
 
 	/* get dentries */
-	old_dentry = sysfs_get_dentry(sd);
+	old_dentry = sysfs_get_dentry(sysfs_sb, sd);
 	if (IS_ERR(old_dentry)) {
 		error = PTR_ERR(old_dentry);
 		old_dentry = NULL;
@@ -857,7 +859,7 @@ int sysfs_move_dir(struct kobject *kobj,
 	}
 	old_parent = old_dentry->d_parent;
 
-	new_parent = sysfs_get_dentry(new_parent_sd);
+	new_parent = sysfs_get_dentry(sysfs_sb, new_parent_sd);
 	if (IS_ERR(new_parent)) {
 		error = PTR_ERR(new_parent);
 		new_parent = NULL;
fs/sysfs/file.c +1 -1
Index: linux-vanilla/fs/sysfs/file.c
===================================================================
@@ -571,7 +571,7 @@ int sysfs_chmod_file(struct kobject *kob
 		goto out;
 
 	mutex_lock(&sysfs_rename_mutex);
-	victim = sysfs_get_dentry(victim_sd);
+	victim = sysfs_get_dentry(sysfs_sb, victim_sd);
 	mutex_unlock(&sysfs_rename_mutex);
 	if (IS_ERR(victim)) {
 		rc = PTR_ERR(victim);
fs/sysfs/sysfs.h +2 -1
Index: linux-vanilla/fs/sysfs/sysfs.h
===================================================================
@@ -112,7 +112,8 @@ extern spinlock_t sysfs_assoc_lock;
 extern const struct file_operations sysfs_dir_operations;
 extern const struct inode_operations sysfs_dir_inode_operations;
 
-struct dentry *sysfs_get_dentry(struct sysfs_dirent *sd);
+struct dentry *sysfs_get_dentry(struct super_block *sb,
+				struct sysfs_dirent *sd);
 struct sysfs_dirent *sysfs_get_active_two(struct sysfs_dirent *sd);
 void sysfs_put_active_two(struct sysfs_dirent *sd);
 void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,
#3 lore raw

PATCH 3/11 sysfs: Implement __sysfs_get_dentry

In reply to Benjamin Thery

sysfs: Implement __sysfs_get_dentry

This function is similar but much simpler to sysfs_get_dentry returns a sysfs dentry if one curently exists.

---
 fs/sysfs/dir.c |   39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)
fs/sysfs/dir.c +39 -0
Index: linux-vanilla/fs/sysfs/dir.c
===================================================================
@@ -768,6 +768,45 @@ void sysfs_remove_dir(struct kobject * k
 	__sysfs_remove_dir(sd);
 }
 
+/**
+ *	__sysfs_get_dentry - get dentry for the given sysfs_dirent
+ *	@sb: superblock of the dentry to return
+ *	@sd: sysfs_dirent of interest
+ *
+ *	Get dentry for @sd.  Only return a dentry if one currently
+ *	exists.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep)
+ *
+ *	RETURNS:
+ *	Pointer to found dentry on success, NULL on failure.
+ */
+static struct dentry *__sysfs_get_dentry(struct super_block *sb,
+					 struct sysfs_dirent *sd)
+{
+	struct inode *inode;
+	struct dentry *dentry = NULL;
+
+	inode = ilookup5_nowait(sysfs_sb, sd->s_ino, sysfs_ilookup_test, sd);
+	if (inode && !(inode->i_state & I_NEW)) {
+		struct dentry *alias;
+		spin_lock(&dcache_lock);
+		list_for_each_entry(alias, &inode->i_dentry, d_alias) {
+			if (!IS_ROOT(alias) && d_unhashed(alias))
+				continue;
+			if (alias->d_sb != sb)
+				continue;
+			dentry = alias;
+			dget_locked(dentry);
+			break;
+		}
+		spin_unlock(&dcache_lock);
+	}
+	iput(inode);
+	return dentry;
+}
+
 int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
 {
 	struct sysfs_dirent *sd = kobj->sd;
#4 lore raw

PATCH 4/11 sysfs: Rename Support multiple superblocks

In reply to Benjamin Thery

sysfs: Rename Support multiple superblocks

This patch modifies the sysfs_rename_dir and sysfs_move_dir routines to support multiple sysfs dentry tries rooted in different sysfs superblocks.

---
 fs/sysfs/dir.c |  194 +++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 136 insertions(+), 58 deletions(-)
fs/sysfs/dir.c +136 -58
Index: linux-vanilla/fs/sysfs/dir.c
===================================================================
@@ -807,43 +807,112 @@ static struct dentry *__sysfs_get_dentry
 	return dentry;
 }
 
+struct sysfs_rename_struct {
+	struct list_head list;
+	struct dentry *old_dentry;
+	struct dentry *new_dentry;
+	struct dentry *old_parent;
+	struct dentry *new_parent;
+};
+
+static void post_rename(struct list_head *head)
+{
+	struct sysfs_rename_struct *srs;
+	while (!list_empty(head)) {
+		srs = list_entry(head->next, struct sysfs_rename_struct, list);
+		dput(srs->old_dentry);
+		dput(srs->new_dentry);
+		dput(srs->old_parent);
+		dput(srs->new_parent);
+		list_del(&srs->list);
+		kfree(srs);
+	}
+}
+
+static int prep_rename(struct list_head *head,
+	struct sysfs_dirent *sd, struct sysfs_dirent *new_parent_sd,
+	const char *name)
+{
+	struct sysfs_rename_struct *srs;
+	struct super_block *sb;
+	struct dentry *dentry;
+	int error;
+
+	list_for_each_entry(sb, &sysfs_fs_type.fs_supers, s_instances) {
+		dentry = sysfs_get_dentry(sb, sd);
+		if (dentry == ERR_PTR(-EXDEV))
+			continue;
+		if (IS_ERR(dentry)) {
+			error = PTR_ERR(dentry);
+			goto err_out;
+		}
+
+		srs = kzalloc(sizeof(*srs), GFP_KERNEL);
+		if (!srs) {
+			dput(dentry);
+			goto err_out;
+		}
+
+		INIT_LIST_HEAD(&srs->list);
+		list_add(head, &srs->list);
+		srs->old_dentry = dentry;
+		srs->old_parent = dget(dentry->d_parent);
+
+		dentry = sysfs_get_dentry(sb, new_parent_sd);
+		if (IS_ERR(dentry)) {
+			error = PTR_ERR(dentry);
+			goto err_out;
+		}
+		srs->new_parent = dentry;
+
+		error = -ENOMEM;
+		dentry = d_alloc_name(srs->new_parent, name);
+		if (!dentry)
+			goto err_out;
+		srs->new_dentry = dentry;
+	}
+	return 0;
+
+err_out:
+	post_rename(head);
+	return error;
+}
+
 int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
 {
 	struct sysfs_dirent *sd = kobj->sd;
-	struct dentry *parent = NULL;
-	struct dentry *old_dentry = NULL, *new_dentry = NULL;
+	struct list_head todo;
+	struct sysfs_rename_struct *srs;
+	struct inode *parent_inode = NULL;
 	const char *dup_name = NULL;
 	int error;
 
+	INIT_LIST_HEAD(&todo);
 	mutex_lock(&sysfs_rename_mutex);
 
 	error = 0;
 	if (strcmp(sd->s_name, new_name) == 0)
 		goto out;	/* nothing to rename */
 
-	/* get the original dentry */
-	old_dentry = sysfs_get_dentry(sysfs_sb, sd);
-	if (IS_ERR(old_dentry)) {
-		error = PTR_ERR(old_dentry);
-		old_dentry = NULL;
-		goto out;
-	}
+	sysfs_grab_supers();
+	error = prep_rename(&todo, sd, sd->s_parent, new_name);
+	if (error)
+		goto out_release;
 
-	parent = old_dentry->d_parent;
+	error = -ENOMEM;
+	mutex_lock(&sysfs_mutex);
+	parent_inode = sysfs_get_inode(sd->s_parent);
+	mutex_unlock(&sysfs_mutex);
+	if (!parent_inode)
+		goto out_release;
 
-	/* lock parent and get dentry for new name */
-	mutex_lock(&parent->d_inode->i_mutex);
+	mutex_lock(&parent_inode->i_mutex);
 	mutex_lock(&sysfs_mutex);
 
 	error = -EEXIST;
 	if (sysfs_find_dirent(sd->s_parent, new_name))
 		goto out_unlock;
 
-	error = -ENOMEM;
-	new_dentry = d_alloc_name(parent, new_name);
-	if (!new_dentry)
-		goto out_unlock;
-
 	/* rename kobject and sysfs_dirent */
 	error = -ENOMEM;
 	new_name = dup_name = kstrdup(new_name, GFP_KERNEL);
@@ -858,17 +927,21 @@ int sysfs_rename_dir(struct kobject * ko
 	sd->s_name = new_name;
 
 	/* rename */
-	d_add(new_dentry, NULL);
-	d_move(old_dentry, new_dentry);
+	list_for_each_entry(srs, &todo, list) {
+		d_add(srs->new_dentry, NULL);
+		d_move(srs->old_dentry, srs->new_dentry);
+	}
 
 	error = 0;
- out_unlock:
+out_unlock:
 	mutex_unlock(&sysfs_mutex);
-	mutex_unlock(&parent->d_inode->i_mutex);
+	mutex_unlock(&parent_inode->i_mutex);
 	kfree(dup_name);
-	dput(old_dentry);
-	dput(new_dentry);
- out:
+out_release:
+	iput(parent_inode);
+	post_rename(&todo);
+	sysfs_release_supers();
+out:
 	mutex_unlock(&sysfs_rename_mutex);
 	return error;
 }
@@ -877,10 +950,12 @@ int sysfs_move_dir(struct kobject *kobj,
 {
 	struct sysfs_dirent *sd = kobj->sd;
 	struct sysfs_dirent *new_parent_sd;
-	struct dentry *old_parent, *new_parent = NULL;
-	struct dentry *old_dentry = NULL, *new_dentry = NULL;
+	struct list_head todo;
+	struct sysfs_rename_struct *srs;
+	struct inode *old_parent_inode = NULL, *new_parent_inode = NULL;
 	int error;
 
+	INIT_LIST_HEAD(&todo);
 	mutex_lock(&sysfs_rename_mutex);
 	BUG_ON(!sd->s_parent);
 	new_parent_sd = new_parent_kobj->sd ? new_parent_kobj->sd : &sysfs_root;
@@ -889,26 +964,29 @@ int sysfs_move_dir(struct kobject *kobj,
 	if (sd->s_parent == new_parent_sd)
 		goto out;	/* nothing to move */
 
-	/* get dentries */
-	old_dentry = sysfs_get_dentry(sysfs_sb, sd);
-	if (IS_ERR(old_dentry)) {
-		error = PTR_ERR(old_dentry);
-		old_dentry = NULL;
-		goto out;
-	}
-	old_parent = old_dentry->d_parent;
-
-	new_parent = sysfs_get_dentry(sysfs_sb, new_parent_sd);
-	if (IS_ERR(new_parent)) {
-		error = PTR_ERR(new_parent);
-		new_parent = NULL;
-		goto out;
-	}
+	sysfs_grab_supers();
+	error = prep_rename(&todo, sd, new_parent_sd, sd->s_name);
+	if (error)
+		goto out_release;
+
+	error = -ENOMEM;
+	mutex_lock(&sysfs_mutex);
+	old_parent_inode = sysfs_get_inode(sd->s_parent);
+	mutex_unlock(&sysfs_mutex);
+	if (!old_parent_inode)
+		goto out_release;
+
+	error = -ENOMEM;
+	mutex_lock(&sysfs_mutex);
+	new_parent_inode = sysfs_get_inode(new_parent_sd);
+	mutex_unlock(&sysfs_mutex);
+	if (!new_parent_inode)
+		goto out_release;
 
 again:
-	mutex_lock(&old_parent->d_inode->i_mutex);
-	if (!mutex_trylock(&new_parent->d_inode->i_mutex)) {
-		mutex_unlock(&old_parent->d_inode->i_mutex);
+	mutex_lock(&old_parent_inode->i_mutex);
+	if (!mutex_trylock(&new_parent_inode->i_mutex)) {
+		mutex_unlock(&old_parent_inode->i_mutex);
 		goto again;
 	}
 	mutex_lock(&sysfs_mutex);
@@ -917,14 +995,11 @@ again:
 	if (sysfs_find_dirent(new_parent_sd, sd->s_name))
 		goto out_unlock;
 
-	error = -ENOMEM;
-	new_dentry = d_alloc_name(new_parent, sd->s_name);
-	if (!new_dentry)
-		goto out_unlock;
-
 	error = 0;
-	d_add(new_dentry, NULL);
-	d_move(old_dentry, new_dentry);
+	list_for_each_entry(srs, &todo, list) {
+		d_add(srs->new_dentry, NULL);
+		d_move(srs->old_dentry, srs->new_dentry);
+	}
 
 	/* Remove from old parent's list and insert into new parent's list. */
 	sysfs_unlink_sibling(sd);
@@ -933,14 +1008,17 @@ again:
 	sd->s_parent = new_parent_sd;
 	sysfs_link_sibling(sd);
 
- out_unlock:
+out_unlock:
 	mutex_unlock(&sysfs_mutex);
-	mutex_unlock(&new_parent->d_inode->i_mutex);
-	mutex_unlock(&old_parent->d_inode->i_mutex);
- out:
-	dput(new_parent);
-	dput(old_dentry);
-	dput(new_dentry);
+	mutex_unlock(&new_parent_inode->i_mutex);
+	mutex_unlock(&old_parent_inode->i_mutex);
+
+out_release:
+	iput(new_parent_inode);
+	iput(old_parent_inode);
+	post_rename(&todo);
+	sysfs_release_supers();
+out:
 	mutex_unlock(&sysfs_rename_mutex);
 	return error;
 }
#5 lore raw

PATCH 5/11 sysfs: sysfs_chmod_file handle multiple superblocks

In reply to Benjamin Thery

sysfs: sysfs_chmod_file handle multiple superblocks

Teach sysfs_chmod_file how to handle multiple sysfs superblocks.

---
 fs/sysfs/file.c |   54 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 24 deletions(-)
fs/sysfs/file.c +30 -24
Index: linux-vanilla/fs/sysfs/file.c
===================================================================
@@ -560,7 +560,8 @@ EXPORT_SYMBOL_GPL(sysfs_add_file_to_grou
 int sysfs_chmod_file(struct kobject *kobj, struct attribute *attr, mode_t mode)
 {
 	struct sysfs_dirent *victim_sd = NULL;
-	struct dentry *victim = NULL;
+	struct super_block *sb;
+	struct dentry *victim;
 	struct inode * inode;
 	struct iattr newattrs;
 	int rc;
@@ -571,31 +572,36 @@ int sysfs_chmod_file(struct kobject *kob
 		goto out;
 
 	mutex_lock(&sysfs_rename_mutex);
-	victim = sysfs_get_dentry(sysfs_sb, victim_sd);
-	mutex_unlock(&sysfs_rename_mutex);
-	if (IS_ERR(victim)) {
-		rc = PTR_ERR(victim);
-		victim = NULL;
-		goto out;
-	}
-
-	inode = victim->d_inode;
+	sysfs_grab_supers();
+	list_for_each_entry(sb, &sysfs_fs_type.fs_supers, s_instances) {
+		victim = sysfs_get_dentry(sb, victim_sd);
+		if (victim == ERR_PTR(-EXDEV))
+			continue;
+		if (IS_ERR(victim)) {
+			rc = PTR_ERR(victim);
+			victim = NULL;
+			goto out_unlock;
+		}
+
+		inode = victim->d_inode;
+		mutex_lock(&inode->i_mutex);
+		newattrs.ia_mode = (mode & S_IALLUGO) |
+				   (inode->i_mode & ~S_IALLUGO);
+		newattrs.ia_valid = ATTR_MODE | ATTR_CTIME;
+		rc = notify_change(victim, &newattrs);
+		if (rc == 0) {
+			mutex_lock(&sysfs_mutex);
+			victim_sd->s_mode = newattrs.ia_mode;
+			mutex_unlock(&sysfs_mutex);
+		}
+		mutex_unlock(&inode->i_mutex);
 
-	mutex_lock(&inode->i_mutex);
-
-	newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
-	newattrs.ia_valid = ATTR_MODE | ATTR_CTIME;
-	rc = notify_change(victim, &newattrs);
-
-	if (rc == 0) {
-		mutex_lock(&sysfs_mutex);
-		victim_sd->s_mode = newattrs.ia_mode;
-		mutex_unlock(&sysfs_mutex);
+		dput(victim);
 	}
-
-	mutex_unlock(&inode->i_mutex);
- out:
-	dput(victim);
+out_unlock:
+	sysfs_release_supers();
+	mutex_unlock(&sysfs_rename_mutex);
+out:
 	sysfs_put(victim_sd);
 	return rc;
 }
#6 lore raw

PATCH 6/11 sysfs: Implement sysfs tagged directory support.

In reply to Benjamin Thery

sysfs: Implement sysfs tagged directory support.

The problem. When implementing a network namespace I need to be able to have multiple network devices with the same name. Currently this is a problem for /sys/class/net/*, /sys/devices/virtual/net/*, and potentially a few other directories of the form /sys/ ... /net/*.

What this patch does is to add an additional tag field to the sysfs dirent structure. For directories that should show different contents depending on the context such as /sys/class/net/, and /sys/devices/virtual/net/ this tag field is used to specify the context in which those directories should be visible. Effectively this is the same as creating multiple distinct directories with the same name but internally to sysfs the result is nicer.

I am calling the concept of a single directory that looks like multiple directories all at the same path in the filesystem tagged directories.

For the networking namespace the set of directories whose contents I need to filter with tags can depend on the presence or absence of hotplug hardware or which modules are currently loaded. Which means I need a simple race free way to setup those directories as tagged.

To achieve a race free design all tagged directories are created and managed by sysfs itself. The upper level code that knows what tagged directories we need provides just two methods that enable this:
sb_tag() - that returns a "void *" tag that identifies the context of
the process that mounted sysfs.
kobject_tag(kobj) - that returns a "void *" tag that identifies the context
a kobject should be in.
Everything else is left up to sysfs.

For the network namespace sb_tag and kobject_tag are essentially one line functions, and look to remain that.

The work needed in sysfs is more extensive. At each directory or symlink creating I need to check if the directory it is being created in is a tagged directory and if so generate the appropriate tag to place on the sysfs_dirent. Likewise at each symlink or directory removal I need to check if the sysfs directory it is being removed from is a tagged directory and if so figure out which tag goes along with the name I am deleting.

Currently only directories which hold kobjects, and symlinks are supported. There is not enough information in the current file attribute interfaces to give us anything to discriminate on which makes it useless, and there are no potential users which makes it an uninteresting problem to solve.

---
 fs/sysfs/bin.c        |    2 
 fs/sysfs/dir.c        |  185 ++++++++++++++++++++++++++++++++++++++++++++++----
 fs/sysfs/file.c       |    8 +-
 fs/sysfs/group.c      |    4 -
 fs/sysfs/inode.c      |    7 +
 fs/sysfs/mount.c      |   44 ++++++++++-
 fs/sysfs/symlink.c    |    2 
 fs/sysfs/sysfs.h      |   17 ++++
 include/linux/sysfs.h |   17 ++++
 9 files changed, 257 insertions(+), 29 deletions(-)
fs/sysfs/bin.c +1 -1
Index: linux-vanilla/fs/sysfs/bin.c
===================================================================
@@ -252,7 +252,7 @@ int sysfs_create_bin_file(struct kobject
 
 void sysfs_remove_bin_file(struct kobject * kobj, struct bin_attribute * attr)
 {
-	sysfs_hash_and_remove(kobj->sd, attr->attr.name);
+	sysfs_hash_and_remove(kobj, kobj->sd, attr->attr.name);
 }
 
 EXPORT_SYMBOL_GPL(sysfs_create_bin_file);
fs/sysfs/dir.c +171 -14

306 lines not on this page · raw message

fs/sysfs/file.c +4 -4
Index: linux-vanilla/fs/sysfs/file.c
===================================================================
@@ -447,9 +447,9 @@ void sysfs_notify(struct kobject *k, cha
 	mutex_lock(&sysfs_mutex);
 
 	if (sd && dir)
-		sd = sysfs_find_dirent(sd, dir);
+		sd = sysfs_find_dirent(sd, NULL, dir);
 	if (sd && attr)
-		sd = sysfs_find_dirent(sd, attr);
+		sd = sysfs_find_dirent(sd, NULL, attr);
 	if (sd) {
 		struct sysfs_open_dirent *od;
 
@@ -618,7 +618,7 @@ EXPORT_SYMBOL_GPL(sysfs_chmod_file);
 
 void sysfs_remove_file(struct kobject * kobj, const struct attribute * attr)
 {
-	sysfs_hash_and_remove(kobj->sd, attr->name);
+	sysfs_hash_and_remove(kobj, kobj->sd, attr->name);
 }
 
 
@@ -638,7 +638,7 @@ void sysfs_remove_file_from_group(struct
 	else
 		dir_sd = sysfs_get(kobj->sd);
 	if (dir_sd) {
-		sysfs_hash_and_remove(dir_sd, attr->name);
+		sysfs_hash_and_remove(kobj, dir_sd, attr->name);
 		sysfs_put(dir_sd);
 	}
 }
fs/sysfs/group.c +2 -2
Index: linux-vanilla/fs/sysfs/group.c
===================================================================
@@ -23,7 +23,7 @@ static void remove_files(struct sysfs_di
 	int i;
 
 	for (i = 0, attr = grp->attrs; *attr; i++, attr++)
-		sysfs_hash_and_remove(dir_sd, (*attr)->name);
+		sysfs_hash_and_remove(kobj, dir_sd, (*attr)->name);
 }
 
 static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
@@ -39,7 +39,7 @@ static int create_files(struct sysfs_dir
 		 * visibility.  Do this by first removing then
 		 * re-adding (if required) the file */
 		if (update)
-			sysfs_hash_and_remove(dir_sd, (*attr)->name);
+			sysfs_hash_and_remove(kobj, dir_sd, (*attr)->name);
 		if (grp->is_visible) {
 			mode = grp->is_visible(kobj, *attr, i);
 			if (!mode)
fs/sysfs/inode.c +5 -2
Index: linux-vanilla/fs/sysfs/inode.c
===================================================================
@@ -217,17 +217,20 @@ struct inode * sysfs_get_inode(struct sy
 	return inode;
 }
 
-int sysfs_hash_and_remove(struct sysfs_dirent *dir_sd, const char *name)
+int sysfs_hash_and_remove(struct kobject *kobj, struct sysfs_dirent *dir_sd,
+			  const char *name)
 {
 	struct sysfs_addrm_cxt acxt;
 	struct sysfs_dirent *sd;
+	const void *tag;
 
 	if (!dir_sd)
 		return -ENOENT;
 
 	sysfs_addrm_start(&acxt, dir_sd);
+	tag = sysfs_removal_tag(kobj, dir_sd);
 
-	sd = sysfs_find_dirent(dir_sd, name);
+	sd = sysfs_find_dirent(dir_sd, tag, name);
 	if (sd)
 		sysfs_remove_one(&acxt, sd);
 
fs/sysfs/mount.c +40 -4

66 lines not on this page · raw message

fs/sysfs/symlink.c +1 -1
Index: linux-vanilla/fs/sysfs/symlink.c
===================================================================
@@ -94,7 +94,7 @@ void sysfs_remove_link(struct kobject * 
 	else
 		parent_sd = kobj->sd;
 
-	sysfs_hash_and_remove(parent_sd, name);
+	sysfs_hash_and_remove(kobj, parent_sd, name);
 }
 
 static int sysfs_get_target_path(struct sysfs_dirent *parent_sd,
fs/sysfs/sysfs.h +16 -1
Index: linux-vanilla/fs/sysfs/sysfs.h
===================================================================
@@ -46,6 +46,10 @@ struct sysfs_dirent {
 	const char		*s_name;
 
 	union {
+		const struct sysfs_tagged_dir_operations	*ops;
+		const void 					*tag;
+	} s_tag;
+	union {
 		struct sysfs_elem_dir		s_dir;
 		struct sysfs_elem_symlink	s_symlink;
 		struct sysfs_elem_attr		s_attr;
@@ -69,6 +73,7 @@ struct sysfs_dirent {
 
 #define SYSFS_FLAG_MASK			~SYSFS_TYPE_MASK
 #define SYSFS_FLAG_REMOVED		0x0200
+#define SYSFS_FLAG_TAGGED		0x0400
 
 static inline unsigned int sysfs_type(struct sysfs_dirent *sd)
 {
@@ -87,6 +92,7 @@ struct sysfs_addrm_cxt {
 
 struct sysfs_super_info {
 	int	grabbed;
+	struct sysfs_tag_info tag;
 };
 
 #define sysfs_info(SB) ((struct sysfs_super_info *)(SB)->s_fs_info)
@@ -112,6 +118,13 @@ extern spinlock_t sysfs_assoc_lock;
 extern const struct file_operations sysfs_dir_operations;
 extern const struct inode_operations sysfs_dir_inode_operations;
 
+extern const void *sysfs_creation_tag(struct sysfs_dirent *parent_sd,
+				      struct sysfs_dirent *sd);
+extern const void *sysfs_removal_tag(struct kobject *kobj,
+				     struct sysfs_dirent *dir_sd);
+extern const void *sysfs_lookup_tag(struct sysfs_dirent *dir_sd,
+				    struct super_block *sb);
+extern const void *sysfs_dirent_tag(struct sysfs_dirent *sd);
 struct dentry *sysfs_get_dentry(struct super_block *sb,
 				struct sysfs_dirent *sd);
 struct sysfs_dirent *sysfs_get_active_two(struct sysfs_dirent *sd);
@@ -123,6 +136,7 @@ void sysfs_remove_one(struct sysfs_addrm
 void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt);
 
 struct sysfs_dirent *sysfs_find_dirent(struct sysfs_dirent *parent_sd,
+				       const void *tag,
 				       const unsigned char *name);
 struct sysfs_dirent *sysfs_get_dirent(struct sysfs_dirent *parent_sd,
 				      const unsigned char *name);
@@ -154,7 +168,8 @@ static inline void sysfs_put(struct sysf
  */
 struct inode *sysfs_get_inode(struct sysfs_dirent *sd);
 int sysfs_setattr(struct dentry *dentry, struct iattr *iattr);
-int sysfs_hash_and_remove(struct sysfs_dirent *dir_sd, const char *name);
+int sysfs_hash_and_remove(struct kobject *kobj, struct sysfs_dirent *dir_sd,
+			  const char *name);
 int sysfs_inode_init(void);
 
 /*
include/linux/sysfs.h +17 -0
Index: linux-vanilla/include/linux/sysfs.h
===================================================================
@@ -78,6 +78,14 @@ struct sysfs_ops {
 	ssize_t	(*store)(struct kobject *,struct attribute *,const char *, size_t);
 };
 
+struct sysfs_tag_info {
+};
+
+struct sysfs_tagged_dir_operations {
+	const void *(*sb_tag)(struct sysfs_tag_info *info);
+	const void *(*kobject_tag)(struct kobject *kobj);
+};
+
 #ifdef CONFIG_SYSFS
 
 int sysfs_schedule_callback(struct kobject *kobj, void (*func)(void *),
@@ -116,6 +124,9 @@ void sysfs_remove_file_from_group(struct
 
 void sysfs_notify(struct kobject *kobj, char *dir, char *attr);
 
+int sysfs_enable_tagging(struct kobject *,
+			 const struct sysfs_tagged_dir_operations *);
+
 extern int __must_check sysfs_init(void);
 
 #else /* CONFIG_SYSFS */
@@ -216,6 +227,12 @@ static inline void sysfs_notify(struct k
 {
 }
 
+static inline int sysfs_enable_tagging(struct kobject *kobj,
+		       const struct sysfs_tagged_dir_operations *tag_ops)
+{
+	return 0;
+}
+
 static inline int __must_check sysfs_init(void)
 {
 	return 0;
#7 lore raw

PATCH 7/11 sysfs: Implement sysfs_delete_link and sysfs_rename_link

In reply to Benjamin Thery

sysfs: Implement sysfs_delete_link and sysfs_rename_link

When removing a symlink sysfs_remove_link does not provide enough information to figure out which tagged directory the symlink falls in. So I need sysfs_delete_link which is passed the target of the symlink to delete.

Further half the time when we are removing a symlink the code is actually renaming the symlink but not doing so explicitly because we don't have a symlink rename method. So I have added sysfs_rename_link as well.

Both of these functions now have enough information to find a symlink in a tagged directory. The only restriction is that they must be called before the target kobject is renamed or deleted. If they are called later I loose track of which tag the target kobject was marked with and can no longer find the old symlink to remove it.

---
 fs/sysfs/symlink.c    |   31 +++++++++++++++++++++++++++++++
 include/linux/sysfs.h |   17 +++++++++++++++++
 2 files changed, 48 insertions(+)
fs/sysfs/symlink.c +31 -0
Index: linux-vanilla/fs/sysfs/symlink.c
===================================================================
@@ -80,6 +80,21 @@ int sysfs_create_link(struct kobject * k
 }
 
 /**
+ *	sysfs_delete_link - remove symlink in object's directory.
+ *	@kobj:	object we're acting for.
+ *	@targ:	object we're pointing to.
+ *	@name:	name of the symlink to remove.
+ *
+ *	Unlike sysfs_remove_link sysfs_delete_link has enough information
+ *	to successfully delete symlinks in tagged directories.
+ */
+void sysfs_delete_link(struct kobject *kobj, struct kobject *targ,
+			const char *name)
+{
+	sysfs_hash_and_remove(targ, kobj->sd, name);
+}
+
+/**
  *	sysfs_remove_link - remove symlink in object's directory.
  *	@kobj:	object we're acting for.
  *	@name:	name of the symlink to remove.
@@ -97,6 +112,22 @@ void sysfs_remove_link(struct kobject * 
 	sysfs_hash_and_remove(kobj, parent_sd, name);
 }
 
+/**
+ *	sysfs_rename_link - rename symlink in object's directory.
+ *	@kobj:	object we're acting for.
+ *	@targ:	object we're pointing to.
+ *	@old:	previous name of the symlink.
+ *	@new:	new name of the symlink.
+ *
+ *	A helper function for the common rename symlink idiom.
+ */
+int sysfs_rename_link(struct kobject *kobj, struct kobject *targ,
+			const char *old, const char *new)
+{
+	sysfs_delete_link(kobj, targ, old);
+	return sysfs_create_link(kobj, targ, new);
+}
+
 static int sysfs_get_target_path(struct sysfs_dirent *parent_sd,
 				 struct sysfs_dirent *target_sd, char *path)
 {
include/linux/sysfs.h +17 -0
Index: linux-vanilla/include/linux/sysfs.h
===================================================================
@@ -111,6 +111,12 @@ int __must_check sysfs_create_link(struc
 				   const char *name);
 void sysfs_remove_link(struct kobject *kobj, const char *name);
 
+int sysfs_rename_link(struct kobject *kobj, struct kobject *target,
+			const char *old_name, const char *new_name);
+
+void sysfs_delete_link(struct kobject *dir, struct kobject *targ,
+			const char *name);
+
 int __must_check sysfs_create_group(struct kobject *kobj,
 				    const struct attribute_group *grp);
 int sysfs_update_group(struct kobject *kobj,
@@ -195,6 +201,17 @@ static inline void sysfs_remove_link(str
 {
 }
 
+static inline int sysfs_rename_link(struct kobject *k, struct kobject *t,
+				    const char *old_name, const char *new_name)
+{
+	return 0;
+}
+
+static inline void sysfs_delete_link(struct kobject *k, struct kobject *t,
+				     const char *name)
+{
+}
+
 static inline int sysfs_create_group(struct kobject *kobj,
 				     const struct attribute_group *grp)
 {
#8 lore raw

PATCH 8/11 driver core: Implement tagged directory support for device classes.

In reply to Benjamin Thery

driver core: Implement tagged directory support for device classes.

This patch enables tagging on every class directory if struct class has tag_ops.

In addition device_del and device_rename were modified to use sysfs_delete_link and sysfs_rename_link respectively to ensure when these operations happen on devices whose classes have tag_ops that they work properly.

---
 drivers/base/class.c   |   30 ++++++++++++++++++++++++++----
 drivers/base/core.c    |   41 ++++++++++++++++++++++-------------------
 include/linux/device.h |    2 ++
 3 files changed, 50 insertions(+), 23 deletions(-)
drivers/base/class.c +26 -4
Index: linux-vanilla/drivers/base/class.c
===================================================================
@@ -134,6 +134,17 @@ static void remove_class_attrs(struct cl
 	}
 }
 
+static int class_setup_tagging(struct class *cls)
+{
+	const struct sysfs_tagged_dir_operations *tag_ops;
+
+	tag_ops = cls->tag_ops;
+	if (!tag_ops)
+		return 0;
+
+	return sysfs_enable_tagging(&cls->subsys.kobj, tag_ops);
+}
+
 int class_register(struct class *cls)
 {
 	int error;
@@ -159,11 +170,22 @@ int class_register(struct class *cls)
 	cls->subsys.kobj.ktype = &class_ktype;
 
 	error = kset_register(&cls->subsys);
-	if (!error) {
-		error = add_class_attrs(class_get(cls));
-		class_put(cls);
-	}
+	if (error)
+		goto out;
+
+	error = class_setup_tagging(cls);
+	if (error)
+		goto out_unregister;
+
+	error = add_class_attrs(cls);
+	if (error)
+		goto out_unregister;
+
+out:
 	return error;
+out_unregister:
+	kset_unregister(&cls->subsys);
+	goto out;
 }
 
 void class_unregister(struct class *cls)
drivers/base/core.c +22 -19

70 lines not on this page · raw message

include/linux/device.h +2 -0
Index: linux-vanilla/include/linux/device.h
===================================================================
@@ -198,6 +198,8 @@ struct class {
 
 	int (*suspend)(struct device *dev, pm_message_t state);
 	int (*resume)(struct device *dev);
+
+	const struct sysfs_tagged_dir_operations *tag_ops;
 };
 
 extern int __must_check class_register(struct class *class);
#9 lore raw

PATCH 9/11 netns: Enable tagging for net_class directories in sysfs

In reply to Benjamin Thery

net: Enable tagging for net_class directories in sysfs

The problem. Network devices show up in sysfs and with the network namespace active multiple devices with the same name can show up in the same directory, ouch!

To avoid that problem and allow existing applications in network namespaces to see the same interface that is currently presented in sysfs, this patch enables the tagging directory support in sysfs.

By using the network namespace pointers as tags to separate out the the sysfs directory entries we ensure that we don't have conflicts in the directories and applications only see a limited set of the network devices.

---
 fs/sysfs/mount.c      |   36 ++++++++++++++++++++++++++++++++++++
 include/linux/sysfs.h |    2 ++
 net/Kconfig           |    2 +-
 net/core/net-sysfs.c  |   20 ++++++++++++++++++++
 4 files changed, 59 insertions(+), 1 deletion(-)
fs/sysfs/mount.c +36 -0
Index: linux-vanilla/fs/sysfs/mount.c
===================================================================
@@ -16,6 +16,8 @@
 #include <linux/mount.h>
 #include <linux/pagemap.h>
 #include <linux/init.h>
+#include <linux/nsproxy.h>
+#include <net/net_namespace.h>
 
 #include "sysfs.h"
 
@@ -78,6 +80,7 @@ static int sysfs_fill_super(struct super
 	root->d_sb = sb;
 	sb->s_root = root;
 	sb->s_fs_info = info;
+	info->tag.net_ns = hold_net(current->nsproxy->net_ns);
 	return 0;
 
 out_err:
@@ -95,6 +98,9 @@ static int sysfs_test_super(struct super
 	struct sysfs_super_info *info = sysfs_info(sb);
 	int found = 1;
 
+	if (task->nsproxy->net_ns != info->tag.net_ns)
+		found = 0;
+
 	return found;
 }
 
@@ -131,6 +137,8 @@ static void sysfs_kill_sb(struct super_b
 	struct sysfs_super_info *info = sysfs_info(sb);
 
 	kill_anon_super(sb);
+	if (info->tag.net_ns)
+		release_net(info->tag.net_ns);
 	kfree(info);
 }
 
@@ -181,6 +189,31 @@ restart:
 	spin_unlock(&sb_lock);
 }
 
+#ifdef CONFIG_NET
+static void sysfs_net_exit(struct net *net)
+{
+	/* Allow the net namespace to go away while sysfs is still mounted. */
+	struct super_block *sb;
+	mutex_lock(&sysfs_rename_mutex);
+	sysfs_grab_supers();
+	mutex_lock(&sysfs_mutex);
+	list_for_each_entry(sb, &sysfs_fs_type.fs_supers, s_instances) {
+		struct sysfs_super_info *info = sysfs_info(sb);
+		if (info->tag.net_ns != net)
+			continue;
+		release_net(info->tag.net_ns);
+		info->tag.net_ns = NULL;
+	}
+	mutex_unlock(&sysfs_mutex);
+	sysfs_release_supers();
+	mutex_unlock(&sysfs_rename_mutex);
+}
+
+static struct pernet_operations sysfs_net_ops = {
+	.exit = sysfs_net_exit,
+};
+#endif
+
 int __init sysfs_init(void)
 {
 	int err = -ENOMEM;
@@ -205,6 +238,9 @@ int __init sysfs_init(void)
 			unregister_filesystem(&sysfs_fs_type);
 			goto out_err;
 		}
+#ifdef CONFIG_NET
+		register_pernet_subsys(&sysfs_net_ops);
+#endif
 	} else
 		goto out_err;
 out:
include/linux/sysfs.h +2 -0
Index: linux-vanilla/include/linux/sysfs.h
===================================================================
@@ -19,6 +19,7 @@
 
 struct kobject;
 struct module;
+struct net;
 
 /* FIXME
  * The *owner field is no longer used, but leave around
@@ -79,6 +80,7 @@ struct sysfs_ops {
 };
 
 struct sysfs_tag_info {
+	struct net *net_ns;
 };
 
 struct sysfs_tagged_dir_operations {
net/Kconfig +1 -1
Index: linux-vanilla/net/Kconfig
===================================================================
@@ -30,7 +30,7 @@ menu "Networking options"
 config NET_NS
 	bool "Network namespace support"
 	default n
-	depends on EXPERIMENTAL && !SYSFS && NAMESPACES
+	depends on EXPERIMENTAL && NAMESPACES
 	help
 	  Allow user space to create what appear to be multiple instances
 	  of the network stack.
net/core/net-sysfs.c +20 -0
Index: linux-vanilla/net/core/net-sysfs.c
===================================================================
@@ -13,7 +13,9 @@
 #include <linux/kernel.h>
 #include <linux/netdevice.h>
 #include <linux/if_arp.h>
+#include <linux/nsproxy.h>
 #include <net/sock.h>
+#include <net/net_namespace.h>
 #include <linux/rtnetlink.h>
 #include <linux/wireless.h>
 #include <net/iw_handler.h>
@@ -421,6 +423,23 @@ static void netdev_release(struct device
 	kfree((char *)dev - dev->padded);
 }
 
+static const void *net_sb_tag(struct sysfs_tag_info *info)
+{
+	return info->net_ns;
+}
+
+static const void *net_kobject_tag(struct kobject *kobj)
+{
+	struct net_device *dev;
+	dev = container_of(kobj, struct net_device, dev.kobj);
+	return dev_net(dev);
+}
+
+static const struct sysfs_tagged_dir_operations net_tagged_dir_operations = {
+	.sb_tag = net_sb_tag,
+	.kobject_tag = net_kobject_tag,
+};
+
 static struct class net_class = {
 	.name = "net",
 	.dev_release = netdev_release,
@@ -430,6 +449,7 @@ static struct class net_class = {
 #ifdef CONFIG_HOTPLUG
 	.dev_uevent = netdev_uevent,
 #endif
+	.tag_ops = &net_tagged_dir_operations,
 };
 
 /* Delete sysfs entries but hold kobject reference until after all
#10 lore raw

PATCH 10/11 avoid kobject name conflict with different namespaces

In reply to Benjamin Thery

The renaming of a kobject will fail if there is another kobject with the same name belonging to another namespace.

This patch makes the kobject lookup in kobject_rename to check if the object exists _and_ belongs to the same namespace.

---
 fs/sysfs/dir.c        |   10 ++++++++++
 include/linux/sysfs.h |    7 +++++++
 lib/kobject.c         |    2 +-
 3 files changed, 18 insertions(+), 1 deletion(-)
fs/sysfs/dir.c +10 -0
Index: linux-vanilla/fs/sysfs/dir.c
===================================================================
@@ -902,6 +902,16 @@ err_out:
 	return error;
 }
 
+int sysfs_tag_cmp(struct kobject *kobj1, struct kobject *kobj2)
+{
+	struct sysfs_dirent *sd1 = kobj1->sd;
+	struct sysfs_dirent *sd2 = kobj2->sd;
+	const void *tag1 = sysfs_dirent_tag(sd1);
+	const void *tag2 = sysfs_dirent_tag(sd2);
+
+	return tag1 != tag2;
+}
+
 int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
 {
 	struct sysfs_dirent *sd = kobj->sd;
include/linux/sysfs.h +7 -0
Index: linux-vanilla/include/linux/sysfs.h
===================================================================
@@ -95,6 +95,8 @@ int sysfs_schedule_callback(struct kobje
 
 int __must_check sysfs_create_dir(struct kobject *kobj);
 void sysfs_remove_dir(struct kobject *kobj);
+int sysfs_tag_cmp(struct kobject *kobj1, struct kobject *kobj2);
+
 int __must_check sysfs_rename_dir(struct kobject *kobj, const char *new_name);
 int __must_check sysfs_move_dir(struct kobject *kobj,
 				struct kobject *new_parent_kobj);
@@ -154,6 +156,11 @@ static inline void sysfs_remove_dir(stru
 {
 }
 
+static inline int sysfs_tag_cmp(struct kobject *kobj1, struct kobject *kobj2)
+{
+	return 0;
+}
+
 static inline int sysfs_rename_dir(struct kobject *kobj, const char *new_name)
 {
 	return 0;
lib/kobject.c +1 -1
Index: linux-vanilla/lib/kobject.c
===================================================================
@@ -401,7 +401,7 @@ int kobject_rename(struct kobject *kobj,
 	if (kobj->kset) {
 		struct kobject *temp_kobj;
 		temp_kobj = kset_find_obj(kobj->kset, new_name);
-		if (temp_kobj) {
+		if (temp_kobj && !sysfs_tag_cmp(temp_kobj, kobj)) {
 			printk(KERN_WARNING "kobject '%s' cannot be renamed "
 			       "to '%s' as '%s' is already in existence.\n",
 			       kobject_name(kobj), new_name, new_name);
#11 lore raw

Re: [PATCH 10/11] avoid kobject name conflict with different namespaces

Quoted 5 lines · Benjamin Thery

The renaming of a kobject will fail if there is another kobject with the same name belonging to another namespace.

This patch makes the kobject lookup in kobject_rename to check if the object exists _and_ belongs to the same namespace.

Ok so we are dealing with fallout from:

commit 34358c26a2c96b2a068dc44e0ac602106a466bce

Author: Greg Kroah-Hartman <[email protected]>
Date:   Wed Oct 24 16:52:31 2007 -0700

kobject: check for duplicate names in kobject_rename

This should catch any duplicate names before we try to tell sysfs to rename the object. This happens a lot with older versions of udev and the network rename scripts.

Cc David Miller <[email protected]>
Cc Kay Sievers <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
Cc Tejun Heo <[email protected]>

Which added the check in kobject_rename to prevent problems, and seems to be causing a few.

I believe this earlier? patch addresses the problem:

commit c8d90dca3211966ba5189e0f3d4bccd558d9ae08

Author: Stephen Hemminger <[email protected]>
Date:   Fri Oct 26 03:53:42 2007 -0700

[NET] dev_change_name: ignore changes to same name

Prevent error/backtrace from dev_rename() when changing name of network device to the same name. This is a common situation with udev and other scripts that bind addr to device.

And the challenge is that we are getting false positives in the check to see if renames will fail.

	/* see if this name is already in use */
	if (kobj->kset) {
		struct kobject *temp_kobj;
		temp_kobj = kset_find_obj(kobj->kset, new_name);
		if (temp_kobj) {
			printk(KERN_WARNING "kobject '%s' can not be renamed 
				"to '%s' as '%s' is already in existance.\n",
				kobject_name(kobj), new_name, new_name);
			kobject_put(temp_kobj);
			return -EINVAL;
		}
	}

If the kobject layer wants to perform the test above how do we support it by giving it enough information to perform the test without false positives.

Certainly we can go to the sysfs layer but that has the problem of being a layering violation and not working when sysfs is not compiled in. Ouch!

I believe what the sanity check should look like is:

	/* see if this name is already in use */
	if (kobj->kset) {
		struct kobject *temp_kobj;
*               void *tag;
*               tag = kobject_tag(kobj);
*		temp_kobj = kset_find_tagged_obj(kobj->kset, tag, new_name);
*		if (temp_kobj && (temp_kobj != kobj)) {
			printk(KERN_WARNING "kobject '%s' can not be renamed 
				"to '%s' as '%s' is already in existance.\n",
				kobject_name(kobj), new_name, new_name);
			kobject_put(temp_kobj);
			return -EINVAL;
		}
	}

The tricky part is how do we get to kobject_tag (from the sysfs_tagged_dir_operations).

Unless there is another path I think placing an additional pointer in kobj_type so we can find it through ktype is the simplest solution. Although using the kset is also sane.

The easiest and most trivially correct thing to do would be to simply remove the unnecessary check from kobject_rename. We perform the check at the upper levels in the network anyway. And kobject_rename is only used by the network stack.

....

As for the actual patch itself I have two nits to pick.

Quoted 13 lines
---
 fs/sysfs/dir.c        |   10 ++++++++++
 include/linux/sysfs.h |    7 +++++++
 lib/kobject.c         |    2 +-
 3 files changed, 18 insertions(+), 1 deletion(-)

Index: linux-vanilla/fs/sysfs/dir.c
===================================================================

fs/sysfs/dir.c err_out: · line 910

Quoted 9 lines
fs/sysfs/dir.c +6 -0
@@ -902,6 +902,16 @@ err_out:
 	return error;
 }
 
+int sysfs_tag_cmp(struct kobject *kobj1, struct kobject *kobj2)
+{
+	struct sysfs_dirent *sd1 = kobj1->sd;
+	struct sysfs_dirent *sd2 = kobj2->sd;
+	const void *tag1 = sysfs_dirent_tag(sd1);
+	const void *tag2 = sysfs_dirent_tag(sd2);

The new name should be compared with sysfs_creation_tag in case we are dealing with the case of renaming across network
namespaces. We could use sysfs_creation_tag for both as
the only time the dirent_tag and creation_tag should differ is during a rename operation.

Quoted 21 lines
fs/sysfs/dir.c +4 -0
+
+	return tag1 != tag2;
+}
+
 int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
 {
 	struct sysfs_dirent *sd = kobj->sd;

Index: linux-vanilla/include/linux/sysfs.h
===================================================================

include/linux/sysfs.h +2 -0
@@ -95,6 +95,8 @@ int sysfs_schedule_callback(struct kobje
 
 int __must_check sysfs_create_dir(struct kobject *kobj);
 void sysfs_remove_dir(struct kobject *kobj);
+int sysfs_tag_cmp(struct kobject *kobj1, struct kobject *kobj2);
+
 int __must_check sysfs_rename_dir(struct kobject *kobj, const char *new_name);
 int __must_check sysfs_move_dir(struct kobject *kobj,
 				struct kobject *new_parent_kobj);

include/linux/sysfs.h static inline void sysfs_remove_dir(stru · line 163

Quoted 8 lines
include/linux/sysfs.h +5 -0
@@ -154,6 +156,11 @@ static inline void sysfs_remove_dir(stru
 {
 }
 
+static inline int sysfs_tag_cmp(struct kobject *kobj1, struct kobject *kobj2)
+{
+	return 0;
+}
+

Either I am blind or this implementation breaks when we are using kobjects and sysfs support is not compiled in. It might be that we don't do this work but still in principle this is a small bug.

Quoted 18 lines
include/linux/sysfs.h +0 -0
 static inline int sysfs_rename_dir(struct kobject *kobj, const char *new_name)
 {
 	return 0;

Index: linux-vanilla/lib/kobject.c
===================================================================

lib/kobject.c +1 -1
@@ -401,7 +401,7 @@ int kobject_rename(struct kobject *kobj,
 	if (kobj->kset) {
 		struct kobject *temp_kobj;
 		temp_kobj = kset_find_obj(kobj->kset, new_name);
-		if (temp_kobj) {
+		if (temp_kobj && !sysfs_tag_cmp(temp_kobj, kobj)) {
 			printk(KERN_WARNING "kobject '%s' cannot be renamed "
 			       "to '%s' as '%s' is already in existence.\n",
 			       kobject_name(kobj), new_name, new_name);
Signature · 1 line
-- 
#12 lore raw

Re: [PATCH 10/11] avoid kobject name conflict with different namespaces

Quoted 3 lines · Eric W. Biederman

Unless there is another path I think placing an additional pointer in kobj_type so we can find it through ktype is the simplest solution. Although using the kset is also sane.

Ick, ick, ick :)

Quoted 4 lines

The easiest and most trivially correct thing to do would be to simply remove the unnecessary check from kobject_rename. We perform the check at the upper levels in the network anyway. And kobject_rename is only used by the network stack.

Wireless uses it also for some things, and it requires that it fail if a duplicate is found. I thought that s390 also used it, but I don't see that usage in the tree anymore, perhaps they switched to something else.

good luck,

greg k-h

#13 lore raw

Re: [PATCH 10/11] avoid kobject name conflict with different namespaces

Quoted 6 lines · Greg KH
Quoted 3 lines · Eric W. Biederman

Unless there is another path I think placing an additional pointer in kobj_type so we can find it through ktype is the simplest solution. Although using the kset is also sane.

Ick, ick, ick :)

Well yes we are dealing with a pile of seemingly unnecessary layers, that are attempting to add uniformity where no uniformity existed. That is an easy recipe for ick.

Quoted 7 lines
Quoted 4 lines

The easiest and most trivially correct thing to do would be to simply remove the unnecessary check from kobject_rename. We perform the check at the upper levels in the network anyway. And kobject_rename is only used by the network stack.

Wireless uses it also for some things, and it requires that it fail if a duplicate is found.

Looks like it, that use is brand new, and at first glance I thought it was another instance of the device_rename case that kobject uses. Apparently not.

It looks like getting the wireless devices into the network namespace is going to be interesting. Since this phy name is user controllable and shows up in rtnetlink messages it definitely appears to belong in the network namespace.

Virtual/logical devices are such a pain.

....

Given that kobject_rename is growing users we definitely need to fix it so a noop rename does not return -EINVAL.

i.e. if (temp_kobj && (temp_kobj != kobj)) return -EINVAL. instead of just if (temp_kobj) return -EINVAL;

Quoted 2 lines

I thought that s390 also used it, but I don't see
that usage in the tree anymore, perhaps they switched to something else.

That is the device_move -> kobject_move case. Very similar (and nice if we can figure out how to combine them).

Quoted 1 line

good luck,

Thanks. It looks like we are just about there.

Hopefully we can get this merged soon so there won't be many more sets of shifting requirements to chase.

Eric

#14 lore raw

Re: [PATCH 10/11] avoid kobject name conflict with different namespaces

Quoted 4 lines · On Wed, 07 May 2008 13:54:27 -0700, [email protected] (Eric W. Biederman)…
Quoted 2 lines

I thought that s390 also used it, but I don't see
that usage in the tree anymore, perhaps they switched to something else.

That is the device_move -> kobject_move case.

Yes. bluetooth also uses it, IIRC.

Quoted 2 lines

Very similar (and nice
if we can figure out how to combine them).

Sounds like a good idea. I can test the _move() stuff (after I've managed to find some time to try this patchset...)

#15 lore raw

Re: [PATCH 10/11] avoid kobject name conflict with different namespaces

Quoted 9 lines · Cornelia Huck
Quoted 4 lines · On Wed, 07 May 2008 13:54:27 -0700, [email protected] (Eric W. Biederman)…
Quoted 2 lines

I thought that s390 also used it, but I don't see
that usage in the tree anymore, perhaps they switched to something else.

That is the device_move -> kobject_move case.

Yes. bluetooth also uses it, IIRC.

Yes.

Quoted 5 lines
Quoted 2 lines

Very similar (and nice
if we can figure out how to combine them).

Sounds like a good idea. I can test the _move() stuff (after I've managed to find some time to try this patchset...)

Do you remember if we have ever sorted out the race between _move and module unload/directory teardown at the sysfs level?

Eric

#16 lore raw

Re: [PATCH 10/11] avoid kobject name conflict with different namespaces

Quoted 2 lines · On Thu, 08 May 2008 12:28:19 -0700, [email protected] (Eric W. Biederman)…

Do you remember if we have ever sorted out the race between _move and module unload/directory teardown at the sysfs level?

Unfortunately I can't remember anything about this, do you have a pointer to the original discussion?

#17 lore raw

Re: [PATCH 10/11] avoid kobject name conflict with different namespaces

Quoted 8 lines · Cornelia Huck
Quoted 2 lines · On Thu, 08 May 2008 12:28:19 -0700, [email protected] (Eric W. Biederman)…

Do you remember if we have ever sorted out the race between _move and module unload/directory teardown at the sysfs level?

Unfortunately I can't remember anything about this, do you have a pointer to the original discussion?

Ok the conversation was:
"[Bluez-devel] Oops involving RFCOMM and sysfs"
http://lkml.org/lkml/2007/12/28/87

Following the thread through it appears the bug was fixed and it wasn't the race Al Viro was concerned about.
http://lkml.org/lkml/2008/1/9/59

And it looks like all of the symptoms have been corrected. Yeah!

Which implies to me that kobject_move has the same locking restrictions that kobject_rename has. I.e. the upper level better do what it takes to make the operation safe.

Which says that at least for now not worrying about the locking between sysfs_move_dir/sysfs_rename_dir and sysfs_remove_dir is ok. As we require the upper levels not to do that.

Which says that we may be able to figure out how to merge sysfs_move_dir and sysfs_rename_dir. It is still a nasty business though.

Eric

#18 lore raw

Re: [PATCH 10/11] avoid kobject name conflict with different namespaces

In reply to Greg KH
Quoted 7 lines · Greg KH
Quoted 3 lines · Eric W. Biederman

Unless there is another path I think placing an additional pointer in kobj_type so we can find it through ktype is the simplest solution. Although using the kset is also sane.

Ick, ick, ick :)

Quoted 8 lines
Quoted 4 lines

The easiest and most trivially correct thing to do would be to simply remove the unnecessary check from kobject_rename. We perform the check at the upper levels in the network anyway. And kobject_rename is only used by the network stack.

Wireless uses it also for some things, and it requires that it fail if a duplicate is found. I thought that s390 also used it, but I don't see that usage in the tree anymore, perhaps they switched to something else.

Looking at this a little further kobject_rename is a complete noop when sysfs support is not compiled in. That is the kobject does not get renamed, even if the higher level objects do.

This makes the wireless dependency on an error code from kobject_rename completely bogus as the kobject layer is not prepared to give any kind of reliable result, and it makes kobject_rename completely bogus if sysfs support is not compiled in.

Further there is no locking to guarantee renames are atomic or mutually exclusive at the kobject level.

With no locking and code that does nothing in the absence of sysfs attempting to check renames for validity at the kobject level (when renames don't happen at the kobject level) is totally bogus.

Since renames don't happen at the kobject level checking them for sanity at the kobject level makes no sense.

Eric

#19 lore raw

PATCH 1/1 wireless: Add missing locking to cfg80211_dev_rename

In reply to Greg KH

device_rename only performs useful and race free validity checking at the optional sysfs level so depending on it for all of the validity checking in cfg80211_dev_rename is racy.

Instead implement all of the needed validity checking and locking in cfg80211_dev_rename.

---
 net/wireless/core.c |   33 ++++++++++++++++++++++++++++-----
 1 files changed, 28 insertions(+), 5 deletions(-)
net/wireless/core.c +28 -5
index 80afacd..f1da0b9 100644
@@ -143,8 +143,11 @@ void cfg80211_put_dev(struct cfg80211_registered_device *drv)
 int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,
 			char *newname)
 {
+	struct cfg80211_registered_device *drv;
 	int idx, taken = -1, result, digits;
 
+	mutex_lock(&cfg80211_drv_mutex);
+
 	/* prohibit calling the thing phy%d when %d is not its number */
 	sscanf(newname, PHY_NAME "%d%n", &idx, &taken);
 	if (taken == strlen(newname) && idx != rdev->idx) {
@@ -156,14 +159,30 @@ int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,
 		 * deny the name if it is phy<idx> where <idx> is printed
 		 * without leading zeroes. taken == strlen(newname) here
 		 */
+		result = -EINVAL;
 		if (taken == strlen(PHY_NAME) + digits)
-			return -EINVAL;
+			goto out_unlock;
+	}
+
+
+	/* Ignore nop renames */
+	result = 0;
+	if (strcmp(newname, dev_name(&rdev->wiphy.dev)) == 0)
+		goto out_unlock;
+
+	/* Ensure another device does not already have this name. */
+	list_for_each_entry(drv, &cfg80211_drv_list, list) {
+		result = -EINVAL;
+		if (strcmp(newname, dev_name(&drv->wiphy.dev)) == 0)
+			goto out_unlock;
 	}
 
-	/* this will check for collisions */
+	/* this will only check for collisions in sysfs
+	 * which is not even always compiled in.
+	 */
 	result = device_rename(&rdev->wiphy.dev, newname);
 	if (result)
-		return result;
+		goto out_unlock;
 
 	if (!debugfs_rename(rdev->wiphy.debugfsdir->d_parent,
 			    rdev->wiphy.debugfsdir,
@@ -172,9 +191,13 @@ int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,
 		printk(KERN_ERR "cfg80211: failed to rename debugfs dir to %s!\n",
 		       newname);
 
-	nl80211_notify_dev_rename(rdev);
+	result = 0;
+out_unlock:
+	mutex_unlock(&cfg80211_drv_mutex);
+	if (result == 0)
+		nl80211_notify_dev_rename(rdev);
 
-	return 0;
+	return result;
 }
 
 /* exported functions */
Signature · 1 line
1.5.3.rc6.17.g1911
#20 lore raw

Re: [PATCH] wireless: Add missing locking to cfg80211_dev_rename

Quoted 10 lines · Eric W. Biederman

device_rename only performs useful and race free validity checking at the optional sysfs level so depending on it for all of the validity checking in cfg80211_dev_rename is racy.

Instead implement all of the needed validity checking and locking in cfg80211_dev_rename.

I'm about as far as you can get from either a sysfs/kobject or network expert, but both of these patches as well as your general concept that renames should be handled at sysfs seem to me to make perfect sense.

thanks,
-serge

Quoted 72 lines
---
 net/wireless/core.c |   33 ++++++++++++++++++++++++++++-----
 1 files changed, 28 insertions(+), 5 deletions(-)
net/wireless/core.c +28 -5
index 80afacd..f1da0b9 100644
@@ -143,8 +143,11 @@ void cfg80211_put_dev(struct cfg80211_registered_device *drv)
 int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,
 			char *newname)
 {
+	struct cfg80211_registered_device *drv;
 	int idx, taken = -1, result, digits;
 
+	mutex_lock(&cfg80211_drv_mutex);
+
 	/* prohibit calling the thing phy%d when %d is not its number */
 	sscanf(newname, PHY_NAME "%d%n", &idx, &taken);
 	if (taken == strlen(newname) && idx != rdev->idx) {
@@ -156,14 +159,30 @@ int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,
 		 * deny the name if it is phy<idx> where <idx> is printed
 		 * without leading zeroes. taken == strlen(newname) here
 		 */
+		result = -EINVAL;
 		if (taken == strlen(PHY_NAME) + digits)
-			return -EINVAL;
+			goto out_unlock;
+	}
+
+
+	/* Ignore nop renames */
+	result = 0;
+	if (strcmp(newname, dev_name(&rdev->wiphy.dev)) == 0)
+		goto out_unlock;
+
+	/* Ensure another device does not already have this name. */
+	list_for_each_entry(drv, &cfg80211_drv_list, list) {
+		result = -EINVAL;
+		if (strcmp(newname, dev_name(&drv->wiphy.dev)) == 0)
+			goto out_unlock;
 	}
 
-	/* this will check for collisions */
+	/* this will only check for collisions in sysfs
+	 * which is not even always compiled in.
+	 */
 	result = device_rename(&rdev->wiphy.dev, newname);
 	if (result)
-		return result;
+		goto out_unlock;
 
 	if (!debugfs_rename(rdev->wiphy.debugfsdir->d_parent,
 			    rdev->wiphy.debugfsdir,
@@ -172,9 +191,13 @@ int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,
 		printk(KERN_ERR "cfg80211: failed to rename debugfs dir to %s!\n",
 		       newname);
 
-	nl80211_notify_dev_rename(rdev);
+	result = 0;
+out_unlock:
+	mutex_unlock(&cfg80211_drv_mutex);
+	if (result == 0)
+		nl80211_notify_dev_rename(rdev);
 
-	return 0;
+	return result;
 }
 
 /* exported functions */
Signature · 1 line
1.5.3.rc6.17.g1911
maintainer Acked-by on v1 1/1
#21 lore raw

Re: [PATCH] wireless: Add missing locking to cfg80211_dev_rename

In reply to Eric W. Biederman
Quoted 7 lines · Eric W. Biederman

device_rename only performs useful and race free validity checking at the optional sysfs level so depending on it for all of the validity checking in cfg80211_dev_rename is racy.

Instead implement all of the needed validity checking and locking in cfg80211_dev_rename.

Makes sense, thanks, I didn't really think about it not being compiled in.

Quoted 1 line
Quoted 70 lines
---
 net/wireless/core.c |   33 ++++++++++++++++++++++++++++-----
 1 files changed, 28 insertions(+), 5 deletions(-)
net/wireless/core.c +28 -5
index 80afacd..f1da0b9 100644
@@ -143,8 +143,11 @@ void cfg80211_put_dev(struct cfg80211_registered_device *drv)
 int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,
 			char *newname)
 {
+	struct cfg80211_registered_device *drv;
 	int idx, taken = -1, result, digits;
 
+	mutex_lock(&cfg80211_drv_mutex);
+
 	/* prohibit calling the thing phy%d when %d is not its number */
 	sscanf(newname, PHY_NAME "%d%n", &idx, &taken);
 	if (taken == strlen(newname) && idx != rdev->idx) {
@@ -156,14 +159,30 @@ int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,
 		 * deny the name if it is phy<idx> where <idx> is printed
 		 * without leading zeroes. taken == strlen(newname) here
 		 */
+		result = -EINVAL;
 		if (taken == strlen(PHY_NAME) + digits)
-			return -EINVAL;
+			goto out_unlock;
+	}
+
+
+	/* Ignore nop renames */
+	result = 0;
+	if (strcmp(newname, dev_name(&rdev->wiphy.dev)) == 0)
+		goto out_unlock;
+
+	/* Ensure another device does not already have this name. */
+	list_for_each_entry(drv, &cfg80211_drv_list, list) {
+		result = -EINVAL;
+		if (strcmp(newname, dev_name(&drv->wiphy.dev)) == 0)
+			goto out_unlock;
 	}
 
-	/* this will check for collisions */
+	/* this will only check for collisions in sysfs
+	 * which is not even always compiled in.
+	 */
 	result = device_rename(&rdev->wiphy.dev, newname);
 	if (result)
-		return result;
+		goto out_unlock;
 
 	if (!debugfs_rename(rdev->wiphy.debugfsdir->d_parent,
 			    rdev->wiphy.debugfsdir,
@@ -172,9 +191,13 @@ int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,
 		printk(KERN_ERR "cfg80211: failed to rename debugfs dir to %s!\n",
 		       newname);
 
-	nl80211_notify_dev_rename(rdev);
+	result = 0;
+out_unlock:
+	mutex_unlock(&cfg80211_drv_mutex);
+	if (result == 0)
+		nl80211_notify_dev_rename(rdev);
 
-	return 0;
+	return result;
 }
 
 /* exported functions */
#22 lore raw

PATCH 1/1 Fix kobject_rename and !CONFIG_SYSFS

In reply to Greg KH

When looking at kobject_rename I found two bugs with that exist when sysfs support is disabled in the kernel.

kobject_rename does not change the name on the kobject when sysfs support is not compiled in.

kobject_rename without locking attempts to check the validity of a rename operation, which the kobject layer simply does not have the infrastructure to do so.

This patch documents the previously unstated requirement of kobject_rename that is the responsibility of the caller to provide mutual exclusion and to be certain that the new_name for the kobject is valid.

This patch modifies sysfs_rename_dir in !CONFIG_SYSFS case to call kobject_set_name to actually change the kobject_name.

This patch removes the bogus and misleading check in kobject_rename that attempts to see if a rename is valid. The check is bogus because we do not have the proper locking. The check is misleading because it looks like we can and do perform useful checking at the kobject level that we don't.

The users in net/core/dev.c already have all of the necessary checks in place and don't care. The other use in net/core/wireless.c as originally implemented is incorrect because it performs no locking and a simple patch has been sent that adds the necessary locking and sanity checks there. Ensuring this patch will not have an effect on users of kobject_rename or device_rename.

---
 Documentation/kobject.txt |    4 ++++
 drivers/base/core.c       |    5 +++++
 include/linux/sysfs.h     |    2 +-
 lib/kobject.c             |   18 +++++-------------
 4 files changed, 15 insertions(+), 14 deletions(-)
Documentation/kobject.txt +4 -0
index bf3256e..79184b4 100644
@@ -118,6 +118,10 @@ the name of the kobject, call kobject_rename():
 
     int kobject_rename(struct kobject *kobj, const char *new_name);
 
+Note kobject_rename does perform any locking or have a solid notion of
+what names are valid so the provide must provide their own sanity checking
+and serialization.
+
 There is a function called kobject_set_name() but that is legacy cruft and
 is being removed.  If your code needs to call this function, it is
 incorrect and needs to be fixed.
drivers/base/core.c +5 -0
index be288b5..ad68f4c 100644
@@ -1171,6 +1171,11 @@ EXPORT_SYMBOL_GPL(device_destroy);
  * device_rename - renames a device
  * @dev: the pointer to the struct device to be renamed
  * @new_name: the new name of the device
+ *
+ * It is the responsibility of the caller to provide mutual
+ * exclusion between two different calls of device_rename
+ * on the same device to ensure that new_name is valid and
+ * won't conflict with other devices.
  */
 int device_rename(struct device *dev, char *new_name)
 {
include/linux/sysfs.h +1 -1
index 7858eac..db66fa5 100644
@@ -137,7 +137,7 @@ static inline void sysfs_remove_dir(struct kobject *kobj)
 
 static inline int sysfs_rename_dir(struct kobject *kobj, const char *new_name)
 {
-	return 0;
+	return kobject_set_name(kobj, "%s", new_name);
 }
 
 static inline int sysfs_move_dir(struct kobject *kobj,
lib/kobject.c +5 -13
index 718e510..c7fb092 100644
@@ -383,6 +383,11 @@ EXPORT_SYMBOL_GPL(kobject_init_and_add);
  * kobject_rename - change the name of an object
  * @kobj: object in question.
  * @new_name: object's new name
+ *
+ * It is the responsibility of the caller to provide mutual
+ * exclusion between two different calls of kobject_rename
+ * on the same kobject and to ensure that new_name is valid and
+ * won't conflict with other kobjects.
  */
 int kobject_rename(struct kobject *kobj, const char *new_name)
 {
@@ -397,19 +402,6 @@ int kobject_rename(struct kobject *kobj, const char *new_name)
 	if (!kobj->parent)
 		return -EINVAL;
 
-	/* see if this name is already in use */
-	if (kobj->kset) {
-		struct kobject *temp_kobj;
-		temp_kobj = kset_find_obj(kobj->kset, new_name);
-		if (temp_kobj) {
-			printk(KERN_WARNING "kobject '%s' cannot be renamed "
-			       "to '%s' as '%s' is already in existence.\n",
-			       kobject_name(kobj), new_name, new_name);
-			kobject_put(temp_kobj);
-			return -EINVAL;
-		}
-	}
-
 	devpath = kobject_get_path(kobj, GFP_KERNEL);
 	if (!devpath) {
 		error = -ENOMEM;
Signature · 1 line
1.5.3.rc6.17.g1911
#23 lore raw

Re: kobject: Fix kobject_rename and !CONFIG_SYSFS

Quoted 27 lines · Eric W. Biederman

From: Eric W. Biederman <[email protected]>

When looking at kobject_rename I found two bugs with that exist when sysfs support is disabled in the kernel.

kobject_rename does not change the name on the kobject when sysfs support is not compiled in.

kobject_rename without locking attempts to check the validity of a rename operation, which the kobject layer simply does not have the infrastructure to do so.

This patch documents the previously unstated requirement of kobject_rename that is the responsibility of the caller to provide mutual exclusion and to be certain that the new_name for the kobject is valid.

This patch modifies sysfs_rename_dir in !CONFIG_SYSFS case to call kobject_set_name to actually change the kobject_name.

This patch removes the bogus and misleading check in kobject_rename that attempts to see if a rename is valid. The check is bogus because we do not have the proper locking. The check is misleading because it looks like we can and do perform useful checking at the kobject level that we don't.

Quoted 6 lines

The users in net/core/dev.c already have all of the necessary checks in place and don't care. The other use in net/core/wireless.c as originally implemented is incorrect because it performs no locking and a simple patch has been sent that adds the necessary locking and sanity checks there. Ensuring this patch will not have an effect on users of kobject_rename or device_rename.

Eric, Randy Dunlap has found that this patch breaks the build when CONFIG_SYSFS is not enabled. Can you please fix it up before I send it to Linus?

The exact error is:
In file included from /local/linsrc/next-20080509/include/linux/kobject.h:21,

                 from /local/linsrc/next-20080509/include/linux/module.h:16,
                 from /local/linsrc/next-20080509/include/linux/crypto.h:21,
                 from /local/linsrc/next-20080509/arch/x86/kernel/asm-offsets_64.c:7,
                 from /local/linsrc/next-20080509/arch/x86/kernel/asm-offsets.c:4:

/local/linsrc/next-20080509/include/linux/sysfs.h: In function 'sysfs_rename_dir':
/local/linsrc/next-20080509/include/linux/sysfs.h:142: error: implicit declaration of function 'kobject_set_name'

thanks,

greg k-h

#24 lore raw

Re: kobject: Fix kobject_rename and !CONFIG_SYSFS

Quoted 10 lines · Greg KH

Eric, Randy Dunlap has found that this patch breaks the build when CONFIG_SYSFS is not enabled. Can you please fix it up before I send it to Linus?

The exact error is:
In file included from /local/linsrc/next-20080509/include/linux/kobject.h:21,

                 from /local/linsrc/next-20080509/include/linux/module.h:16,
                 from /local/linsrc/next-20080509/include/linux/crypto.h:21,
                 from
Quoted 7 lines

/local/linsrc/next-20080509/arch/x86/kernel/asm-offsets_64.c:7,
from
/local/linsrc/next-20080509/arch/x86/kernel/asm-offsets.c:4:
/local/linsrc/next-20080509/include/linux/sysfs.h: In function 'sysfs_rename_dir':
/local/linsrc/next-20080509/include/linux/sysfs.h:142: error: implicit declaration of function 'kobject_set_name'

I will take a look in the morning and see if I can see what is wrong.

Which tree was this error against? I thought I tested this case, and I'm wondering if there might be another patch that is hiding kobject_set_name.

Eric

#25 lore raw

Re: kobject: Fix kobject_rename and !CONFIG_SYSFS

Quoted 19 lines · Eric W. Biederman
Quoted 9 lines · Greg KH

Eric, Randy Dunlap has found that this patch breaks the build when CONFIG_SYSFS is not enabled. Can you please fix it up before I send it to Linus?

The exact error is:
In file included from /local/linsrc/next-20080509/include/linux/kobject.h:21,

                 from /local/linsrc/next-20080509/include/linux/module.h:16,
                 from /local/linsrc/next-20080509/include/linux/crypto.h:21,
                 from
Quoted 7 lines

/local/linsrc/next-20080509/arch/x86/kernel/asm-offsets_64.c:7,
from
/local/linsrc/next-20080509/arch/x86/kernel/asm-offsets.c:4:
/local/linsrc/next-20080509/include/linux/sysfs.h: In function 'sysfs_rename_dir':
/local/linsrc/next-20080509/include/linux/sysfs.h:142: error: implicit declaration of function 'kobject_set_name'

Quoted 5 lines

I will take a look in the morning and see if I can see what is wrong.

Which tree was this error against? I thought I tested this case, and I'm wondering if there might be another patch that is hiding kobject_set_name.

Argh, headers "cross-dependencies":

* linux/kobject.h includes linux/sysfs.h before defining kobject_set_name()

* linux/sysfs.h needs to include linux/kobject.h to find kobject_set_name() definition (for inlined sysfs_rename_dir() when CONFIG_SYSFS=n)

sysfs_rename_dir() is only called by kobject.c, kobject_rename(). I guess this kind of patch is not acceptable to fix the depency?

lib/kobject.c 2008-05-13 15:58:37.000000000 +0200 renamed from lib/kobject.c 2008-05-13 15:14:38.000000000 +0200 +4 -1
Index: linux-2.6/lib/kobject.c
===================================================================
@@ -416,8 +416,11 @@ int kobject_rename(struct kobject *kobj,
        envp[0] = devpath_string;
        envp[1] = NULL;
 
+#ifdef CONFIG_SYSFS
        error = sysfs_rename_dir(kobj, new_name);
-
+#else
+       error = kobject_set_name(kobj, "%s", new_name);
+#endif
        /* This function is mostly/only used for network interface.
         * Some hotplug package track interfaces by their name and
         * therefore want to know when the name is changed by the user. */

-Benjamin

Quoted 4 lines

Eric

Signature · 3 lines
B e n j a m i n   T h e r y  - BULL/DT/Open Software R&D

   http://www.bull.com
#26 lore raw

Re: kobject: Fix kobject_rename and !CONFIG_SYSFS

Quoted 31 lines · Benjamin Thery
Quoted 20 lines · Eric W. Biederman
Quoted 4 lines · Greg KH

Eric, Randy Dunlap has found that this patch breaks the build when CONFIG_SYSFS is not enabled. Can you please fix it up before I send it to Linus?

Quoted 15 lines

The exact error is:
In file included from /local/linsrc/next-20080509/include/linux/kobject.h:21,
from
/local/linsrc/next-20080509/include/linux/module.h:16,
from
/local/linsrc/next-20080509/include/linux/crypto.h:21,
from
/local/linsrc/next-20080509/arch/x86/kernel/asm-offsets_64.c:7,
from
/local/linsrc/next-20080509/arch/x86/kernel/asm-offsets.c:4:
/local/linsrc/next-20080509/include/linux/sysfs.h: In function 'sysfs_rename_dir':
/local/linsrc/next-20080509/include/linux/sysfs.h:142: error: implicit declaration of function 'kobject_set_name'

Quoted 4 lines

I will take a look in the morning and see if I can see what is wrong. Which tree was this error against? I thought I tested this case, and I'm wondering if there might be another patch that is hiding kobject_set_name.

Argh, headers "cross-dependencies":

* linux/kobject.h includes linux/sysfs.h before defining kobject_set_name()

Quoted 7 lines

* linux/sysfs.h needs to include linux/kobject.h to find kobject_set_name() definition (for inlined sysfs_rename_dir() when CONFIG_SYSFS=n)

sysfs_rename_dir() is only called by kobject.c, kobject_rename(). I guess this kind of patch is not acceptable to fix the depency?

Ick, no. I'd rather add a kobject_set_name() function prototype to sysfs.h in this case, that should remove the error, right?

thanks,

greg k-h

#27 lore raw

PATCH 1/1 Fix kobject_rename and !CONFIG_SYSFS v2

When looking at kobject_rename I found two bugs with that exist when sysfs support is disabled in the kernel.

kobject_rename does not change the name on the kobject when sysfs support is not compiled in.

kobject_rename without locking attempts to check the validity of a rename operation, which the kobject layer simply does not have the infrastructure to do.

This patch documents the previously unstated requirement of kobject_rename that is the responsibility of the caller to provide mutual exclusion and to be certain that the new_name for the kobject is valid.

This patch modifies sysfs_rename_dir in !CONFIG_SYSFS case to call kobject_set_name to actually change the kobject_name.

This patch removes the bogus and misleading check in kobject_rename that attempts to see if a rename is valid. The check is bogus because we do not have the proper locking. The check is misleading because it looks like we can and do perform checking at the kobject level that we don't.

Changelog:
v2: Added a declaration of kboject_set_name to sysfs.h
so the code actually compiles with !CONFIG_SYSFS.

---
 Documentation/kobject.txt |    4 ++++
 drivers/base/core.c       |    5 +++++
 include/linux/sysfs.h     |    4 +++-
 lib/kobject.c             |   18 +++++-------------
 4 files changed, 17 insertions(+), 14 deletions(-)
Documentation/kobject.txt +4 -0
index bf3256e..79184b4 100644
@@ -118,6 +118,10 @@ the name of the kobject, call kobject_rename():
 
     int kobject_rename(struct kobject *kobj, const char *new_name);
 
+Note kobject_rename does perform any locking or have a solid notion of
+what names are valid so the provide must provide their own sanity checking
+and serialization.
+
 There is a function called kobject_set_name() but that is legacy cruft and
 is being removed.  If your code needs to call this function, it is
 incorrect and needs to be fixed.
drivers/base/core.c +5 -0
index be288b5..ad68f4c 100644
@@ -1171,6 +1171,11 @@ EXPORT_SYMBOL_GPL(device_destroy);
  * device_rename - renames a device
  * @dev: the pointer to the struct device to be renamed
  * @new_name: the new name of the device
+ *
+ * It is the responsibility of the caller to provide mutual
+ * exclusion between two different calls of device_rename
+ * on the same device to ensure that new_name is valid and
+ * won't conflict with other devices.
  */
 int device_rename(struct device *dev, char *new_name)
 {
include/linux/sysfs.h +3 -1
index 7858eac..6e61033 100644
@@ -20,6 +20,8 @@
 struct kobject;
 struct module;
 
+extern int kobject_set_name(struct kobject *kobj, const char *name, ...)
+			    __attribute__((format(printf, 2, 3)));
 /* FIXME
  * The *owner field is no longer used, but leave around
  * until the tree gets cleaned up fully.
@@ -137,7 +139,7 @@ static inline void sysfs_remove_dir(struct kobject *kobj)
 
 static inline int sysfs_rename_dir(struct kobject *kobj, const char *new_name)
 {
-	return 0;
+	return kobject_set_name(kobj, "%s", new_name);
 }
 
 static inline int sysfs_move_dir(struct kobject *kobj,
lib/kobject.c +5 -13
index 718e510..c7fb092 100644
@@ -383,6 +383,11 @@ EXPORT_SYMBOL_GPL(kobject_init_and_add);
  * kobject_rename - change the name of an object
  * @kobj: object in question.
  * @new_name: object's new name
+ *
+ * It is the responsibility of the caller to provide mutual
+ * exclusion between two different calls of kobject_rename
+ * on the same kobject and to ensure that new_name is valid and
+ * won't conflict with other kobjects.
  */
 int kobject_rename(struct kobject *kobj, const char *new_name)
 {
@@ -397,19 +402,6 @@ int kobject_rename(struct kobject *kobj, const char *new_name)
 	if (!kobj->parent)
 		return -EINVAL;
 
-	/* see if this name is already in use */
-	if (kobj->kset) {
-		struct kobject *temp_kobj;
-		temp_kobj = kset_find_obj(kobj->kset, new_name);
-		if (temp_kobj) {
-			printk(KERN_WARNING "kobject '%s' cannot be renamed "
-			       "to '%s' as '%s' is already in existence.\n",
-			       kobject_name(kobj), new_name, new_name);
-			kobject_put(temp_kobj);
-			return -EINVAL;
-		}
-	}
-
 	devpath = kobject_get_path(kobj, GFP_KERNEL);
 	if (!devpath) {
 		error = -ENOMEM;
Signature · 1 line
1.5.3.rc6.17.g1911
#28 lore raw

Re: [PATCH] Fix kobject_rename and !CONFIG_SYSFS v2

Quoted 17 lines · Eric W. Biederman

Changelog:
v2: Added a declaration of kboject_set_name to sysfs.h
so the code actually compiles with !CONFIG_SYSFS.

---
 Documentation/kobject.txt |    4 ++++
 drivers/base/core.c       |    5 +++++
 include/linux/sysfs.h     |    4 +++-
 lib/kobject.c             |   18 +++++-------------
 4 files changed, 17 insertions(+), 14 deletions(-)
Documentation/kobject.txt +0 -0
index bf3256e..79184b4 100644

Documentation/kobject.txt the name of the kobject, call kobject_rename(): · line 121

Quoted 4 lines
Documentation/kobject.txt +1 -0
@@ -118,6 +118,10 @@ the name of the kobject, call kobject_rename():
 
     int kobject_rename(struct kobject *kobj, const char *new_name);
 
+Note kobject_rename does perform any locking or have a solid notion of

^not

Documentation/kobject.txt the name of the kobject, call kobject_rename(): · line 122

Quoted 1 line
Documentation/kobject.txt +1 -0
+what names are valid so the provide must provide their own sanity checking
                               ~~~~~~~
                               caller
Quoted 5 lines
Documentation/kobject.txt +2 -0
+and serialization.
+
 There is a function called kobject_set_name() but that is legacy cruft and
 is being removed.  If your code needs to call this function, it is
 incorrect and needs to be fixed.
Signature · 1 line
~Randy
#29 lore raw

Re: [PATCH] Fix kobject_rename and !CONFIG_SYSFS v2

Quoted 27 lines · Randy.Dunlap
Quoted 17 lines · Eric W. Biederman

Changelog:
v2: Added a declaration of kboject_set_name to sysfs.h
so the code actually compiles with !CONFIG_SYSFS.

---
 Documentation/kobject.txt |    4 ++++
 drivers/base/core.c       |    5 +++++
 include/linux/sysfs.h     |    4 +++-
 lib/kobject.c             |   18 +++++-------------
 4 files changed, 17 insertions(+), 14 deletions(-)
Documentation/kobject.txt +0 -0
index bf3256e..79184b4 100644

Documentation/kobject.txt the name of the kobject, call kobject_rename(): · line 121

Quoted 4 lines
Documentation/kobject.txt +1 -0
@@ -118,6 +118,10 @@ the name of the kobject, call kobject_rename():
 
     int kobject_rename(struct kobject *kobj, const char *new_name);
 
+Note kobject_rename does perform any locking or have a solid notion of

^not

Documentation/kobject.txt the name of the kobject, call kobject_rename(): · line 122

Quoted 1 line
Documentation/kobject.txt +1 -0
+what names are valid so the provide must provide their own sanity checking
Quoted 8 lines
                               ~~~~~~~
                               caller

Documentation/kobject.txt the name of the kobject, call kobject_rename(): · line 127

Quoted 5 lines
Documentation/kobject.txt +2 -0
+and serialization.
+
 There is a function called kobject_set_name() but that is legacy cruft and
 is being removed.  If your code needs to call this function, it is
 incorrect and needs to be fixed.

Good catch.

Thanks Randy.

Eric

#30 lore raw

Re: [PATCH] Fix kobject_rename and !CONFIG_SYSFS v2

In reply to Eric W. Biederman
Quoted 3 lines · Eric W. Biederman

When looking at kobject_rename I found two bugs with that exist when sysfs support is disabled in the kernel.

<snip>

Better, but can you address Randy's objections?

thanks,

greg k-h

#31 lore raw

PATCH 1/1 Fix kobject_rename and !CONFIG_SYSFS v3

When looking at kobject_rename I found two bugs with that exist when sysfs support is disabled in the kernel.

kobject_rename does not change the name on the kobject when sysfs support is not compiled in.

kobject_rename without locking attempts to check the validity of a rename operation, which the kobject layer simply does not have the infrastructure to do.

This patch documents the previously unstated requirement of kobject_rename that is the responsibility of the caller to provide mutual exclusion and to be certain that the new_name for the kobject is valid.

This patch modifies sysfs_rename_dir in !CONFIG_SYSFS case to call kobject_set_name to actually change the kobject_name.

This patch removes the bogus and misleading check in kobject_rename that attempts to see if a rename is valid. The check is bogus because we do not have the proper locking. The check is misleading because it looks like we can and do perform checking at the kobject level that we don't.

Changelog:
v3: Documentation typo fixes

v2: Added a declaration of kboject_set_name to sysfs.h
so the code actually compiles with !CONFIG_SYSFS.

Unscrambling the header dependencies so everything looks beautiful is a project for another day.

---
 Documentation/kobject.txt |    4 ++++
 drivers/base/core.c       |    5 +++++
 include/linux/sysfs.h     |    4 +++-
 lib/kobject.c             |   18 +++++-------------
 4 files changed, 17 insertions(+), 14 deletions(-)
Documentation/kobject.txt +4 -0
index bf3256e..79184b4 100644
@@ -118,6 +118,10 @@ the name of the kobject, call kobject_rename():
 
     int kobject_rename(struct kobject *kobj, const char *new_name);
 
+Note kobject_rename does perform any locking or have a solid notion of
+what names are valid so the provide must provide their own sanity checking
+and serialization.
+
 There is a function called kobject_set_name() but that is legacy cruft and
 is being removed.  If your code needs to call this function, it is
 incorrect and needs to be fixed.
drivers/base/core.c +5 -0
index be288b5..ad68f4c 100644
@@ -1171,6 +1171,11 @@ EXPORT_SYMBOL_GPL(device_destroy);
  * device_rename - renames a device
  * @dev: the pointer to the struct device to be renamed
  * @new_name: the new name of the device
+ *
+ * It is the responsibility of the caller to provide mutual
+ * exclusion between two different calls of device_rename
+ * on the same device to ensure that new_name is valid and
+ * won't conflict with other devices.
  */
 int device_rename(struct device *dev, char *new_name)
 {
include/linux/sysfs.h +3 -1
index 7858eac..6e61033 100644
@@ -20,6 +20,8 @@
 struct kobject;
 struct module;
 
+extern int kobject_set_name(struct kobject *kobj, const char *name, ...)
+			    __attribute__((format(printf, 2, 3)));
 /* FIXME
  * The *owner field is no longer used, but leave around
  * until the tree gets cleaned up fully.
@@ -137,7 +139,7 @@ static inline void sysfs_remove_dir(struct kobject *kobj)
 
 static inline int sysfs_rename_dir(struct kobject *kobj, const char *new_name)
 {
-	return 0;
+	return kobject_set_name(kobj, "%s", new_name);
 }
 
 static inline int sysfs_move_dir(struct kobject *kobj,
lib/kobject.c +5 -13
index 718e510..c7fb092 100644
@@ -383,6 +383,11 @@ EXPORT_SYMBOL_GPL(kobject_init_and_add);
  * kobject_rename - change the name of an object
  * @kobj: object in question.
  * @new_name: object's new name
+ *
+ * It is the responsibility of the caller to provide mutual
+ * exclusion between two different calls of kobject_rename
+ * on the same kobject and to ensure that new_name is valid and
+ * won't conflict with other kobjects.
  */
 int kobject_rename(struct kobject *kobj, const char *new_name)
 {
@@ -397,19 +402,6 @@ int kobject_rename(struct kobject *kobj, const char *new_name)
 	if (!kobj->parent)
 		return -EINVAL;
 
-	/* see if this name is already in use */
-	if (kobj->kset) {
-		struct kobject *temp_kobj;
-		temp_kobj = kset_find_obj(kobj->kset, new_name);
-		if (temp_kobj) {
-			printk(KERN_WARNING "kobject '%s' cannot be renamed "
-			       "to '%s' as '%s' is already in existence.\n",
-			       kobject_name(kobj), new_name, new_name);
-			kobject_put(temp_kobj);
-			return -EINVAL;
-		}
-	}
-
 	devpath = kobject_get_path(kobj, GFP_KERNEL);
 	if (!devpath) {
 		error = -ENOMEM;
Signature · 1 line
1.5.3.rc6.17.g1911
#32 lore raw

Re: [PATCH] Fix kobject_rename and !CONFIG_SYSFS v3

Quoted 2 lines

Changelog:
v3: Documentation typo fixes

Inserted/attached wrong patch file?
I don't see any text changes.

Quoted 20 lines
Documentation/kobject.txt +1 -0
index bf3256e..79184b4 100644
@@ -118,6 +118,10 @@ the name of the kobject, call kobject_rename():
 
     int kobject_rename(struct kobject *kobj, const char
 *new_name);
 
+Note kobject_rename does perform any locking or have a solid

notion of
+what names are valid so the provide must provide their own sanity checking

Documentation/kobject.txt +2 -0
+and serialization.
+
 There is a function called kobject_set_name() but that is

legacy cruft and
is being removed. If your code needs to call this function,
it is
incorrect and needs to be fixed.

#33 lore raw

PATCH 1/1 Fix kobject_rename and !CONFIG_SYSFS v4

When looking at kobject_rename I found two bugs with that exist when sysfs support is disabled in the kernel.

kobject_rename does not change the name on the kobject when sysfs support is not compiled in.

kobject_rename without locking attempts to check the validity of a rename operation, which the kobject layer simply does not have the infrastructure to do.

This patch documents the previously unstated requirement of kobject_rename that is the responsibility of the caller to provide mutual exclusion and to be certain that the new_name for the kobject is valid.

This patch modifies sysfs_rename_dir in !CONFIG_SYSFS case to call kobject_set_name to actually change the kobject_name.

This patch removes the bogus and misleading check in kobject_rename that attempts to see if a rename is valid. The check is bogus because we do not have the proper locking. The check is misleading because it looks like we can and do perform checking at the kobject level that we don't.

Changelog:
v4: Documentation typo fixes

v2: Added a declaration of kboject_set_name to sysfs.h
so the code actually compiles with !CONFIG_SYSFS.

Unscrambling the header dependencies so everything looks beautiful is a project for another day.

---
 Documentation/kobject.txt |    4 ++++
 drivers/base/core.c       |    5 +++++
 include/linux/sysfs.h     |    4 +++-
 lib/kobject.c             |   18 +++++-------------
 4 files changed, 17 insertions(+), 14 deletions(-)
Documentation/kobject.txt +4 -0
index bf3256e..ac80d82 100644
@@ -118,6 +118,10 @@ the name of the kobject, call kobject_rename():
 
     int kobject_rename(struct kobject *kobj, const char *new_name);
 
+Note kobject_rename does not perform any locking or have a solid notion of
+what names are valid so the caller must provide their own sanity checking
+and serialization.
+
 There is a function called kobject_set_name() but that is legacy cruft and
 is being removed.  If your code needs to call this function, it is
 incorrect and needs to be fixed.
drivers/base/core.c +5 -0
index be288b5..ad68f4c 100644
@@ -1171,6 +1171,11 @@ EXPORT_SYMBOL_GPL(device_destroy);
  * device_rename - renames a device
  * @dev: the pointer to the struct device to be renamed
  * @new_name: the new name of the device
+ *
+ * It is the responsibility of the caller to provide mutual
+ * exclusion between two different calls of device_rename
+ * on the same device to ensure that new_name is valid and
+ * won't conflict with other devices.
  */
 int device_rename(struct device *dev, char *new_name)
 {
include/linux/sysfs.h +3 -1
index 7858eac..6e61033 100644
@@ -20,6 +20,8 @@
 struct kobject;
 struct module;
 
+extern int kobject_set_name(struct kobject *kobj, const char *name, ...)
+			    __attribute__((format(printf, 2, 3)));
 /* FIXME
  * The *owner field is no longer used, but leave around
  * until the tree gets cleaned up fully.
@@ -137,7 +139,7 @@ static inline void sysfs_remove_dir(struct kobject *kobj)
 
 static inline int sysfs_rename_dir(struct kobject *kobj, const char *new_name)
 {
-	return 0;
+	return kobject_set_name(kobj, "%s", new_name);
 }
 
 static inline int sysfs_move_dir(struct kobject *kobj,
lib/kobject.c +5 -13
index 718e510..c7fb092 100644
@@ -383,6 +383,11 @@ EXPORT_SYMBOL_GPL(kobject_init_and_add);
  * kobject_rename - change the name of an object
  * @kobj: object in question.
  * @new_name: object's new name
+ *
+ * It is the responsibility of the caller to provide mutual
+ * exclusion between two different calls of kobject_rename
+ * on the same kobject and to ensure that new_name is valid and
+ * won't conflict with other kobjects.
  */
 int kobject_rename(struct kobject *kobj, const char *new_name)
 {
@@ -397,19 +402,6 @@ int kobject_rename(struct kobject *kobj, const char *new_name)
 	if (!kobj->parent)
 		return -EINVAL;
 
-	/* see if this name is already in use */
-	if (kobj->kset) {
-		struct kobject *temp_kobj;
-		temp_kobj = kset_find_obj(kobj->kset, new_name);
-		if (temp_kobj) {
-			printk(KERN_WARNING "kobject '%s' cannot be renamed "
-			       "to '%s' as '%s' is already in existence.\n",
-			       kobject_name(kobj), new_name, new_name);
-			kobject_put(temp_kobj);
-			return -EINVAL;
-		}
-	}
-
 	devpath = kobject_get_path(kobj, GFP_KERNEL);
 	if (!devpath) {
 		error = -ENOMEM;
Signature · 1 line
1.5.3.rc6.17.g1911
maintainer
#34 lore raw

Re: [PATCH] Fix kobject_rename and !CONFIG_SYSFS v4

Quoted 78 lines · On Tue, 13 May 2008 21:39:45 -0700 [email protected] (Eric W. Biederman)…

When looking at kobject_rename I found two bugs with that exist when sysfs support is disabled in the kernel.

kobject_rename does not change the name on the kobject when sysfs support is not compiled in.

kobject_rename without locking attempts to check the validity of a rename operation, which the kobject layer simply does not have the infrastructure to do.

This patch documents the previously unstated requirement of kobject_rename that is the responsibility of the caller to provide mutual exclusion and to be certain that the new_name for the kobject is valid.

This patch modifies sysfs_rename_dir in !CONFIG_SYSFS case to call kobject_set_name to actually change the kobject_name.

This patch removes the bogus and misleading check in kobject_rename that attempts to see if a rename is valid. The check is bogus because we do not have the proper locking. The check is misleading because it looks like we can and do perform checking at the kobject level that we don't.

Changelog:
v4: Documentation typo fixes

v2: Added a declaration of kboject_set_name to sysfs.h
so the code actually compiles with !CONFIG_SYSFS.

Unscrambling the header dependencies so everything looks beautiful is a project for another day.

---
 Documentation/kobject.txt |    4 ++++
 drivers/base/core.c       |    5 +++++
 include/linux/sysfs.h     |    4 +++-
 lib/kobject.c             |   18 +++++-------------
 4 files changed, 17 insertions(+), 14 deletions(-)
Documentation/kobject.txt +4 -0
index bf3256e..ac80d82 100644
@@ -118,6 +118,10 @@ the name of the kobject, call kobject_rename():
 
     int kobject_rename(struct kobject *kobj, const char *new_name);
 
+Note kobject_rename does not perform any locking or have a solid notion of
+what names are valid so the caller must provide their own sanity checking
+and serialization.
+
 There is a function called kobject_set_name() but that is legacy cruft and
 is being removed.  If your code needs to call this function, it is
 incorrect and needs to be fixed.
drivers/base/core.c +5 -0
index be288b5..ad68f4c 100644
@@ -1171,6 +1171,11 @@ EXPORT_SYMBOL_GPL(device_destroy);
  * device_rename - renames a device
  * @dev: the pointer to the struct device to be renamed
  * @new_name: the new name of the device
+ *
+ * It is the responsibility of the caller to provide mutual
+ * exclusion between two different calls of device_rename
+ * on the same device to ensure that new_name is valid and
+ * won't conflict with other devices.
  */
 int device_rename(struct device *dev, char *new_name)
 {
include/linux/sysfs.h +0 -0
index 7858eac..6e61033 100644

include/linux/sysfs.h line 24

Quoted 5 lines
include/linux/sysfs.h +2 -0
@@ -20,6 +20,8 @@
 struct kobject;
 struct module;
 
+extern int kobject_set_name(struct kobject *kobj, const char *name, ...)
+			    __attribute__((format(printf, 2, 3)));

Duplicating the kobject_set_name() declaration in sysfs.h is rather a hack.

It'd be better to move it into a new header file, included by both sysfs.h and kobject.h. Perhaps there are other declarations which can be moved with it.

#35 lore raw

Re: [PATCH] Fix kobject_rename and !CONFIG_SYSFS v4

Quoted 8 lines · Andrew Morton
Quoted 8 lines
include/linux/sysfs.h +0 -0
index 7858eac..6e61033 100644
@@ -20,6 +20,8 @@
 struct kobject;
 struct module;
 
Quoted 4 lines

include/linux/sysfs.h line 24

Quoted 2 lines
include/linux/sysfs.h +2 -0
+extern int kobject_set_name(struct kobject *kobj, const char *name, ...)
+			    __attribute__((format(printf, 2, 3)));

Duplicating the kobject_set_name() declaration in sysfs.h is rather a hack.

A bit. It really isn't worse then a struct declaration like struct kobject;

Quoted 3 lines

It'd be better to move it into a new header file, included by both sysfs.h and kobject.h. Perhaps there are other declarations which can be moved with it.

My gut feel says that sysfs.h should include kobject.h instead of the other way around.

However it gets reorganized, it is an entirely separate problem from the one this patch sets out to solve and so should go in a different patch.

Eric

maintainer
#36 lore raw

Re: [PATCH] Fix kobject_rename and !CONFIG_SYSFS v4

Quoted 20 lines · On Wed, 14 May 2008 02:01:37 -0700 [email protected] (Eric W. Biederman)…
Quoted 8 lines · Andrew Morton
Quoted 8 lines
include/linux/sysfs.h +0 -0
index 7858eac..6e61033 100644
@@ -20,6 +20,8 @@
 struct kobject;
 struct module;
 
Quoted 4 lines

include/linux/sysfs.h line 24

Quoted 2 lines
include/linux/sysfs.h +2 -0
+extern int kobject_set_name(struct kobject *kobj, const char *name, ...)
+			    __attribute__((format(printf, 2, 3)));

Duplicating the kobject_set_name() declaration in sysfs.h is rather a hack.

A bit. It really isn't worse then a struct declaration like struct kobject;

Quoted 3 lines

It'd be better to move it into a new header file, included by both sysfs.h and kobject.h. Perhaps there are other declarations which can be moved with it.

Quoted 7 lines

My gut feel says that sysfs.h should include kobject.h instead of the other way around.

However it gets reorganized, it is an entirely separate problem from the one this patch sets out to solve and so should go in a different patch.

umm, well, actually, it's a problem which your patch introduces, by adding a new dependency.

uninlining sysfs_rename_dir() would be a sensible solution.

#37 lore raw

Re: [PATCH] Fix kobject_rename and !CONFIG_SYSFS v4

Quoted 15 lines · Andrew Morton
Quoted 13 lines · On Wed, 14 May 2008 02:01:37 -0700 [email protected] (Eric W. Biederman)…
Quoted 11 lines · Andrew Morton

include/linux/sysfs.h line 24

Quoted 10 lines
include/linux/sysfs.h +2 -0
index 7858eac..6e61033 100644
@@ -20,6 +20,8 @@
 struct kobject;
 struct module;
 
+extern int kobject_set_name(struct kobject *kobj, const char *name, ...)
+			    __attribute__((format(printf, 2, 3)));

Duplicating the kobject_set_name() declaration in sysfs.h is rather a hack.

Quoted 7 lines
Quoted 7 lines

A bit. It really isn't worse then a struct declaration like struct kobject;

Quoted 3 lines

It'd be better to move it into a new header file, included by both sysfs.h and kobject.h. Perhaps there are other declarations which can be moved with it.

My gut feel says that sysfs.h should include kobject.h instead of the other way around.

This is also my feeling.
To do this, we need to move "struct attributes" related definitions to kboject.h, and fix some .c files that don't include sysfs.h as they should (sysfs.h was included indirectly via kojbect.h in these files: eg kernel/sched.c, fs/partitions/check.c).

Quoted 10 lines
Quoted 5 lines

However it gets reorganized, it is an entirely separate problem from the one this patch sets out to solve and so should go in a different patch.

umm, well, actually, it's a problem which your patch introduces, by adding a new dependency.

uninlining sysfs_rename_dir() would be a sensible solution.

It is inlined only when CONFIG_SYSFS=n. When sysfs is enabled sysfs_rename_dir() is compiled from fs/sysfs/dir.c

Uninlining it will require us to find an appropriate .c file to put it
in: we can't put it in fs/sysfs/dir.c. It is not built if CONFIG_SYSFS is disabled.

-Benjamin

Signature · 3 lines
B e n j a m i n   T h e r y  - BULL/DT/Open Software R&D

   http://www.bull.com
maintainer
#38 lore raw

Re: [PATCH] Fix kobject_rename and !CONFIG_SYSFS v4

Quoted 8 lines · On Wed, 14 May 2008 11:51:17 +0200 Benjamin Thery <[email protected]>…
Quoted 1 line

uninlining sysfs_rename_dir() would be a sensible solution.

It is inlined only when CONFIG_SYSFS=n. When sysfs is enabled sysfs_rename_dir() is compiled from fs/sysfs/dir.c

Uninlining it will require us to find an appropriate .c file to put it
in: we can't put it in fs/sysfs/dir.c. It is not built if CONFIG_SYSFS is disabled.

Well yes, that's the problem. I stuck it in lib/kobject.c but ugh.

I guess we could make it a macro and quickly run away.

#39 lore raw

Re: kobject: Fix kobject_rename and !CONFIG_SYSFS

In reply to Greg KH

Quoting Greg KH <[email protected]>:

Quoted 37 lines · Benjamin Thery
Quoted 36 lines
Quoted 20 lines · Eric W. Biederman
Quoted 4 lines · Greg KH

Eric, Randy Dunlap has found that this patch breaks the build when CONFIG_SYSFS is not enabled. Can you please fix it up before I send it to Linus?

Quoted 15 lines

The exact error is:
In file included from /local/linsrc/next-20080509/include/linux/kobject.h:21,
from
/local/linsrc/next-20080509/include/linux/module.h:16,
from
/local/linsrc/next-20080509/include/linux/crypto.h:21,
from
/local/linsrc/next-20080509/arch/x86/kernel/asm-offsets_64.c:7,
from
/local/linsrc/next-20080509/arch/x86/kernel/asm-offsets.c:4:
/local/linsrc/next-20080509/include/linux/sysfs.h: In function 'sysfs_rename_dir':
/local/linsrc/next-20080509/include/linux/sysfs.h:142: error: implicit declaration of function 'kobject_set_name'

Quoted 4 lines

I will take a look in the morning and see if I can see what is wrong. Which tree was this error against? I thought I tested this case, and I'm wondering if there might be another patch that is hiding kobject_set_name.

Argh, headers "cross-dependencies":

* linux/kobject.h includes linux/sysfs.h before defining kobject_set_name()

* linux/sysfs.h needs to include linux/kobject.h to find kobject_set_name() definition (for inlined sysfs_rename_dir() when CONFIG_SYSFS=n)

Quoted 5 lines
Quoted 2 lines

sysfs_rename_dir() is only called by kobject.c, kobject_rename(). I guess this kind of patch is not acceptable to fix the depency?

Ick, no. I'd rather add a kobject_set_name() function prototype to sysfs.h in this case, that should remove the error, right?

That's what I did first. But I thought it was worse than the above solution.
Anyway, adding kobject_set_name() to sysfs.h did fix the error.

-Benjamin

Quoted 6 lines

thanks,

greg k-h

----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.

#40 lore raw

Re: kobject: Fix kobject_rename and !CONFIG_SYSFS

Quoted 3 lines · Benjamin Thery

That's what I did first. But I thought it was worse than the above solution.
Anyway, adding kobject_set_name() to sysfs.h did fix the error.

I figure sorting out the headers is worth while, but that is another project.

Because kobject.h includes sysfs.h we can't get the declarations out of sync which is the really important part.

Eric

#41 lore raw

PATCH 11/11 sysfs: user namespaces: add ns to user_struct

In reply to Benjamin Thery

Add the user_namespace to user_struct.

Add a user_namespace to the sysfs_tag struct.

Mark the /sys/kernel/uids directory to be tagged so that processes in different user namespaces can remount /sys and see their own uid listings.

Without this patch, having CONFIG_FAIR_SCHED=y makes user namespaces unusable, because when you
clone(CLONE_NEWUSER)
it will auto-create the root userid and try to create /sys/kernel/uids/0. Since that already exists from the parent user namespace, the create fails, and the clone misleadingly ends up returning -ENOMEM.

This patch fixes the issue by allowing each user namespace to remount /sys, and having /sys filter the /sys/kernel/uid/ entries by user namespace.

---
 fs/dquot.c                     |    2 -
 fs/ioprio.c                    |    2 -
 fs/sysfs/mount.c               |   25 +++++++++++++++++++++
 include/linux/sched.h          |    3 +-
 include/linux/sysfs.h          |    9 +++++++
 include/linux/types.h          |    5 ++++
 include/linux/user_namespace.h |    5 ++++
 kernel/user.c                  |   48 ++++++++++++++++++++++++++++++++++-------
 kernel/user_namespace.c        |    3 +-
 security/keys/process_keys.c   |   14 +++++------
 10 files changed, 97 insertions(+), 19 deletions(-)
fs/dquot.c +1 -1
Index: linux-vanilla/fs/dquot.c
===================================================================
@@ -968,7 +968,7 @@ static void send_warning(const struct dq
 		MINOR(dquot->dq_sb->s_dev));
 	if (ret)
 		goto attr_err_out;
-	ret = nla_put_u64(skb, QUOTA_NL_A_CAUSED_ID, current->user->uid);
+	ret = nla_put_u64(skb, QUOTA_NL_A_CAUSED_ID, current->user->uid.uid);
 	if (ret)
 		goto attr_err_out;
 	genlmsg_end(skb, msg_head);
fs/ioprio.c +1 -1
Index: linux-vanilla/fs/ioprio.c
===================================================================
@@ -224,7 +224,7 @@ asmlinkage long sys_ioprio_get(int which
 				break;
 
 			do_each_thread(g, p) {
-				if (p->uid != user->uid)
+				if (!task_user_equiv(p, user))
 					continue;
 				tmpio = get_task_ioprio(p);
 				if (tmpio < 0)
fs/sysfs/mount.c +25 -0
Index: linux-vanilla/fs/sysfs/mount.c
===================================================================
@@ -17,6 +17,7 @@
 #include <linux/pagemap.h>
 #include <linux/init.h>
 #include <linux/nsproxy.h>
+#include <linux/user_namespace.h>
 #include <net/net_namespace.h>
 
 #include "sysfs.h"
@@ -81,6 +82,7 @@ static int sysfs_fill_super(struct super
 	sb->s_root = root;
 	sb->s_fs_info = info;
 	info->tag.net_ns = hold_net(current->nsproxy->net_ns);
+	info->tag.user_ns = current->nsproxy->user_ns;
 	return 0;
 
 out_err:
@@ -100,6 +102,8 @@ static int sysfs_test_super(struct super
 
 	if (task->nsproxy->net_ns != info->tag.net_ns)
 		found = 0;
+	if (task->nsproxy->user_ns != info->tag.user_ns)
+		found = 0;
 
 	return found;
 }
@@ -214,6 +218,27 @@ static struct pernet_operations sysfs_ne
 };
 #endif
 
+#ifdef CONFIG_USER_NS
+void sysfs_userns_exit(struct user_namespace *user_ns)
+{
+	/* Allow the net namespace to go away while sysfs is still mounted. */
+	struct super_block *sb;
+	printk(KERN_NOTICE "sysfs: user namespace exiting\n");
+	mutex_lock(&sysfs_rename_mutex);
+	sysfs_grab_supers();
+	mutex_lock(&sysfs_mutex);
+	list_for_each_entry(sb, &sysfs_fs_type.fs_supers, s_instances) {
+		struct sysfs_super_info *info = sysfs_info(sb);
+		if (info->tag.user_ns != user_ns)
+			continue;
+		info->tag.user_ns = NULL;
+	}
+	mutex_unlock(&sysfs_mutex);
+	sysfs_release_supers();
+	mutex_unlock(&sysfs_rename_mutex);
+}
+#endif
+
 int __init sysfs_init(void)
 {
 	int err = -ENOMEM;
include/linux/sched.h +2 -1
Index: linux-vanilla/include/linux/sched.h
===================================================================
@@ -595,7 +595,7 @@ struct user_struct {
 
 	/* Hash table maintenance information */
 	struct hlist_node uidhash_node;
-	uid_t uid;
+	struct k_uid_t uid;
 
 #ifdef CONFIG_USER_SCHED
 	struct task_group *tg;
@@ -1697,6 +1697,7 @@ static inline struct user_struct *get_ui
 extern void free_uid(struct user_struct *);
 extern void switch_uid(struct user_struct *);
 extern void release_uids(struct user_namespace *ns);
+extern int task_user_equiv(struct task_struct *tsk, struct user_struct *u);
 
 #include <asm/current.h>
 
include/linux/sysfs.h +9 -0
Index: linux-vanilla/include/linux/sysfs.h
===================================================================
@@ -20,6 +20,7 @@
 struct kobject;
 struct module;
 struct net;
+struct user_namespace;
 
 /* FIXME
  * The *owner field is no longer used, but leave around
@@ -81,6 +82,7 @@ struct sysfs_ops {
 
 struct sysfs_tag_info {
 	struct net *net_ns;
+	struct user_namespace *user_ns;
 };
 
 struct sysfs_tagged_dir_operations {
@@ -139,6 +141,9 @@ int sysfs_enable_tagging(struct kobject 
 
 extern int __must_check sysfs_init(void);
 
+struct user_namespace;
+void sysfs_userns_exit(struct user_namespace *user_ns);
+
 #else /* CONFIG_SYSFS */
 
 static inline int sysfs_schedule_callback(struct kobject *kobj,
@@ -264,6 +269,10 @@ static inline int __must_check sysfs_ini
 	return 0;
 }
 
+static inline void sysfs_userns_exit(struct user_namespace *user_ns)
+{
+}
+
 #endif /* CONFIG_SYSFS */
 
 #endif /* _SYSFS_H_ */
include/linux/types.h +5 -0
Index: linux-vanilla/include/linux/types.h
===================================================================
@@ -37,6 +37,11 @@ typedef __kernel_gid32_t	gid_t;
 typedef __kernel_uid16_t        uid16_t;
 typedef __kernel_gid16_t        gid16_t;
 
+struct k_uid_t {
+	uid_t uid;
+	struct user_namespace *ns;
+};
+
 typedef unsigned long		uintptr_t;
 
 #ifdef CONFIG_UID16
include/linux/user_namespace.h +5 -0
Index: linux-vanilla/include/linux/user_namespace.h
===================================================================
@@ -12,10 +12,15 @@
 struct user_namespace {
 	struct kref		kref;
 	struct hlist_head	uidhash_table[UIDHASH_SZ];
+	struct kset		*kset;
 	struct user_struct	*root_user;
 };
 
 extern struct user_namespace init_user_ns;
+extern int register_user_ns_kobj(struct user_namespace *ns);
+extern void unregister_user_ns_kobj(struct user_namespace *ns);
+extern int register_user_ns_kobj(struct user_namespace *ns);
+extern void unregister_user_ns_kobj(struct user_namespace *ns);
 
 #ifdef CONFIG_USER_NS
 
kernel/user.c +40 -8

98 lines not on this page · raw message

kernel/user_namespace.c +2 -1
Index: linux-vanilla/kernel/user_namespace.c
===================================================================
@@ -22,7 +22,7 @@ static struct user_namespace *clone_user
 	struct user_struct *new_user;
 	int n;
 
-	ns = kmalloc(sizeof(struct user_namespace), GFP_KERNEL);
+	ns = kzalloc(sizeof(struct user_namespace), GFP_KERNEL);
 	if (!ns)
 		return ERR_PTR(-ENOMEM);
 
@@ -71,6 +71,7 @@ void free_user_ns(struct kref *kref)
 	struct user_namespace *ns;
 
 	ns = container_of(kref, struct user_namespace, kref);
+	sysfs_userns_exit(ns);
 	release_uids(ns);
 	kfree(ns);
 }
security/keys/process_keys.c +7 -7
Index: linux-vanilla/security/keys/process_keys.c
===================================================================
@@ -47,7 +47,7 @@ static int install_user_keyrings(struct 
 	char buf[20];
 	int ret;
 
-	kenter("%p{%u}", user, user->uid);
+	kenter("%p{%u}", user, user->uid.uid);
 
 	if (user->uid_keyring) {
 		kleave(" = 0 [exist]");
@@ -62,13 +62,13 @@ static int install_user_keyrings(struct 
 		 * - there may be one in existence already as it may have been
 		 *   pinned by a session, but the user_struct pointing to it
 		 *   may have been destroyed by setuid */
-		sprintf(buf, "_uid.%u", user->uid);
+		sprintf(buf, "_uid.%u", user->uid.uid);
 
 		uid_keyring = find_keyring_by_name(buf, true);
 		if (IS_ERR(uid_keyring)) {
-			uid_keyring = keyring_alloc(buf, user->uid, (gid_t) -1,
-						    tsk, KEY_ALLOC_IN_QUOTA,
-						    NULL);
+			uid_keyring = keyring_alloc(buf, user->uid.uid,
+						    (gid_t) -1, tsk,
+						    KEY_ALLOC_IN_QUOTA, NULL);
 			if (IS_ERR(uid_keyring)) {
 				ret = PTR_ERR(uid_keyring);
 				goto error;
@@ -77,12 +77,12 @@ static int install_user_keyrings(struct 
 
 		/* get a default session keyring (which might also exist
 		 * already) */
-		sprintf(buf, "_uid_ses.%u", user->uid);
+		sprintf(buf, "_uid_ses.%u", user->uid.uid);
 
 		session_keyring = find_keyring_by_name(buf, true);
 		if (IS_ERR(session_keyring)) {
 			session_keyring =
-				keyring_alloc(buf, user->uid, (gid_t) -1,
+				keyring_alloc(buf, user->uid.uid, (gid_t) -1,
 					      tsk, KEY_ALLOC_IN_QUOTA, NULL);
 			if (IS_ERR(session_keyring)) {
 				ret = PTR_ERR(session_keyring);
#42 lore raw

Re: [PATCH 11/11] sysfs: user namespaces: add ns to user_struct

Quoted 16 lines · Benjamin Thery

Add the user_namespace to user_struct.

Add a user_namespace to the sysfs_tag struct.

Mark the /sys/kernel/uids directory to be tagged so that processes in different user namespaces can remount /sys and see their own uid listings.

Without this patch, having CONFIG_FAIR_SCHED=y makes user namespaces unusable, because when you
clone(CLONE_NEWUSER)
it will auto-create the root userid and try to create /sys/kernel/uids/0. Since that already exists from the parent user namespace, the create fails, and the clone misleadingly ends up returning -ENOMEM.

Quoted 5 lines

This patch fixes the issue by allowing each user namespace to remount /sys, and having /sys filter the /sys/kernel/uid/ entries by user namespace.

Benjamin,

thanks for reposting. Please do drop this patch from the set, though. A slimmer version of it really is needed asap, but this version as Eric had pointed out has some holdouts from my previous approach, which I need to drop.

thanks,
-serge

Quoted 371 lines
---
 fs/dquot.c                     |    2 -
 fs/ioprio.c                    |    2 -
 fs/sysfs/mount.c               |   25 +++++++++++++++++++++
 include/linux/sched.h          |    3 +-
 include/linux/sysfs.h          |    9 +++++++
 include/linux/types.h          |    5 ++++
 include/linux/user_namespace.h |    5 ++++
 kernel/user.c                  |   48 ++++++++++++++++++++++++++++++++++-------
 kernel/user_namespace.c        |    3 +-
 security/keys/process_keys.c   |   14 +++++------
 10 files changed, 97 insertions(+), 19 deletions(-)

Index: linux-vanilla/fs/dquot.c
===================================================================

fs/dquot.c +1 -1
@@ -968,7 +968,7 @@ static void send_warning(const struct dq
 		MINOR(dquot->dq_sb->s_dev));
 	if (ret)
 		goto attr_err_out;
-	ret = nla_put_u64(skb, QUOTA_NL_A_CAUSED_ID, current->user->uid);
+	ret = nla_put_u64(skb, QUOTA_NL_A_CAUSED_ID, current->user->uid.uid);
 	if (ret)
 		goto attr_err_out;
 	genlmsg_end(skb, msg_head);

Index: linux-vanilla/fs/ioprio.c
===================================================================

fs/ioprio.c +1 -1
@@ -224,7 +224,7 @@ asmlinkage long sys_ioprio_get(int which
 				break;
 
 			do_each_thread(g, p) {
-				if (p->uid != user->uid)
+				if (!task_user_equiv(p, user))
 					continue;
 				tmpio = get_task_ioprio(p);
 				if (tmpio < 0)

Index: linux-vanilla/fs/sysfs/mount.c
===================================================================

fs/sysfs/mount.c +25 -0
@@ -17,6 +17,7 @@
 #include <linux/pagemap.h>
 #include <linux/init.h>
 #include <linux/nsproxy.h>
+#include <linux/user_namespace.h>
 #include <net/net_namespace.h>
 
 #include "sysfs.h"
@@ -81,6 +82,7 @@ static int sysfs_fill_super(struct super
 	sb->s_root = root;
 	sb->s_fs_info = info;
 	info->tag.net_ns = hold_net(current->nsproxy->net_ns);
+	info->tag.user_ns = current->nsproxy->user_ns;
 	return 0;
 
 out_err:
@@ -100,6 +102,8 @@ static int sysfs_test_super(struct super
 
 	if (task->nsproxy->net_ns != info->tag.net_ns)
 		found = 0;
+	if (task->nsproxy->user_ns != info->tag.user_ns)
+		found = 0;
 
 	return found;
 }
@@ -214,6 +218,27 @@ static struct pernet_operations sysfs_ne
 };
 #endif
 
+#ifdef CONFIG_USER_NS
+void sysfs_userns_exit(struct user_namespace *user_ns)
+{
+	/* Allow the net namespace to go away while sysfs is still mounted. */
+	struct super_block *sb;
+	printk(KERN_NOTICE "sysfs: user namespace exiting\n");
+	mutex_lock(&sysfs_rename_mutex);
+	sysfs_grab_supers();
+	mutex_lock(&sysfs_mutex);
+	list_for_each_entry(sb, &sysfs_fs_type.fs_supers, s_instances) {
+		struct sysfs_super_info *info = sysfs_info(sb);
+		if (info->tag.user_ns != user_ns)
+			continue;
+		info->tag.user_ns = NULL;
+	}
+	mutex_unlock(&sysfs_mutex);
+	sysfs_release_supers();
+	mutex_unlock(&sysfs_rename_mutex);
+}
+#endif
+
 int __init sysfs_init(void)
 {
 	int err = -ENOMEM;

Index: linux-vanilla/include/linux/sched.h
===================================================================

include/linux/sched.h +2 -1
@@ -595,7 +595,7 @@ struct user_struct {
 
 	/* Hash table maintenance information */
 	struct hlist_node uidhash_node;
-	uid_t uid;
+	struct k_uid_t uid;
 
 #ifdef CONFIG_USER_SCHED
 	struct task_group *tg;
@@ -1697,6 +1697,7 @@ static inline struct user_struct *get_ui
 extern void free_uid(struct user_struct *);
 extern void switch_uid(struct user_struct *);
 extern void release_uids(struct user_namespace *ns);
+extern int task_user_equiv(struct task_struct *tsk, struct user_struct *u);
 
 #include <asm/current.h>
 

Index: linux-vanilla/include/linux/sysfs.h
===================================================================

include/linux/sysfs.h +9 -0
@@ -20,6 +20,7 @@
 struct kobject;
 struct module;
 struct net;
+struct user_namespace;
 
 /* FIXME
  * The *owner field is no longer used, but leave around
@@ -81,6 +82,7 @@ struct sysfs_ops {
 
 struct sysfs_tag_info {
 	struct net *net_ns;
+	struct user_namespace *user_ns;
 };
 
 struct sysfs_tagged_dir_operations {
@@ -139,6 +141,9 @@ int sysfs_enable_tagging(struct kobject 
 
 extern int __must_check sysfs_init(void);
 
+struct user_namespace;
+void sysfs_userns_exit(struct user_namespace *user_ns);
+
 #else /* CONFIG_SYSFS */
 
 static inline int sysfs_schedule_callback(struct kobject *kobj,
@@ -264,6 +269,10 @@ static inline int __must_check sysfs_ini
 	return 0;
 }
 
+static inline void sysfs_userns_exit(struct user_namespace *user_ns)
+{
+}
+
 #endif /* CONFIG_SYSFS */
 
 #endif /* _SYSFS_H_ */

Index: linux-vanilla/include/linux/types.h
===================================================================

include/linux/types.h +5 -0
@@ -37,6 +37,11 @@ typedef __kernel_gid32_t	gid_t;
 typedef __kernel_uid16_t        uid16_t;
 typedef __kernel_gid16_t        gid16_t;
 
+struct k_uid_t {
+	uid_t uid;
+	struct user_namespace *ns;
+};
+
 typedef unsigned long		uintptr_t;
 
 #ifdef CONFIG_UID16

Index: linux-vanilla/include/linux/user_namespace.h
===================================================================

include/linux/user_namespace.h +5 -0
@@ -12,10 +12,15 @@
 struct user_namespace {
 	struct kref		kref;
 	struct hlist_head	uidhash_table[UIDHASH_SZ];
+	struct kset		*kset;
 	struct user_struct	*root_user;
 };
 
 extern struct user_namespace init_user_ns;
+extern int register_user_ns_kobj(struct user_namespace *ns);
+extern void unregister_user_ns_kobj(struct user_namespace *ns);
+extern int register_user_ns_kobj(struct user_namespace *ns);
+extern void unregister_user_ns_kobj(struct user_namespace *ns);
 
 #ifdef CONFIG_USER_NS
 

Index: linux-vanilla/kernel/user.c
===================================================================

kernel/user.c +40 -8
@@ -53,6 +53,10 @@ struct user_struct root_user = {
 	.files		= ATOMIC_INIT(0),
 	.sigpending	= ATOMIC_INIT(0),
 	.locked_shm     = 0,
+	.uid		= {
+		.uid = 0,
+		.ns = &init_user_ns,
+	},
 #ifdef CONFIG_USER_SCHED
 	.tg		= &init_task_group,
 #endif
@@ -71,13 +75,23 @@ static void uid_hash_remove(struct user_
 	hlist_del_init(&up->uidhash_node);
 }
 
-static struct user_struct *uid_hash_find(uid_t uid, struct hlist_head *hashent)
+int task_user_equiv(struct task_struct *tsk, struct user_struct *u)
+{
+	if (tsk->uid != u->uid.uid)
+		return 0;
+	if (tsk->nsproxy->user_ns != u->uid.ns)
+		return 0;
+	return 1;
+}
+
+static struct user_struct *uid_hash_find(uid_t uid,
+				struct hlist_head *hashent)
 {
 	struct user_struct *user;
 	struct hlist_node *h;
 
 	hlist_for_each_entry(user, h, hashent, uidhash_node) {
-		if (user->uid == uid) {
+		if (user->uid.uid == uid) {
 			atomic_inc(&user->__count);
 			return user;
 		}
@@ -236,6 +250,23 @@ static void uids_release(struct kobject 
 	return;
 }
 
+static const void *userns_sb_tag(struct sysfs_tag_info *info)
+{
+       return info->user_ns;
+}
+
+static const void *userns_kobject_tag(struct kobject *kobj)
+{
+       struct user_struct *up;
+       up = container_of(kobj, struct user_struct, kobj);
+       return up->uid.ns;
+}
+
+static struct sysfs_tagged_dir_operations userns_tagged_dir_operations = {
+       .sb_tag = userns_sb_tag,
+       .kobject_tag = userns_kobject_tag,
+};
+
 static struct kobj_type uids_ktype = {
 	.sysfs_ops = &kobj_sysfs_ops,
 	.default_attrs = uids_attributes,
@@ -246,19 +277,19 @@ static struct kobj_type uids_ktype = {
 static int uids_user_create(struct user_struct *up)
 {
 	struct kobject *kobj = &up->kobj;
-	int error;
+	int err;
 
 	memset(kobj, 0, sizeof(struct kobject));
 	kobj->kset = uids_kset;
-	error = kobject_init_and_add(kobj, &uids_ktype, NULL, "%d", up->uid);
-	if (error) {
+	err = kobject_init_and_add(kobj, &uids_ktype, NULL, "%d", up->uid.uid);
+	if (err) {
 		kobject_put(kobj);
 		goto done;
 	}
 
 	kobject_uevent(kobj, KOBJ_ADD);
 done:
-	return error;
+	return err;
 }
 
 /* create these entries in sysfs:
@@ -271,7 +302,7 @@ int __init uids_sysfs_init(void)
 	uids_kset = kset_create_and_add("uids", NULL, kernel_kobj);
 	if (!uids_kset)
 		return -ENOMEM;
-
+	sysfs_enable_tagging(&uids_kset->kobj, &userns_tagged_dir_operations);
 	return uids_user_create(&root_user);
 }
 
@@ -403,7 +434,8 @@ struct user_struct *alloc_uid(struct use
 		if (!new)
 			goto out_unlock;
 
-		new->uid = uid;
+		new->uid.uid = uid;
+		new->uid.ns = ns;
 		atomic_set(&new->__count, 1);
 
 		if (sched_create_user(new) < 0)

Index: linux-vanilla/kernel/user_namespace.c
===================================================================

kernel/user_namespace.c +2 -1
@@ -22,7 +22,7 @@ static struct user_namespace *clone_user
 	struct user_struct *new_user;
 	int n;
 
-	ns = kmalloc(sizeof(struct user_namespace), GFP_KERNEL);
+	ns = kzalloc(sizeof(struct user_namespace), GFP_KERNEL);
 	if (!ns)
 		return ERR_PTR(-ENOMEM);
 
@@ -71,6 +71,7 @@ void free_user_ns(struct kref *kref)
 	struct user_namespace *ns;
 
 	ns = container_of(kref, struct user_namespace, kref);
+	sysfs_userns_exit(ns);
 	release_uids(ns);
 	kfree(ns);
 }

Index: linux-vanilla/security/keys/process_keys.c
===================================================================

security/keys/process_keys.c +7 -7
@@ -47,7 +47,7 @@ static int install_user_keyrings(struct 
 	char buf[20];
 	int ret;
 
-	kenter("%p{%u}", user, user->uid);
+	kenter("%p{%u}", user, user->uid.uid);
 
 	if (user->uid_keyring) {
 		kleave(" = 0 [exist]");
@@ -62,13 +62,13 @@ static int install_user_keyrings(struct 
 		 * - there may be one in existence already as it may have been
 		 *   pinned by a session, but the user_struct pointing to it
 		 *   may have been destroyed by setuid */
-		sprintf(buf, "_uid.%u", user->uid);
+		sprintf(buf, "_uid.%u", user->uid.uid);
 
 		uid_keyring = find_keyring_by_name(buf, true);
 		if (IS_ERR(uid_keyring)) {
-			uid_keyring = keyring_alloc(buf, user->uid, (gid_t) -1,
-						    tsk, KEY_ALLOC_IN_QUOTA,
-						    NULL);
+			uid_keyring = keyring_alloc(buf, user->uid.uid,
+						    (gid_t) -1, tsk,
+						    KEY_ALLOC_IN_QUOTA, NULL);
 			if (IS_ERR(uid_keyring)) {
 				ret = PTR_ERR(uid_keyring);
 				goto error;
@@ -77,12 +77,12 @@ static int install_user_keyrings(struct 
 
 		/* get a default session keyring (which might also exist
 		 * already) */
-		sprintf(buf, "_uid_ses.%u", user->uid);
+		sprintf(buf, "_uid_ses.%u", user->uid.uid);
 
 		session_keyring = find_keyring_by_name(buf, true);
 		if (IS_ERR(session_keyring)) {
 			session_keyring =
-				keyring_alloc(buf, user->uid, (gid_t) -1,
+				keyring_alloc(buf, user->uid.uid, (gid_t) -1,
 					      tsk, KEY_ALLOC_IN_QUOTA, NULL);
 			if (IS_ERR(session_keyring)) {
 				ret = PTR_ERR(session_keyring);
Signature · 1 line
-- 
#43 lore raw
In reply to Benjamin Thery
Quoted 8 lines · Benjamin Thery

This is still the same port of Eric Biederman's patchset to implement tagged directories in sysfs that was discussed a few days ago.

This time it applies on top of 2.6.26-rc1,
which includes a fix from Daniel Lezcano to fix net device renaming for sysfs (and of course all the network namespaces stuff that was committed in net-2.6.26).

Quoted 5 lines

This patchset still contains the patch from Serge Hallyn that implements tagging for user namespaces.

It also contains a patch from Daniel Lezcano which allows to have net devices with the same name in two different network namespaces.

Does this all work properly with both CONFIG_SYSFS_DEPRECATED enabled and disabled?

Can you show some outputs of 'tree' in both cases on the /sys/class/net/ directory?

thanks,

greg k-h

#44 lore raw
Quoted 9 lines · Greg KH
Quoted 8 lines · Benjamin Thery

This is still the same port of Eric Biederman's patchset to implement tagged directories in sysfs that was discussed a few days ago.

This time it applies on top of 2.6.26-rc1,
which includes a fix from Daniel Lezcano to fix net device renaming for sysfs (and of course all the network namespaces stuff that was committed in net-2.6.26).

Quoted 8 lines
Quoted 5 lines

This patchset still contains the patch from Serge Hallyn that implements tagging for user namespaces.

It also contains a patch from Daniel Lezcano which allows to have net devices with the same name in two different network namespaces.

Does this all work properly with both CONFIG_SYSFS_DEPRECATED enabled and disabled?

Yes.
I tested both cases and with my limited knowledge of sysfs (I'm mostly interested in /sys/class/net population), this looked fine to me.

Quoted 3 lines

Can you show some outputs of 'tree' in both cases on the /sys/class/net/ directory?

Sure.
I hope you don't mind if I'll send them tomorrow I'm back home right now and have limited access to my test machines.

-benjamin

Quoted 11 lines

thanks,

greg k-h

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#45 lore raw
In reply to Greg KH
Quoted 15 lines · Greg KH
Quoted 10 lines · Benjamin Thery

This is still the same port of Eric Biederman's patchset to implement tagged directories in sysfs that was discussed a few days ago.

This time it applies on top of 2.6.26-rc1, which includes a fix from Daniel Lezcano to fix net device renaming for sysfs (and of course all the network namespaces stuff that was committed in net-2.6.26).

This patchset still contains the patch from Serge Hallyn that implements tagging for user namespaces.

It also contains a patch from Daniel Lezcano which allows to have net devices with the same name in two different network namespaces.

Does this all work properly with both CONFIG_SYSFS_DEPRECATED enabled and disabled?

Quoted 6 lines

Can you show some outputs of 'tree' in both cases on the /sys/class/net/ directory?

thanks,

greg k-h

Here is the output from an ls -l of /sys/class/net. I tryed with different configurations and checked the renamed network devices are propagated to the sysfs.

1) CONFIG_SYSFS_DEPRECATED && CONFIG_SYS && CONFIG_NET_NS
=========================================================

Outside a namespace:
--------------------

qemu:~> ip l
1: lo: <LOOPBACK,UP,10000> mtu 16436 qdisc noqueue

    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
3: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop
    link/ether 1e:43:9e:1f:1f:55 brd ff:ff:ff:ff:ff:ff
4: br0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc noqueue
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
16: veth1: <BROADCAST,MULTICAST,PROMISC> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether 8e:94:e6:72:63:cf brd ff:ff:ff:ff:ff:ff

qemu:~/lxc> ll /sys/class/net/
total 0
drwxr-xr-x 5 root root 0 2008-05-07 15:24 br0
drwxr-xr-x 3 root root 0 2008-05-07 13:16 dummy0
drwxr-xr-x 4 root root 0 2008-05-07 15:24 eth0
drwxr-xr-x 3 root root 0 2008-05-07 13:16 lo
drwxr-xr-x 4 root root 0 2008-05-07 15:26 veth1

Inside the namespace:
----------------------

qemu:~> ip l
14: lo: <LOOPBACK,UP,10000> mtu 16436 qdisc noqueue

    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
15: veth2: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether 46:85:e5:76:90:05 brd ff:ff:ff:ff:ff:ff

virtnode:~lxc # ll /sys/class/net/
total 0
drwxr-xr-x 3 root root 0 May 7 15:28 lo
drwxr-xr-x 3 root root 0 May 7 15:26 veth2

2) CONFIG_SYSFS && CONFIG_NET_NS
================================

Outside the namespace:
----------------------

qemu:~> ip l
1: lo: <LOOPBACK,UP,10000> mtu 16436 qdisc noqueue

    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
3: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop
    link/ether 72:16:3d:88:11:70 brd ff:ff:ff:ff:ff:ff
4: br0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc noqueue
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
13: veth1: <BROADCAST,MULTICAST,PROMISC> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether aa:6f:e8:dd:b8:ba brd ff:ff:ff:ff:ff:ff

* Content of /sys/class/net:

qemu:~/lxc> ll /sys/class/net/
total 0
lrwxrwxrwx 1 root root 0 2008-05-07 14:31 br0 -> ../../devices/virtual/net/br0 lrwxrwxrwx 1 root root 0 2008-05-07 14:28 dummy0 -> ../../devices/virtual/net/dummy0 lrwxrwxrwx 1 root root 0 2008-05-07 14:28 eth0 -> ../../devices/pci0000:00/0000:00:03.0/net/eth0 lrwxrwxrwx 1 root root 0 2008-05-07 14:28 lo -> ../../devices/virtual/net/lo lrwxrwxrwx 1 root root 0 2008-05-07 14:35 veth1 -> ../../devices/virtual/net/veth1

Inside the namespace:
----------------------

qemu:~> ip l
11: lo: <LOOPBACK,UP,10000> mtu 16436 qdisc noqueue

    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
12: veth2: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether ee:c5:a3:7c:58:bd brd ff:ff:ff:ff:ff:ff

virtnode:~/lxc # ll /sys/class/net/
total 0
lrwxrwxrwx 1 root root 0 May 7 14:40 lo -> ../../devices/virtual/net/lo lrwxrwxrwx 1 root root 0 May 7 14:40 veth2 -> ../../devices/virtual/net/veth2

3) CONFIG_SYSFS
===============

qemu:~> ip l
1: lo: <LOOPBACK,UP,10000> mtu 16436 qdisc noqueue

    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
3: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop
    link/ether 6e:3a:5f:2f:fa:cb brd ff:ff:ff:ff:ff:ff
4: br0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc noqueue
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff

qemu:~/lxc> ll /sys/class/net/
total 0
lrwxrwxrwx 1 root root 0 2008-05-07 15:01 br0 -> ../../devices/virtual/net/br0 lrwxrwxrwx 1 root root 0 2008-05-07 14:59 dummy0 -> ../../devices/virtual/net/dummy0 lrwxrwxrwx 1 root root 0 2008-05-07 14:59 eth0 -> ../../devices/pci0000:00/0000:00:03.0/net/eth0 lrwxrwxrwx 1 root root 0 2008-05-07 14:59 lo -> ../../devices/virtual/net/lo

4) CONFIG_SYSFS_DEPRECATED && CONFIG_SYSFS
==========================================

qemu:~> ip l
1: lo: <LOOPBACK,UP,10000> mtu 16436 qdisc noqueue

    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
3: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop
    link/ether 16:85:f0:a8:34:44 brd ff:ff:ff:ff:ff:ff
4: br0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc noqueue
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff

qemu:~> ll /sys/class/net/
total 0
drwxr-xr-x 5 root root 0 2008-05-07 15:07 br0
drwxr-xr-x 3 root root 0 2008-05-07 15:06 dummy0
drwxr-xr-x 4 root root 0 2008-05-07 15:07 eth0
drwxr-xr-x 3 root root 0 2008-05-07 15:06 lo

#46 lore raw
Quoted 25 lines · Daniel Lezcano
Quoted 6 lines · Greg KH
Quoted 4 lines · Benjamin Thery

This is still the same port of Eric Biederman's patchset to implement tagged directories in sysfs that was discussed a few days ago.

This time it applies on top of 2.6.26-rc1, which includes a fix from

Daniel Lezcano to fix net device renaming

Quoted 1 line
Quoted 1 line

for sysfs (and of course all the network namespaces stuff that was

committed in net-2.6.26).

Quoted 5 lines
Quoted 5 lines

This patchset still contains the patch from Serge Hallyn that implements tagging for user namespaces.

It also contains a patch from Daniel Lezcano which allows to have net

devices with the same name in two different network namespaces.

Quoted 8 lines
Quoted 1 line

Does this all work properly with both CONFIG_SYSFS_DEPRECATED enabled and disabled?

Can you show some outputs of 'tree' in both cases on the /sys/class/net/ directory?

Quoted 8 lines
Quoted 4 lines

thanks,

greg k-h

Here is the output from an ls -l of /sys/class/net. I tryed with different
configurations and checked the renamed network devices are propagated to the sysfs.

Thanks Daniel.
You covered more cases than I was planning to send :)

Benjamin

Quoted 146 lines

1) CONFIG_SYSFS_DEPRECATED && CONFIG_SYS && CONFIG_NET_NS
=========================================================

Outside a namespace:
--------------------

qemu:~> ip l
1: lo: <LOOPBACK,UP,10000> mtu 16436 qdisc noqueue

    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
 2: eth0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
 3: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop
    link/ether 1e:43:9e:1f:1f:55 brd ff:ff:ff:ff:ff:ff
 4: br0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc noqueue
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
 16: veth1: <BROADCAST,MULTICAST,PROMISC> mtu 1500 qdisc pfifo_fast qlen
1000
    link/ether 8e:94:e6:72:63:cf brd ff:ff:ff:ff:ff:ff

qemu:~/lxc> ll /sys/class/net/
total 0
drwxr-xr-x 5 root root 0 2008-05-07 15:24 br0
drwxr-xr-x 3 root root 0 2008-05-07 13:16 dummy0
drwxr-xr-x 4 root root 0 2008-05-07 15:24 eth0
drwxr-xr-x 3 root root 0 2008-05-07 13:16 lo
drwxr-xr-x 4 root root 0 2008-05-07 15:26 veth1

Inside the namespace:
----------------------

qemu:~> ip l
14: lo: <LOOPBACK,UP,10000> mtu 16436 qdisc noqueue

    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
 15: veth2: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc pfifo_fast qlen
1000
    link/ether 46:85:e5:76:90:05 brd ff:ff:ff:ff:ff:ff

virtnode:~lxc # ll /sys/class/net/
total 0
drwxr-xr-x 3 root root 0 May 7 15:28 lo
drwxr-xr-x 3 root root 0 May 7 15:26 veth2

2) CONFIG_SYSFS && CONFIG_NET_NS
================================

Outside the namespace:
----------------------

qemu:~> ip l
1: lo: <LOOPBACK,UP,10000> mtu 16436 qdisc noqueue

    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
 2: eth0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
 3: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop
    link/ether 72:16:3d:88:11:70 brd ff:ff:ff:ff:ff:ff
 4: br0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc noqueue
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
 13: veth1: <BROADCAST,MULTICAST,PROMISC> mtu 1500 qdisc pfifo_fast qlen
1000
    link/ether aa:6f:e8:dd:b8:ba brd ff:ff:ff:ff:ff:ff

* Content of /sys/class/net:

qemu:~/lxc> ll /sys/class/net/
total 0
lrwxrwxrwx 1 root root 0 2008-05-07 14:31 br0 ->
../../devices/virtual/net/br0
lrwxrwxrwx 1 root root 0 2008-05-07 14:28 dummy0 ->
../../devices/virtual/net/dummy0
lrwxrwxrwx 1 root root 0 2008-05-07 14:28 eth0 ->
../../devices/pci0000:00/0000:00:03.0/net/eth0
lrwxrwxrwx 1 root root 0 2008-05-07 14:28 lo ->
../../devices/virtual/net/lo
lrwxrwxrwx 1 root root 0 2008-05-07 14:35 veth1 ->
../../devices/virtual/net/veth1

Inside the namespace:
----------------------

qemu:~> ip l
11: lo: <LOOPBACK,UP,10000> mtu 16436 qdisc noqueue

    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
 12: veth2: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc pfifo_fast qlen
1000
    link/ether ee:c5:a3:7c:58:bd brd ff:ff:ff:ff:ff:ff

virtnode:~/lxc # ll /sys/class/net/
total 0
lrwxrwxrwx 1 root root 0 May 7 14:40 lo -> ../../devices/virtual/net/lo lrwxrwxrwx 1 root root 0 May 7 14:40 veth2 ->
../../devices/virtual/net/veth2

3) CONFIG_SYSFS
===============

qemu:~> ip l
1: lo: <LOOPBACK,UP,10000> mtu 16436 qdisc noqueue

    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
 2: eth0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
 3: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop
    link/ether 6e:3a:5f:2f:fa:cb brd ff:ff:ff:ff:ff:ff
 4: br0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc noqueue
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff

qemu:~/lxc> ll /sys/class/net/
total 0
lrwxrwxrwx 1 root root 0 2008-05-07 15:01 br0 ->
../../devices/virtual/net/br0
lrwxrwxrwx 1 root root 0 2008-05-07 14:59 dummy0 ->
../../devices/virtual/net/dummy0
lrwxrwxrwx 1 root root 0 2008-05-07 14:59 eth0 ->
../../devices/pci0000:00/0000:00:03.0/net/eth0
lrwxrwxrwx 1 root root 0 2008-05-07 14:59 lo ->
../../devices/virtual/net/lo

4) CONFIG_SYSFS_DEPRECATED && CONFIG_SYSFS
==========================================

qemu:~> ip l
1: lo: <LOOPBACK,UP,10000> mtu 16436 qdisc noqueue

    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
 2: eth0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
 3: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop
    link/ether 16:85:f0:a8:34:44 brd ff:ff:ff:ff:ff:ff
 4: br0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc noqueue
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff

qemu:~> ll /sys/class/net/
total 0
drwxr-xr-x 5 root root 0 2008-05-07 15:07 br0
drwxr-xr-x 3 root root 0 2008-05-07 15:06 dummy0
drwxr-xr-x 4 root root 0 2008-05-07 15:07 eth0
drwxr-xr-x 3 root root 0 2008-05-07 15:06 lo

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#47 lore raw
In reply to Daniel Lezcano

So, what is the status for this patchset?

So far, we discussed patch 10 a bit and Eric found a better way to fix the kobject_rename() issue. But there hasn't been much discussion on the rest of the patchset this time.

Does this mean that there is no strong objection against it now? Do you think the approach is suitable for what we're aiming to?

Reminder: We need this for network namespaces. Without it network namespaces testing is still a bit complicated in 2.6.26 (need to disable sysfs). Also, Serge uses it too for user namespaces.

Should I resend it one more time ported on top of a more recent tree in the hope it can be merged?
BTW which tree is the best? Linus', Greg's, -mm?

Regards,
Benjamin

Quoted 151 lines · Daniel Lezcano
Quoted 15 lines · Greg KH
Quoted 10 lines · Benjamin Thery

This is still the same port of Eric Biederman's patchset to implement tagged directories in sysfs that was discussed a few days ago.

This time it applies on top of 2.6.26-rc1, which includes a fix from Daniel Lezcano to fix net device renaming for sysfs (and of course all the network namespaces stuff that was committed in net-2.6.26).

This patchset still contains the patch from Serge Hallyn that implements tagging for user namespaces.

It also contains a patch from Daniel Lezcano which allows to have net devices with the same name in two different network namespaces.

Does this all work properly with both CONFIG_SYSFS_DEPRECATED enabled and disabled?

Quoted 6 lines

Can you show some outputs of 'tree' in both cases on the /sys/class/net/ directory?

thanks,

greg k-h

Here is the output from an ls -l of /sys/class/net. I tryed with different configurations and checked the renamed network devices are propagated to the sysfs.

1) CONFIG_SYSFS_DEPRECATED && CONFIG_SYS && CONFIG_NET_NS
=========================================================

Outside a namespace:
--------------------

qemu:~> ip l
1: lo: <LOOPBACK,UP,10000> mtu 16436 qdisc noqueue

    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
3: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop
    link/ether 1e:43:9e:1f:1f:55 brd ff:ff:ff:ff:ff:ff
4: br0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc noqueue
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
16: veth1: <BROADCAST,MULTICAST,PROMISC> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether 8e:94:e6:72:63:cf brd ff:ff:ff:ff:ff:ff

qemu:~/lxc> ll /sys/class/net/
total 0
drwxr-xr-x 5 root root 0 2008-05-07 15:24 br0
drwxr-xr-x 3 root root 0 2008-05-07 13:16 dummy0
drwxr-xr-x 4 root root 0 2008-05-07 15:24 eth0
drwxr-xr-x 3 root root 0 2008-05-07 13:16 lo
drwxr-xr-x 4 root root 0 2008-05-07 15:26 veth1

Inside the namespace:
----------------------

qemu:~> ip l
14: lo: <LOOPBACK,UP,10000> mtu 16436 qdisc noqueue

    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
15: veth2: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether 46:85:e5:76:90:05 brd ff:ff:ff:ff:ff:ff

virtnode:~lxc # ll /sys/class/net/
total 0
drwxr-xr-x 3 root root 0 May 7 15:28 lo
drwxr-xr-x 3 root root 0 May 7 15:26 veth2

2) CONFIG_SYSFS && CONFIG_NET_NS
================================

Outside the namespace:
----------------------

qemu:~> ip l
1: lo: <LOOPBACK,UP,10000> mtu 16436 qdisc noqueue

    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
3: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop
    link/ether 72:16:3d:88:11:70 brd ff:ff:ff:ff:ff:ff
4: br0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc noqueue
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
13: veth1: <BROADCAST,MULTICAST,PROMISC> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether aa:6f:e8:dd:b8:ba brd ff:ff:ff:ff:ff:ff

* Content of /sys/class/net:

qemu:~/lxc> ll /sys/class/net/
total 0
lrwxrwxrwx 1 root root 0 2008-05-07 14:31 br0 -> ../../devices/virtual/net/br0 lrwxrwxrwx 1 root root 0 2008-05-07 14:28 dummy0 -> ../../devices/virtual/net/dummy0 lrwxrwxrwx 1 root root 0 2008-05-07 14:28 eth0 -> ../../devices/pci0000:00/0000:00:03.0/net/eth0 lrwxrwxrwx 1 root root 0 2008-05-07 14:28 lo -> ../../devices/virtual/net/lo lrwxrwxrwx 1 root root 0 2008-05-07 14:35 veth1 -> ../../devices/virtual/net/veth1

Inside the namespace:
----------------------

qemu:~> ip l
11: lo: <LOOPBACK,UP,10000> mtu 16436 qdisc noqueue

    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
12: veth2: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether ee:c5:a3:7c:58:bd brd ff:ff:ff:ff:ff:ff

virtnode:~/lxc # ll /sys/class/net/
total 0
lrwxrwxrwx 1 root root 0 May 7 14:40 lo -> ../../devices/virtual/net/lo lrwxrwxrwx 1 root root 0 May 7 14:40 veth2 -> ../../devices/virtual/net/veth2

3) CONFIG_SYSFS
===============

qemu:~> ip l
1: lo: <LOOPBACK,UP,10000> mtu 16436 qdisc noqueue

    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
3: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop
    link/ether 6e:3a:5f:2f:fa:cb brd ff:ff:ff:ff:ff:ff
4: br0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc noqueue
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff

qemu:~/lxc> ll /sys/class/net/
total 0
lrwxrwxrwx 1 root root 0 2008-05-07 15:01 br0 -> ../../devices/virtual/net/br0 lrwxrwxrwx 1 root root 0 2008-05-07 14:59 dummy0 -> ../../devices/virtual/net/dummy0 lrwxrwxrwx 1 root root 0 2008-05-07 14:59 eth0 -> ../../devices/pci0000:00/0000:00:03.0/net/eth0 lrwxrwxrwx 1 root root 0 2008-05-07 14:59 lo -> ../../devices/virtual/net/lo

4) CONFIG_SYSFS_DEPRECATED && CONFIG_SYSFS
==========================================

qemu:~> ip l
1: lo: <LOOPBACK,UP,10000> mtu 16436 qdisc noqueue

    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
3: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop
    link/ether 16:85:f0:a8:34:44 brd ff:ff:ff:ff:ff:ff
4: br0: <BROADCAST,MULTICAST,UP,10000> mtu 1500 qdisc noqueue
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff

qemu:~> ll /sys/class/net/
total 0
drwxr-xr-x 5 root root 0 2008-05-07 15:07 br0
drwxr-xr-x 3 root root 0 2008-05-07 15:06 dummy0
drwxr-xr-x 4 root root 0 2008-05-07 15:07 eth0
drwxr-xr-x 3 root root 0 2008-05-07 15:06 lo

Signature · 3 lines
B e n j a m i n   T h e r y  - BULL/DT/Open Software R&D

   http://www.bull.com