Re: [RFC] sysfs_rename_link() and its usage
From: Eric W. Biederman <hidden>
Date: 2014-01-16 23:35:08
Also in:
lkml
Subsystem:
driver core, kobjects, debugfs and sysfs, filesystems (vfs and infrastructure), the rest · Maintainers:
Greg Kroah-Hartman, "Rafael J. Wysocki", Danilo Krummrich, Alexander Viro, Christian Brauner, Linus Torvalds
Possibly related (same subject, not in this thread)
- 2014-01-16 · Re: [RFC] sysfs_rename_link() and its usage · Veaceslav Falico <hidden>
- 2014-01-15 · Re: [RFC] sysfs_rename_link() and its usage · Tejun Heo <tj@kernel.org>
- 2014-01-15 · Re: [RFC] sysfs_rename_link() and its usage · Eric W. Biederman <hidden>
- 2014-01-15 · Re: [RFC] sysfs_rename_link() and its usage · Tejun Heo <tj@kernel.org>
- 2014-01-15 · Re: [RFC] sysfs_rename_link() and its usage · Ding Tianhong <hidden>
Veaceslav Falico [off-list ref] writes:
On Wed, Jan 15, 2014 at 03:25:16PM -0800, Eric W. Biederman wrote:quoted
Tejun Heo [off-list ref] writes:quoted
Hey, Veaceslav, Eric.Hi Tejun, Eric,quoted
quoted
On Tue, Jan 14, 2014 at 05:35:23PM -0800, Eric W. Biederman wrote:quoted
quoted
quoted
quoted
quoted
quoted
This works like a charm. However, if I want to use (obviously, with the symlink present): sysfs_rename_link(&(a->dev.kobj), &(b->dev.kobj), oldname, newname);You forgot the namespace option to this call, what kernel version are you using here?It's git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next , 3.13-rc6 with some networking patches on top of it.Does this work on 3.12? How about Greg's driver-core-next? Do you have a minimal test case that I can use to reproduce the issue?Sorry for the latency in responses, I'll update once I'll manage to test it on those. ...snip...quoted
quoted
Veaceslav, please confirm whether the issue is reproducible w/ v3.12.Anyway since a symlink living in a different namespace from it's target is just nonsense this (only compile tested) patch should fix the issue, and make sysfs_rename_link usable for people without a masters degree in sysfs again.It's still there :-(. I've used your patch and added my small addition[1] to test the sysfs_rename_link() (on top of net-next, 3.13-rc7), however the issue is still there:
I expect the bug is my quick patch missed testing for sysfs_ns_type to see if we care at all about namespaces in sysfs_rename_link. Which would make just the sysfs_rename_link bit look like. Something like that. Eric
diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index 3ae3f1bf1a09..8b51d1b6cc21 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c@@ -194,15 +194,13 @@ EXPORT_SYMBOL_GPL(sysfs_remove_link); * @targ: object we're pointing to. * @old: previous name of the symlink. * @new: new name of the symlink. - * @new_ns: new namespace of the symlink. - * * A helper function for the common rename symlink idiom. */ -int sysfs_rename_link_ns(struct kobject *kobj, struct kobject *targ, - const char *old, const char *new, const void *new_ns) +int sysfs_rename_link(struct kobject *kobj, struct kobject *targ, + const char *old, const char *new) { struct sysfs_dirent *parent_sd, *sd = NULL; - const void *old_ns = NULL; + const void *old_ns = NULL, *new_ns = NULL; int result; if (!kobj)
@@ -224,13 +222,16 @@ int sysfs_rename_link_ns(struct kobject *kobj, struct kobject *targ, if (sd->s_symlink.target_sd->s_dir.kobj != targ) goto out; + if (sysfs_ns_type(parent_sd)) + new_ns = kobject_namespace(targ); + result = sysfs_rename(sd, parent_sd, new, new_ns); out: sysfs_put(sd); return result; } -EXPORT_SYMBOL_GPL(sysfs_rename_link_ns); +EXPORT_SYMBOL_GPL(sysfs_rename_link); static int sysfs_get_target_path(struct sysfs_dirent *parent_sd, struct sysfs_dirent *target_sd, char *path)
quoted hunk
[ 79.038340] net bond0: renaming to bondbla [ 79.038380] ------------[ cut here ]------------ [ 79.038411] WARNING: CPU: 1 PID: 5318 at fs/sysfs/dir.c:618 sysfs_find_dirent+0x84/0x110() [ 79.038449] sysfs: ns invalid in 'bridge0' for 'lower_bond0' ...snip... [ 79.038877] [<ffffffff810ae826>] warn_slowpath_fmt+0x46/0x50 [ 79.038903] [<ffffffff812ba890>] ? sysfs_get_dirent_ns+0x30/0x80 [ 79.038930] [<ffffffff812b97c4>] sysfs_find_dirent+0x84/0x110 [ 79.038957] [<ffffffff812ba89e>] sysfs_get_dirent_ns+0x3e/0x80 [ 79.038983] [<ffffffff812baf87>] sysfs_rename_link+0x57/0xe0 [ 79.039030] [<ffffffff81689e72>] netdev_adjacent_rename_links+0xa2/0x160 The current scheme (sysfs_remove_link() + sysfs_add_link()) works perfectly well without any namespaces. I'll dig into it once I have some spare time, it's not at all critical. [1]: the patch (I've included your patch too, just in case):diff --git a/drivers/base/core.c b/drivers/base/core.c index 67b180d..0c9377a 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c@@ -1825,9 +1825,8 @@ int device_rename(struct device *dev, const char *new_name) } if (dev->class) { - error = sysfs_rename_link_ns(&dev->class->p->subsys.kobj, - kobj, old_device_name, - new_name, kobject_namespace(kobj)); + error = sysfs_rename_link(&dev->class->p->subsys.kobj, + kobj, old_device_name, new_name); if (error) goto out; }diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c index 3ae3f1b..651444a 100644 --- a/fs/sysfs/symlink.c +++ b/fs/sysfs/symlink.c@@ -194,12 +194,10 @@ EXPORT_SYMBOL_GPL(sysfs_remove_link); * @targ: object we're pointing to. * @old: previous name of the symlink. * @new: new name of the symlink. - * @new_ns: new namespace of the symlink. - * * A helper function for the common rename symlink idiom. */ -int sysfs_rename_link_ns(struct kobject *kobj, struct kobject *targ, - const char *old, const char *new, const void *new_ns) +int sysfs_rename_link(struct kobject *kobj, struct kobject *targ, + const char *old, const char *new) { struct sysfs_dirent *parent_sd, *sd = NULL; const void *old_ns = NULL;@@ -224,13 +222,13 @@ int sysfs_rename_link_ns(struct kobject *kobj, struct kobject *targ, if (sd->s_symlink.target_sd->s_dir.kobj != targ) goto out; - result = sysfs_rename(sd, parent_sd, new, new_ns); + result = sysfs_rename(sd, parent_sd, new, kobject_namespace(targ)); out: sysfs_put(sd); return result; } -EXPORT_SYMBOL_GPL(sysfs_rename_link_ns); +EXPORT_SYMBOL_GPL(sysfs_rename_link); static int sysfs_get_target_path(struct sysfs_dirent *parent_sd, struct sysfs_dirent *target_sd, char *path)diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h index 6695040..093d992 100644 --- a/include/linux/sysfs.h +++ b/include/linux/sysfs.h@@ -213,9 +213,8 @@ int __must_check sysfs_create_link_nowarn(struct kobject *kobj, const char *name); void sysfs_remove_link(struct kobject *kobj, const char *name); -int sysfs_rename_link_ns(struct kobject *kobj, struct kobject *target, - const char *old_name, const char *new_name, - const void *new_ns); +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);@@ -341,9 +340,8 @@ static inline void sysfs_remove_link(struct kobject *kobj, const char *name) { } -static inline int sysfs_rename_link_ns(struct kobject *k, struct kobject *t, - const char *old_name, - const char *new_name, const void *ns) +static inline int sysfs_rename_link(struct kobject *k, struct kobject *t, + const char *old_name, const char *new_name) { return 0; }@@ -455,12 +453,6 @@ static inline void sysfs_remove_file(struct kobject *kobj, return sysfs_remove_file_ns(kobj, attr, NULL); } -static inline int sysfs_rename_link(struct kobject *kobj, struct kobject*target, - const char *old_name, const char *new_name) -{ - return sysfs_rename_link_ns(kobj, target, old_name, new_name, NULL); -} - static inline struct sysfs_dirent * sysfs_get_dirent(struct sysfs_dirent *parent_sd, const unsigned char *name) {diff --git a/net/core/dev.c b/net/core/dev.c index 9957557..5d24d8e 100644 --- a/net/core/dev.c +++ b/net/core/dev.c@@ -5005,19 +5005,20 @@ EXPORT_SYMBOL(netdev_upper_dev_unlink); void netdev_adjacent_rename_links(struct net_device *dev, char *oldname) { struct netdev_adjacent *iter; + char old_linkname[IFNAMSIZ+7], new_linkname[IFNAMSIZ+7]; list_for_each_entry(iter, &dev->adj_list.upper, list) { - netdev_adjacent_sysfs_del(iter->dev, oldname, - &iter->dev->adj_list.lower); - netdev_adjacent_sysfs_add(iter->dev, dev, - &iter->dev->adj_list.lower); + sprintf(old_linkname, "lower_%s", oldname); + sprintf(new_linkname, "lower_%s", dev->name); + sysfs_rename_link(&(iter->dev->dev.kobj), &(dev->dev.kobj), + old_linkname, new_linkname); } list_for_each_entry(iter, &dev->adj_list.lower, list) { - netdev_adjacent_sysfs_del(iter->dev, oldname, - &iter->dev->adj_list.upper); - netdev_adjacent_sysfs_add(iter->dev, dev, - &iter->dev->adj_list.upper); + sprintf(old_linkname, "upper_%s", oldname); + sprintf(new_linkname, "upper_%s", dev->name); + sysfs_rename_link(&(iter->dev->dev.kobj), &(dev->dev.kobj), + old_linkname, new_linkname); } }