Hi,
I'm hitting a strange issue and/or I'm completely lost in sysfs internals.
Consider having two net_device *a, *b; which are registered normally.
Now, to create a link from /sys/class/net/a->name/linkname to b, one should
use:
sysfs_create_link(&(a->dev.kobj), &(b->dev.kobj), linkname);
To remove it, even simpler:
sysfs_remove_link(&(a->dev.kobj), linkname);
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);
this fails with:
"sysfs: ns invalid in 'a->name' for 'oldname'"
in
608 struct sysfs_dirent *sysfs_find_dirent(struct sysfs_dirent *parent_sd,
...
615 if (!!sysfs_ns_type(parent_sd) != !!ns) {
616 WARN(1, KERN_WARNING "sysfs: ns %s in '%s' for '%s'\n",
617 sysfs_ns_type(parent_sd) ? "required" : "invalid",
618 parent_sd->s_name, name);
619 return NULL;
620 }
Code path:
warn_slowpath_fmt+0x46/0x50
sysfs_get_dirent_ns+0x30/0x80
sysfs_find_dirent+0x84/0x110
sysfs_get_dirent_ns+0x3e/0x80
sysfs_rename_link_ns+0x54/0xd0
I have no idea what this code means. Is there any reason for it to
fail (i.e. am I doing something wrong?) or I've hit a bug?
I've tested the only user of it (bridge) - and it works fine, however it's
not using its own net_device's kobject but rather its own dir.
Thank you!
On Tue, Jan 14, 2014 at 06:17:40PM +0100, Veaceslav Falico wrote:
> Hi,
>
> I'm hitting a strange issue and/or I'm completely lost in sysfs internals.
>
> Consider having two net_device *a, *b; which are registered normally.
> Now, to create a link from /sys/class/net/a->name/linkname to b, one should
> use:
>
> sysfs_create_link(&(a->dev.kobj), &(b->dev.kobj), linkname);
>
> To remove it, even simpler:
>
> sysfs_remove_link(&(a->dev.kobj), linkname);
>
> 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?
> this fails with:
>
> "sysfs: ns invalid in 'a->name' for 'oldname'"
Looks like the namespace for this link isn't valid.
> in
>
> 608 struct sysfs_dirent *sysfs_find_dirent(struct sysfs_dirent *parent_sd,
> ...
> 615 if (!!sysfs_ns_type(parent_sd) != !!ns) {
> 616 WARN(1, KERN_WARNING "sysfs: ns %s in '%s' for '%s'\n",
> 617 sysfs_ns_type(parent_sd) ? "required" : "invalid",
> 618 parent_sd->s_name, name);
> 619 return NULL;
> 620 }
>
> Code path:
> warn_slowpath_fmt+0x46/0x50
> sysfs_get_dirent_ns+0x30/0x80
> sysfs_find_dirent+0x84/0x110
> sysfs_get_dirent_ns+0x3e/0x80
> sysfs_rename_link_ns+0x54/0xd0
>
> I have no idea what this code means. Is there any reason for it to
> fail (i.e. am I doing something wrong?) or I've hit a bug?
What exactly are you trying to do here? Care to provide a pointer to
your code somewhere?
> I've tested the only user of it (bridge) - and it works fine, however it's
> not using its own net_device's kobject but rather its own dir.
The driver core also uses this function, and it works there, so I'd
blame your code :)
thanks,
greg k-h
On Tue, Jan 14, 2014 at 10:21:35AM -0800, Greg KH wrote:
>On Tue, Jan 14, 2014 at 06:17:40PM +0100, Veaceslav Falico wrote:
>> Hi,
>>
>> I'm hitting a strange issue and/or I'm completely lost in sysfs internals.
>>
>> Consider having two net_device *a, *b; which are registered normally.
>> Now, to create a link from /sys/class/net/a->name/linkname to b, one should
>> use:
>>
>> sysfs_create_link(&(a->dev.kobj), &(b->dev.kobj), linkname);
>>
>> To remove it, even simpler:
>>
>> sysfs_remove_link(&(a->dev.kobj), linkname);
>>
>> 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.
And wrt namespace - there are two functions, one is sysfs_rename_link(),
which calls the second one - sysfs_rename_link_ns() with NULL namespace.
>
>> this fails with:
>>
>> "sysfs: ns invalid in 'a->name' for 'oldname'"
>
>Looks like the namespace for this link isn't valid.
Yep, though dunno why.
>
>> in
>>
>> 608 struct sysfs_dirent *sysfs_find_dirent(struct sysfs_dirent *parent_sd,
>> ...
>> 615 if (!!sysfs_ns_type(parent_sd) != !!ns) {
>> 616 WARN(1, KERN_WARNING "sysfs: ns %s in '%s' for '%s'\n",
>> 617 sysfs_ns_type(parent_sd) ? "required" : "invalid",
>> 618 parent_sd->s_name, name);
>> 619 return NULL;
>> 620 }
>>
>> Code path:
>> warn_slowpath_fmt+0x46/0x50
>> sysfs_get_dirent_ns+0x30/0x80
>> sysfs_find_dirent+0x84/0x110
>> sysfs_get_dirent_ns+0x3e/0x80
>> sysfs_rename_link_ns+0x54/0xd0
>>
>> I have no idea what this code means. Is there any reason for it to
>> fail (i.e. am I doing something wrong?) or I've hit a bug?
>
>What exactly are you trying to do here? Care to provide a pointer to
>your code somewhere?
I've tried to handle the network device renames, ended up doing it with
sysfs_remove/add_link() calls. Patches:
http://patchwork.ozlabs.org/patch/310798/
http://patchwork.ozlabs.org/patch/310796/
If an upper device (say, vlan, bonding, bridge etc.) enslaves another
device, symlinks are created (given bridge0 and eth0):
/sys/class/net/eth0/upper_bridge0 -> ../bridge0
/sys/class/net/bridge0/lower_eth0 -> ../eth0
However, in case someone renames eth0/bridge0, these symlinks should also
be renamed, and sysfs_rename_link() should fit great here. But it fails.
>
>> I've tested the only user of it (bridge) - and it works fine, however it's
>> not using its own net_device's kobject but rather its own dir.
>
>The driver core also uses this function, and it works there, so I'd
>blame your code :)
Yeah, I'd also like to blame it, I'm afraid of touching anything
sysfs-related :).
>
>thanks,
>
>greg k-h
On Tue, Jan 14, 2014 at 08:12:08PM +0100, Veaceslav Falico wrote:
> On Tue, Jan 14, 2014 at 10:21:35AM -0800, Greg KH wrote:
> >On Tue, Jan 14, 2014 at 06:17:40PM +0100, Veaceslav Falico wrote:
> >>Hi,
> >>
> >>I'm hitting a strange issue and/or I'm completely lost in sysfs internals.
> >>
> >>Consider having two net_device *a, *b; which are registered normally.
> >>Now, to create a link from /sys/class/net/a->name/linkname to b, one should
> >>use:
> >>
> >>sysfs_create_link(&(a->dev.kobj), &(b->dev.kobj), linkname);
> >>
> >>To remove it, even simpler:
> >>
> >>sysfs_remove_link(&(a->dev.kobj), linkname);
> >>
> >>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.
>
> And wrt namespace - there are two functions, one is sysfs_rename_link(),
> which calls the second one - sysfs_rename_link_ns() with NULL namespace.
>
> >
> >>this fails with:
> >>
> >>"sysfs: ns invalid in 'a->name' for 'oldname'"
> >
> >Looks like the namespace for this link isn't valid.
>
> Yep, though dunno why.
Are you testing this with network namespaces enabled? Perhaps that is
why, you need to specify the namespace of the link that you are
changing.
The fact that the bridge link works is odd to me, I would think that it
too needs to specify the network namespace involved, but perhaps bridge
objects aren't part of any specific network namespace? I don't know the
bridging code at all, sorry.
So try calling sysfs_rename_link_ns() and specify the namespace of the
kobject you are changing, and see if that works or not.
thanks,
greg k-h
On Tue, Jan 14, 2014 at 11:31:39AM -0800, Greg KH wrote:
>On Tue, Jan 14, 2014 at 08:12:08PM +0100, Veaceslav Falico wrote:
>> On Tue, Jan 14, 2014 at 10:21:35AM -0800, Greg KH wrote:
>> >On Tue, Jan 14, 2014 at 06:17:40PM +0100, Veaceslav Falico wrote:
>> >>Hi,
>> >>
>> >>I'm hitting a strange issue and/or I'm completely lost in sysfs internals.
>> >>
>> >>Consider having two net_device *a, *b; which are registered normally.
>> >>Now, to create a link from /sys/class/net/a->name/linkname to b, one should
>> >>use:
>> >>
>> >>sysfs_create_link(&(a->dev.kobj), &(b->dev.kobj), linkname);
>> >>
>> >>To remove it, even simpler:
>> >>
>> >>sysfs_remove_link(&(a->dev.kobj), linkname);
>> >>
>> >>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.
>>
>> And wrt namespace - there are two functions, one is sysfs_rename_link(),
>> which calls the second one - sysfs_rename_link_ns() with NULL namespace.
>>
>> >
>> >>this fails with:
>> >>
>> >>"sysfs: ns invalid in 'a->name' for 'oldname'"
>> >
>> >Looks like the namespace for this link isn't valid.
>>
>> Yep, though dunno why.
>
>Are you testing this with network namespaces enabled? Perhaps that is
>why, you need to specify the namespace of the link that you are
>changing.
>
>The fact that the bridge link works is odd to me, I would think that it
>too needs to specify the network namespace involved, but perhaps bridge
>objects aren't part of any specific network namespace? I don't know the
>bridging code at all, sorry.
Yep, might be it, will test soon and come back with the results.
What still bugs me, though, is the logic - why is it possible to remove/add
without specifying namespace, while it fails to rename it? Maybe the rename
function should do a better job at detecting the namespace?
>
>So try calling sysfs_rename_link_ns() and specify the namespace of the
>kobject you are changing, and see if that works or not.
>
>thanks,
>
>greg k-h
On Tue, Jan 14, 2014 at 10:06:10PM +0100, Veaceslav Falico wrote:
> On Tue, Jan 14, 2014 at 11:31:39AM -0800, Greg KH wrote:
> >On Tue, Jan 14, 2014 at 08:12:08PM +0100, Veaceslav Falico wrote:
> >>On Tue, Jan 14, 2014 at 10:21:35AM -0800, Greg KH wrote:
> >>>On Tue, Jan 14, 2014 at 06:17:40PM +0100, Veaceslav Falico wrote:
> >>>>Hi,
> >>>>
> >>>>I'm hitting a strange issue and/or I'm completely lost in sysfs internals.
> >>>>
> >>>>Consider having two net_device *a, *b; which are registered normally.
> >>>>Now, to create a link from /sys/class/net/a->name/linkname to b, one should
> >>>>use:
> >>>>
> >>>>sysfs_create_link(&(a->dev.kobj), &(b->dev.kobj), linkname);
> >>>>
> >>>>To remove it, even simpler:
> >>>>
> >>>>sysfs_remove_link(&(a->dev.kobj), linkname);
> >>>>
> >>>>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.
> >>
> >>And wrt namespace - there are two functions, one is sysfs_rename_link(),
> >>which calls the second one - sysfs_rename_link_ns() with NULL namespace.
> >>
> >>>
> >>>>this fails with:
> >>>>
> >>>>"sysfs: ns invalid in 'a->name' for 'oldname'"
> >>>
> >>>Looks like the namespace for this link isn't valid.
> >>
> >>Yep, though dunno why.
> >
> >Are you testing this with network namespaces enabled? Perhaps that is
> >why, you need to specify the namespace of the link that you are
> >changing.
> >
> >The fact that the bridge link works is odd to me, I would think that it
> >too needs to specify the network namespace involved, but perhaps bridge
> >objects aren't part of any specific network namespace? I don't know the
> >bridging code at all, sorry.
>
> Yep, might be it, will test soon and come back with the results.
>
> What still bugs me, though, is the logic - why is it possible to remove/add
> without specifying namespace, while it fails to rename it? Maybe the rename
> function should do a better job at detecting the namespace?
Yes, maybe it should, patches are always gladly welcome :)
thanks,
greg k-h
Veaceslav Falico <[email protected]> writes:
> On Tue, Jan 14, 2014 at 11:31:39AM -0800, Greg KH wrote:
>>On Tue, Jan 14, 2014 at 08:12:08PM +0100, Veaceslav Falico wrote:
>>> On Tue, Jan 14, 2014 at 10:21:35AM -0800, Greg KH wrote:
>>> >On Tue, Jan 14, 2014 at 06:17:40PM +0100, Veaceslav Falico wrote:
>>> >>Hi,
>>> >>
>>> >>I'm hitting a strange issue and/or I'm completely lost in sysfs internals.
>>> >>
>>> >>Consider having two net_device *a, *b; which are registered normally.
>>> >>Now, to create a link from /sys/class/net/a->name/linkname to b, one should
>>> >>use:
>>> >>
>>> >>sysfs_create_link(&(a->dev.kobj), &(b->dev.kobj), linkname);
>>> >>
>>> >>To remove it, even simpler:
>>> >>
>>> >>sysfs_remove_link(&(a->dev.kobj), linkname);
>>> >>
>>> >>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.
>>>
>>> And wrt namespace - there are two functions, one is sysfs_rename_link(),
>>> which calls the second one - sysfs_rename_link_ns() with NULL namespace.
>>>
>>> >
>>> >>this fails with:
>>> >>
>>> >>"sysfs: ns invalid in 'a->name' for 'oldname'"
>>> >
>>> >Looks like the namespace for this link isn't valid.
>>>
>>> Yep, though dunno why.
>>
>>Are you testing this with network namespaces enabled? Perhaps that is
>>why, you need to specify the namespace of the link that you are
>>changing.
>>
>>The fact that the bridge link works is odd to me, I would think that it
>>too needs to specify the network namespace involved, but perhaps bridge
>>objects aren't part of any specific network namespace? I don't know the
>>bridging code at all, sorry.
>
> Yep, might be it, will test soon and come back with the results.
>
> What still bugs me, though, is the logic - why is it possible to remove/add
> without specifying namespace, while it fails to rename it? Maybe the rename
> function should do a better job at detecting the namespace?
The basic sysfs_rename primitive and device_rename support moving
network devices across namespaces. Since the new name can be in a new
namespace the network namespace the new network namespace needs to be
specified.
The reason the bridge code is different is that apparently Tejun broke
the bridge code when he converted sysfs. Apparently no one has tested
the appropriate bridge path with network namespaces enabled and screamed
yet.
sysfs_rename_link was originally written to infer figure everything out
based on the target device. Because symlinks only make sense being in
the same namespace of their devices. Tejun broke the inference and now
you are having hard time using the code.
Tejun could you please take care of this breakage.
The commit that broke things was:
commit 4b30ee58ee64c64f59fd876e4afa6ed82caef3a4
Author: Tejun Heo <[email protected]>
Date: Wed Sep 11 22:29:06 2013 -0400
sysfs: remove ktype->namespace() invocations in symlink code
There's no reason for sysfs to be calling ktype->namespace(). It is
backwards, obfuscates what's going on and unnecessarily tangles two
separate layers.
There are two places where symlink code calls ktype->namespace().
* sysfs_do_create_link_sd() calls it to find out the namespace tag of
the target directory. Unless symlinking races with cross-namespace
renaming, this equals @target_sd->s_ns.
* sysfs_rename_link() uses it to find out the new namespace to rename
to and the new namespace can be different from the existing one.
The function is renamed to sysfs_rename_link_ns() with an explicit
@ns argument and the ktype->namespace() invocation is shifted to the
device layer.
While this patch replaces ktype->namespace() invocation with the
recorded result in @target_sd, this shouldn't result in any behvior
difference.
Signed-off-by: Tejun Heo <[email protected]>
Cc: Eric W. Biederman <[email protected]>
Cc: Kay Sievers <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Eric
On 2014/1/15 1:17, Veaceslav Falico wrote:
> Hi,
>
> I'm hitting a strange issue and/or I'm completely lost in sysfs internals.
>
> Consider having two net_device *a, *b; which are registered normally.
> Now, to create a link from /sys/class/net/a->name/linkname to b, one should
> use:
>
> sysfs_create_link(&(a->dev.kobj), &(b->dev.kobj), linkname);
>
> To remove it, even simpler:
>
> sysfs_remove_link(&(a->dev.kobj), linkname);
>
> 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);
>
> this fails with:
>
> "sysfs: ns invalid in 'a->name' for 'oldname'"
>
> in
>
> 608 struct sysfs_dirent *sysfs_find_dirent(struct sysfs_dirent *parent_sd,
> ...
> 615 if (!!sysfs_ns_type(parent_sd) != !!ns) {
> 616 WARN(1, KERN_WARNING "sysfs: ns %s in '%s' for '%s'\n",
> 617 sysfs_ns_type(parent_sd) ? "required" : "invalid",
> 618 parent_sd->s_name, name);
> 619 return NULL;
> 620 }
>
> Code path:
> warn_slowpath_fmt+0x46/0x50
> sysfs_get_dirent_ns+0x30/0x80
> sysfs_find_dirent+0x84/0x110
> sysfs_get_dirent_ns+0x3e/0x80
> sysfs_rename_link_ns+0x54/0xd0
>
> I have no idea what this code means. Is there any reason for it to
> fail (i.e. am I doing something wrong?) or I've hit a bug?
>
> I've tested the only user of it (bridge) - and it works fine, however it's
> not using its own net_device's kobject but rather its own dir.
>
I use the sysfs_rename_link(x,x) and meet the same problem, I review the code for bridge,
I found the br->ifobj was using kobject_create_and_add() to add a subdir for this, maybe it
helps?
Ding
> Thank you!
> --
> 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
>
>
Hey, Veaceslav, 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?
> sysfs_rename_link was originally written to infer figure everything out
> based on the target device. Because symlinks only make sense being in
> the same namespace of their devices. Tejun broke the inference and now
> you are having hard time using the code.
I wouldn't be surprised if I broke it but
> The commit that broke things was:
>
> commit 4b30ee58ee64c64f59fd876e4afa6ed82caef3a4
> Author: Tejun Heo <[email protected]>
> Date: Wed Sep 11 22:29:06 2013 -0400
>
> sysfs: remove ktype->namespace() invocations in symlink code
I'm not so sure about the above commit. The warning is from rename
failing to locate the existing node while the above patch updates the
target ns to rename to. The old_ns part stays the same before and
after the commit.
Veaceslav, please confirm whether the issue is reproducible w/ v3.12.
Thanks.
--
tejun
Tejun Heo <[email protected]> writes:
> Hey, Veaceslav, 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?
>
>> sysfs_rename_link was originally written to infer figure everything out
>> based on the target device. Because symlinks only make sense being in
>> the same namespace of their devices. Tejun broke the inference and now
>> you are having hard time using the code.
>
> I wouldn't be surprised if I broke it but
>
>> The commit that broke things was:
>>
>> commit 4b30ee58ee64c64f59fd876e4afa6ed82caef3a4
>> Author: Tejun Heo <[email protected]>
>> Date: Wed Sep 11 22:29:06 2013 -0400
>>
>> sysfs: remove ktype->namespace() invocations in symlink code
>
> I'm not so sure about the above commit. The warning is from rename
> failing to locate the existing node while the above patch updates the
> target ns to rename to. The old_ns part stays the same before and
> after the commit.
So passing a NULL namespace when a node already has a namespace will
cause that warning.
I'm not quite certain about the bridge code. It should be in a network
namespace but at first glance I am not seeing that.
However what is most definitely broken is the usablitily of
sysfs_rename_link. Renaming symlinks used to just do the right thing,
and figure out all of the namespace bits and not making the callers
worry about it. Now there are new callers that can't figure out how to
call the function. That is a huge usability breakage, and the commit I
singled out is definitely one of them.
> 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.
Eric
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 67b180d855b2..0c9377a5bb89 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 3ae3f1bf1a09..651444a9d1c5 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 6695040a0317..093d99244290 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)
{
Hey, Eric.
On Wed, Jan 15, 2014 at 03:25:16PM -0800, Eric W. Biederman wrote:
> diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
> index 3ae3f1bf1a09..651444a9d1c5 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));
Oh yeah, this is a lot better. It's kinda weird that it requires
sysfs user to invoke sysfs_rename_link() without actually specifying
anything which has changed after namespace update tho. The best thing
would be just tying the namespace of symlink to its target so that it
gets updated automatically along with the target without requiring
this separate step. Ah well, we don't maintain backlinks (yet), so
this should do for now, I suppose.
Thanks.
--
tejun
On Wed, Jan 15, 2014 at 03:25:16PM -0800, Eric W. Biederman wrote:
>Tejun Heo <[email protected]> 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);
}
}
Veaceslav Falico <[email protected]> writes:
> On Wed, Jan 15, 2014 at 03:25:16PM -0800, Eric W. Biederman wrote:
>>Tejun Heo <[email protected]> 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);
> }
> }
>