Re: [RFC] sysfs_rename_link() and its usage

[ebiederm@xmission.com (Eric W. Biederman)]

Veaceslav Falico <vfalico@redhat.com> writes:

> On Wed, Jan 15, 2014 at 03:25:16PM -0800, Eric W. Biederman wrote:
>>Tejun Heo <tj@kernel.org> 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:

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)



>
> [   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);
>  	}
>  }
>