Re: [RFC] sysfs_rename_link() and its usage

From: Veaceslav Falico
Date: Wed Jan 15 2014 - 19:14:21 EST

On Wed, Jan 15, 2014 at 03:25:16PM -0800, Eric W. Biederman wrote:
Tejun Heo <tj@xxxxxxxxxx> writes:

Hey, Veaceslav, Eric.

Hi Tejun, Eric,


On Tue, Jan 14, 2014 at 05:35:23PM -0800, Eric W. Biederman wrote:
>>> >>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...
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:

[ 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);
}
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/