New: sysfs changes break hwsim and bnep drivers
Subsystems (6)
A message that is not in the archive (sent off-list or not yet received): [email protected]/
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 9 lines · Andrew Morton
Quoted 1 line · On Sun, 20 Jun 2010 11:23:02 GMT [email protected] wrote:
Said to be a post-2.6.34 regression caused by the net namespaces changes.
There's a massive thread there and I didn't work out whether it's already been fixed?
The fix is pending.
The cause is understood. Roughly we started caring about something being true in all cases and there are bugs where it is not.
The patch below is less pretty than I would like, but it is trivial and it fixes the problem in all cases, and in exactly the cases that are broken today.
I have received no feedback since I posted it. Right now there are no viable alternatives on the table for the 2.6.35 timeframe. The only suggested alternatives is to rewrite the affected drivers sysfs support and fix the driver core so that rewrite is actually possible.
Quoted 1 line
From
ada3a8448d376000e8cf586f0e4062e2f578c80fMon Sep 17 00:00:00 2001
From: Eric W. Biederman <[email protected]>
Date: Sat, 19 Jun 2010 22:58:39 -0700
Subject: [PATCH] Driver-core: Always create network class directories in get_device_parent.
In get_device_parent there was added check to not add a class directory when a class device was put under another class device. The check was put in place as a just in case measure to not break old userspace if any existing code happened to depend on it. Devices in the input subsystem are affected by this code path so there is a reasonable chance that some piece of user space will break if we just remove this cludge.
At the same time there are at least two network drivers that have potential unnecessary namespace conflicts because class directories have not been created for their network devices.
With the introduction of tagged sysfs directories for properly handling network namespace support this omission in creating the class directories went from a bad thing in terms of namespace pollution, to actually breaking device_remove.
Currently there are two reported network device drivers that break because the class directory was not created by the device layer. The usb bnep driver and the mac80211_hwsim driver.
Every solution proposed changes the sysfs layout for the affected devices, and thus has the potential to break userspace.
Since we are changing the sysfs layout anyway, and since we are now talking about several devices all with the same problem, all caused by the same over convservative bit of code. Let's fix the device layer for network devices.
--- drivers/base/core.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/base/core.c +1 -1
@@ -673,7 +673,7 @@ static struct kobject *get_device_parent(struct device *dev,*/ if (parent ==NULL ) parent_kobj =virtual_device_parent (dev); -else if (parent->class) +else if (parent->class && (strcmp (dev->class->name,"net" ) !=0 ))return &parent->kobj;else parent_kobj = &parent->kobj;
Signature · 1 line
1.6.5.2.143.g8cc62
Quoted 17 lines · Eric W. Biederman
Quoted 9 lines · Andrew Morton
Quoted 1 line · On Sun, 20 Jun 2010 11:23:02 GMT [email protected] wrote:
Said to be a post-2.6.34 regression caused by the net namespaces changes.
There's a massive thread there and I didn't work out whether it's already been fixed?
The fix is pending.
The cause is understood. Roughly we started caring about something being true in all cases and there are bugs where it is not.
Quoted 8 lines
The patch below is less pretty than I would like, but it is trivial and it fixes the problem in all cases, and in exactly the cases that are broken today.
I have received no feedback since I posted it. Right now there are no viable alternatives on the table for the 2.6.35 timeframe. The only suggested alternatives is to rewrite the affected drivers sysfs support and fix the driver core so that rewrite is actually possible.
I _really_ do not like this patch.
The correct thing is to fix the modules that are affected, and that's only the wireless testing module, right? Or is there something else?
And if that code is properly converted to a bus, what needs to change in the driver core?
thanks,
greg k-h
Quoted 7 lines · Greg KH
I _really_ do not like this patch.
The correct thing is to fix the modules that are affected, and that's only the wireless testing module, right? Or is there something else?
And if that code is properly converted to a bus, what needs to change in the driver core?
The usb bnep driver.
Eric
Quoted 11 lines · Eric W. Biederman
Quoted 7 lines · Greg KH
I _really_ do not like this patch.
The correct thing is to fix the modules that are affected, and that's only the wireless testing module, right? Or is there something else?
And if that code is properly converted to a bus, what needs to change in the driver core?
The usb bnep driver.
Odd, what is bnep doing differently here from all other network drivers? Is it trying to stack the bluetooth class in the middle somehow? Shouldn't this be easy to fix up in the driver itself?
thanks,
greg k-h
Quoted 8 lines · Greg KH
Quoted 7 lines · Eric W. Biederman
Quoted 5 lines · Greg KH
I _really_ do not like this patch.
The correct thing is to fix the modules that are affected, and that's only the wireless testing module, right? Or is there something else?
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
And if that code is properly converted to a bus, what needs to change in the driver core?
The usb bnep driver.
Odd, what is bnep doing differently here from all other network drivers? Is it trying to stack the bluetooth class in the middle somehow? Shouldn't this be easy to fix up in the driver itself?
The cause of all of the failures is a class device with a class device parent, instead of a bus device parent. Which causes the net/ directory not to be created for us to put the network devices under.
I don't have a clue about how that driver works, and I don't know how many other drivers that do something strange like this are out there lurking.
I have not been involved previously in any changes from class device to bus device so I don't have a clue how difficult it would. All I know is that with the mac80211_hwsim driver we had first had two people who really knew what they were doing and it was hard enough there still isn't a working conversion away from class devices.
Eric
Quoted 23 lines · Eric W. Biederman
Quoted 8 lines · Greg KH
Quoted 7 lines · Eric W. Biederman
Quoted 5 lines · Greg KH
I _really_ do not like this patch.
The correct thing is to fix the modules that are affected, and that's only the wireless testing module, right? Or is there something else?
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
And if that code is properly converted to a bus, what needs to change in the driver core?
The usb bnep driver.
Odd, what is bnep doing differently here from all other network drivers? Is it trying to stack the bluetooth class in the middle somehow? Shouldn't this be easy to fix up in the driver itself?
The cause of all of the failures is a class device with a class device parent, instead of a bus device parent. Which causes the
net/directory not to be created for us to put the network devices under.
Quoted 8 lines
I don't have a clue about how that driver works, and I don't know how many other drivers that do something strange like this are out there lurking.
I have not been involved previously in any changes from class device to bus device so I don't have a clue how difficult it would. All I know is that with the
mac80211_hwsimdriver we had first had two people who really knew what they were doing and it was hard enough there still isn't a working conversion away from class devices.
But what caused the breakage here?
Does this only show up if you enable network namespaces? Or is it a problem with the "normal" kernel configuration of no network namespaces?
The only thing that changed here was your network namespace work, what caused this problem to show up? Was it bisectable down to a single patch?
Classes on top of classes should never have originally worked, I guess we just let them slide by accident, and we can go and fix them up as they are found. But for now, for .35, it would be good to find the root cause of the problem here. It might be as simple as putting a CONFIG_BROKEN on network namespaces until we get this working, right?
thanks,
greg k-h
PATCH 1/1 sysfs: Don't allow the creation of symlinks we can't remove
Recently my tagged sysfs support revealed a flaw in the device core that a few rare drivers are running into such that we don't always put network devices in a class subdirectory named net/.
Since we are not creating the class directory the network devices wind up in a non-tagged directory, but the symlinks to the network devices from /sys/class/net are in a tagged directory. All of which works until we go to remove or rename the symlink. When we remove or rename a symlink we look in the namespace of the target of the symlink. Since the target of the symlink is in a non-tagged sysfs directory we don't have a namespace to look in, and we fail to remove the symlink.
Detect this problem up front and simply don't create symlinks we won't be able to remove later. This prevents symlink leakage and fails in a much clearer and more understandable way.
--- fs/sysfs/symlink.c | 23 ++++++++++++++++++----- 1 files changed, 18 insertions(+), 5 deletions(-)
fs/sysfs/symlink.c +18 -5
@@ -28,6 +28,7 @@ static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,struct sysfs_dirent *target_sd =NULL ;struct sysfs_dirent *sd =NULL ;struct sysfs_addrm_cxt acxt; +enum kobj_ns_type ns_type;int error;BUG_ON (!name);
@@ -58,16 +59,28 @@ static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,if (!sd) goto out_put; -if (sysfs_ns_type (parent_sd)) + ns_type =sysfs_ns_type (parent_sd); +if (ns_type) sd->s_ns = target->ktype->namespace (target); sd->s_symlink.target_sd = target_sd; target_sd =NULL ;/* reference is now owned by the symlink */ sysfs_addrm_start (&acxt, parent_sd); -if (warn) - error =sysfs_add_one (&acxt, sd); -else - error =__sysfs_add_one (&acxt, sd); +/* Symlinks must be between directories with the same ns_type */ +if (ns_type ==sysfs_ns_type (sd->s_symlink.target_sd->s_parent)) { +if (warn) + error =sysfs_add_one (&acxt, sd); +else + error =__sysfs_add_one (&acxt, sd); + }else { + error = -EINVAL ; +WARN (1 ,KERN_WARNING +"sysfs: symlink across ns_types %s/%s -> %s/%s\n" , + parent_sd->s_name, + sd->s_name, + sd->s_symlink.target_sd->s_parent->s_name, + sd->s_symlink.target_sd->s_name); + }sysfs_addrm_finish (&acxt);if (error)
Signature · 1 line
1.6.5.2.143.g8cc62
RFC PATCH 1/1 mac80211_hwsim: No parent is better than an illegimate one.
Don't call SET_IEEE80211_DEV. This weakens the connections between the phy files in sysfs slightly but otherwise it makes the driver work in the face of tagged sysfs support.
---
Johannes does this change look usable?
drivers/net/wireless/mac80211_hwsim.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
drivers/net/wireless/mac80211_hwsim.c +0 -1
@@ -1282,7 +1282,6 @@ static int __init init_mac80211_hwsim(void) } data->dev->driver = &mac80211_hwsim_driver; -SET_IEEE80211_DEV (hw, data->dev); addr[3 ] = i >>8 ; addr[4 ] = i;memcpy (data->addresses[0 ].addr, addr,ETH_ALEN );
Signature · 1 line
1.6.5.2.143.g8cc62
Johannes Berg
maintainerRe: [RFC][PATCH] mac80211_hwsim: No parent is better than an illegimate one.
Quoted 8 lines · Eric W. Biederman
Don't call
SET_IEEE80211_DEV. This weakens the connections between the phy files in sysfs slightly but otherwise it makes the driver work in the face of tagged sysfs support.Signed-off-by Eric W. Biederman <[email protected]>---
Johannes does this change look usable?
It'll probably work, but anything in userspace relying on it will fail, and you'll have some unused virtual devices in sysfs? Not too keen on it, but if we really can't fix it ....
johannes
Quoted 15 lines
drivers/net/wireless/mac80211_hwsim.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)drivers/net/wireless/mac80211_hwsim.c +0
-1@@ -1282,7 +1282,6 @@ static int __init init_mac80211_hwsim(void) } data->dev->driver = &mac80211_hwsim_driver; -SET_IEEE80211_DEV (hw, data->dev); addr[3 ] = i >>8 ; addr[4 ] = i;memcpy (data->addresses[0 ].addr, addr,ETH_ALEN );
Re: [RFC][PATCH] mac80211_hwsim: No parent is better than an illegimate one.
Quoted 5 lines · Johannes Berg
Quoted 4 lines · Eric W. Biederman
Don't call
SET_IEEE80211_DEV. This weakens the connections between the phy files in sysfs slightly but otherwise it makes the driver work in the face of tagged sysfs support.
Quoted 7 lines
Quoted 4 lines
Signed-off-by Eric W. Biederman <[email protected]>---
Johannes does this change look usable?
It'll probably work, but anything in userspace relying on it will fail, and you'll have some unused virtual devices in sysfs?
Well anything in userspace that isn't mac80211_hwsim specific will likely already fail because the format of the link is different for mac80211_hwsim then for every other wireless device.
Quoted 1 line
Not too keen on it, but if we really can't fix it ....
Perhaps. I'm not familiar enough with the details of the device layer to promise an eta on anything like that.
Eric
Johannes Berg
maintainerRe: [RFC][PATCH] mac80211_hwsim: No parent is better than an illegimate one.
Quoted 6 lines · Eric W. Biederman
Quoted 2 lines
It'll probably work, but anything in userspace relying on it will fail, and you'll have some unused virtual devices in sysfs?
Well anything in userspace that isn't
mac80211_hwsimspecific will likely already fail because the format of the link is different formac80211_hwsimthen for every other wireless device.
Well, I seem to have a vague memory of network-manager for example requiring the link but not really caring much what it is, but I might remember something else that wasn't hwsim related.
johannes
Re: [PATCH] sysfs: Don't allow the creation of symlinks we can't remove
In reply to Eric W. BiedermanQuoted 13 lines · Eric W. Biederman
Recently my tagged sysfs support revealed a flaw in the device core that a few rare drivers are running into such that we don't always put network devices in a class subdirectory named
net/.Since we are not creating the class directory the network devices wind up in a non-tagged directory, but the symlinks to the network devices from
/sys/class/netare in a tagged directory. All of which works until we go to remove or rename the symlink. When we remove or rename a symlink we look in the namespace of the target of the symlink. Since the target of the symlink is in a non-tagged sysfs directory we don't have a namespace to look in, and we fail to remove the symlink.
Quoted 3 lines
Detect this problem up front and simply don't create symlinks we won't be able to remove later. This prevents symlink leakage and fails in a much clearer and more understandable way.
With this patch, how does the existing code fail as the drivers aren't fixed up?
I like this change, just worried it will cause problems if it gets into .35, without your RFC patch. Will it?
thanks,
greg k-h
Re: [PATCH] sysfs: Don't allow the creation of symlinks we can't remove
Quoted 5 lines · Greg KH
With this patch, how does the existing code fail as the drivers aren't fixed up?
I like this change, just worried it will cause problems if it gets into .35, without your RFC patch. Will it?
It don't expect this patch to be worse than where we are current at. Network devices are renamed, and come and go enough that both the mac80211_hwsim and the bnep driver are currently unusable with a failure only the rename and remove.
This patch simply moves the failure into creation where we are a little more prepared to deal with problems, and this patch is limited to mac80211_hwsim, bnep, and any hypothetically undiscovered other network devices that have the same problem.
mac80211_hwsim with just this patch becomes somewhat usable as it's primary network device gets registered and the module can be loaded and unloaded. It just doesn't create the wlan0 and wlan1 interfaces for the wifi interfaces.
On a slightly unrelated note, what got us on trying to convert mac80211_hwsim was that everything happens in a single module which makes avoiding cleanup races hard. I am hoping bnep is structured enough differently so we can convert it to a bus device using the existing infrastructure. I haven't figured out bnep yet though ;(
Eric
Re: [PATCH] sysfs: Don't allow the creation of symlinks we can't remove
Quoted 13 lines · Eric W. Biederman
Quoted 5 lines · Greg KH
With this patch, how does the existing code fail as the drivers aren't fixed up?
I like this change, just worried it will cause problems if it gets into .35, without your RFC patch. Will it?
It don't expect this patch to be worse than where we are current at. Network devices are renamed, and come and go enough that both the
mac80211_hwsimand the bnep driver are currently unusable with a failure only the rename and remove.
Quoted 8 lines
This patch simply moves the failure into creation where we are a little more prepared to deal with problems, and this patch is limited to
mac80211_hwsim, bnep, and any hypothetically undiscovered other network devices that have the same problem.
mac80211_hwsimwith just this patch becomes somewhat usable as it's primary network device gets registered and the module can be loaded and unloaded. It just doesn't create the wlan0 and wlan1 interfaces for the wifi interfaces.
Fair enough, I've commited it now, let's see what happens :)
Quoted 5 lines
On a slightly unrelated note, what got us on trying to convert
mac80211_hwsimwas that everything happens in a single module which makes avoiding cleanup races hard. I am hoping bnep is structured enough differently so we can convert it to a bus device using the existing infrastructure. I haven't figured out bnep yet though ;(
Yeah, good luck with that...
greg k-h
Re: [PATCH] sysfs: Don't allow the creation of symlinks we can't remove
Quoted 19 lines · On Thu, 8 Jul 2010 16:06:01 -0700 Greg KH <[email protected]> wrote:
Quoted 18 lines · Eric W. Biederman
Quoted 5 lines · Greg KH
With this patch, how does the existing code fail as the drivers aren't fixed up?
I like this change, just worried it will cause problems if it gets into .35, without your RFC patch. Will it?
It don't expect this patch to be worse than where we are current at. Network devices are renamed, and come and go enough that both the
mac80211_hwsimand the bnep driver are currently unusable with a failure only the rename and remove.This patch simply moves the failure into creation where we are a little more prepared to deal with problems, and this patch is limited to
mac80211_hwsim, bnep, and any hypothetically undiscovered other network devices that have the same problem.
Quoted 5 lines
Quoted 3 lines
mac80211_hwsimwith just this patch becomes somewhat usable as it's primary network device gets registered and the module can be loaded and unloaded. It just doesn't create the wlan0 and wlan1 interfaces for the wifi interfaces.Fair enough, I've commited it now, let's see what happens :)
geethanks!
On the FC6 test box I have no networking.
akpm2:/usr/src/25> grep E1000 .configCONFIG_E1000=yCONFIG_E1000E=y
calling e1000_init_module+0x0/0x84 @ 1
e1000: Intel(R) PRO/1000 Network Driver - version 7.3.21-k6-NAPI
e1000: Copyright (c) 1999-2006 Intel Corporation. initcall e1000_init_module+0x0/0x84 returned 0 after 252 usecs calling e1000_init_module+0x0/0x3e @ 1
e1000e: Intel(R) PRO/1000 Network Driver - 1.2.7-k2
e1000e: Copyright (c) 1999 - 2010 Intel Corporation. e1000e 0000:05:00.0: PCI INT A -> GSI 18 (level, low) -> IRQ 18 e1000e 0000:05:00.0: setting latency timer to 64 e1000e 0000:05:00.0: irq 74 for MSI/MSI-X
------------[ cut here ]------------ WARNING: at fs/sysfs/symlink.c:82 sysfs_do_create_link+0x140/0x19a() sysfs: symlink across ns_types 0000:05:00.0/net:eth0 -> net/eth0 Modules linked in: Pid: 1, comm: swapper Not tainted 2.6.35-rc5 #1 Call Trace: [<ffffffff81111b21>] ? sysfs_do_create_link+0x140/0x19a [<ffffffff81036283>] warn_slowpath_common+0x7e/0x97 [<ffffffff8103631c>] warn_slowpath_fmt+0x69/0x6b [<ffffffff81055ffb>] ? debug_mutex_free_waiter+0x4c/0x50 [<ffffffff811107f3>] ? sysfs_addrm_start+0x25/0x27 [<ffffffff811107f3>] ? sysfs_addrm_start+0x25/0x27 [<ffffffff81111b21>] sysfs_do_create_link+0x140/0x19a [<ffffffff810595b2>] ? trace_hardirqs_on+0xd/0xf [<ffffffff81111b96>] sysfs_create_link+0xe/0x10 [<ffffffff8121451d>] device_add+0x299/0x5b2 [<ffffffff812195f4>] ? device_pm_init+0x30/0x3f [<ffffffff812edea0>] netdev_register_kobject+0x79/0x159 [<ffffffff812dfd00>] register_netdevice+0x272/0x31a [<ffffffff812dfde2>] register_netdev+0x3a/0x48 [<ffffffff813677a8>] e1000_probe+0x956/0xb05 [<ffffffff81375267>] ? _raw_spin_unlock+0x23/0x28 [<ffffffff8119916c>] local_pci_probe+0x12/0x16 [<ffffffff811995bc>] pci_device_probe+0xca/0xf4 [<ffffffff8121662e>] ? driver_sysfs_add+0x4a/0x70 [<ffffffff81216879>] driver_probe_device+0xa1/0x137 [<ffffffff81216968>] __driver_attach+0x59/0x7d [<ffffffff8121690f>] ? __driver_attach+0x0/0x7d [<ffffffff8121560c>] bus_for_each_dev+0x4f/0x80 [<ffffffff81216449>] driver_attach+0x1c/0x1e [<ffffffff81216190>] bus_add_driver+0xb4/0x206 [<ffffffff81216d87>] driver_register+0x92/0xff [<ffffffff81198cea>] __pci_register_driver+0x60/0xbd [<ffffffff81a159c8>] ? e1000_init_module+0x0/0x3e [<ffffffff81a15a04>] e1000_init_module+0x3c/0x3e [<ffffffff810002cb>] do_one_initcall+0x52/0x13f [<ffffffff819edcc6>] kernel_init+0x140/0x1c9 [<ffffffff81003814>] kernel_thread_helper+0x4/0x10 [<ffffffff81375cfc>] ? restore_args+0x0/0x30 [<ffffffff819edb86>] ? kernel_init+0x0/0x1c9 [<ffffffff81003810>] ? kernel_thread_helper+0x0/0x10 ---[ end trace 948c9041178f15c8 ]---
Re: [PATCH] sysfs: Don't allow the creation of symlinks we can't remove
Quoted 26 lines · Andrew Morton
Quoted 23 lines · On Thu, 8 Jul 2010 16:06:01 -0700 Greg KH <[email protected]> wrote:
Quoted 21 lines · Eric W. Biederman
Quoted 5 lines · Greg KH
With this patch, how does the existing code fail as the drivers aren't fixed up?
I like this change, just worried it will cause problems if it gets into .35, without your RFC patch. Will it?
It don't expect this patch to be worse than where we are current at. Network devices are renamed, and come and go enough that both the
mac80211_hwsimand the bnep driver are currently unusable with a failure only the rename and remove.This patch simply moves the failure into creation where we are a little more prepared to deal with problems, and this patch is limited to
mac80211_hwsim, bnep, and any hypothetically undiscovered other network devices that have the same problem.
mac80211_hwsimwith just this patch becomes somewhat usable as it's primary network device gets registered and the module can be loaded and unloaded. It just doesn't create the wlan0 and wlan1 interfaces for the wifi interfaces.
Quoted 5 lines
Quoted 1 line
Fair enough, I've commited it now, let's see what happens :)
geethanks!
On the FC6 test box I have no networking.
Ick.
Eric, any ideas?
cover Support untagged symlinks to tagged directories.
Quoted 14 lines · Greg KH
Quoted 13 lines · Andrew Morton
Quoted 9 lines · On Thu, 8 Jul 2010 16:06:01 -0700 Greg KH <[email protected]> wrote:
Quoted 8 lines · Eric W. Biederman
Quoted 5 lines · Greg KH
With this patch, how does the existing code fail as the drivers aren't fixed up?
I like this change, just worried it will cause problems if it gets into .35, without your RFC patch. Will it?
Quoted 7 lines
Quoted 3 lines
geethanks!
On the FC6 test box I have no networking.
Ick.
Eric, any ideas?
Yes. I just found some time to test my fixes and things are looking good. It really is just two one line fixes.
On the other part of this debug with SYSFS_DEPRECATED enabled it with mac80211_hwsim drivers works fine no problems. I expect the bnep driver will also be fine.
What is affecting those two is arguably a bug in the non-deprecated sysfs mode.
Regardless here are my fixes. I have split this into a patch for the warning and a patch for sysfs_delete_link. Because at least the sysfs_delete_link code needs to make into 2.6.35 if we can.
Eric
PATCH 1/2 sysfs: sysfs_delete_link handle symlinks from untagged to tagged directories.
This happens for network devices when SYSFS_DEPRECATED is enabled.
--- fs/sysfs/symlink.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
fs/sysfs/symlink.c +1 -1
@@ -135,7 +135,7 @@ void sysfs_delete_link(struct kobject *kobj, struct kobject *targ, {const void *ns =NULL ;spin_lock (&sysfs_assoc_lock); -if (targ->sd) +if (targ->sd &&sysfs_ns_type (kobj->sd)) ns = targ->sd->s_ns;spin_unlock (&sysfs_assoc_lock);sysfs_hash_and_remove (kobj->sd, ns, name);
Signature · 1 line
1.6.5.2.143.g8cc62
PATCH 2/2 sysfs: allow creating symlinks from untagged to tagged directories
Supporting symlinks from untagged to tagged directories is reasonable, and needed to support CONFIG_SYSFS_DEPRECATED. So don't fail a prior allowing that case to work.
--- fs/sysfs/symlink.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
fs/sysfs/symlink.c +2 -1
@@ -67,7 +67,8 @@ static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,sysfs_addrm_start (&acxt, parent_sd);/* Symlinks must be between directories with the same ns_type */ -if (ns_type ==sysfs_ns_type (sd->s_symlink.target_sd->s_parent)) { +if (!ns_type || + (ns_type ==sysfs_ns_type (sd->s_symlink.target_sd->s_parent))) {if (warn) error =sysfs_add_one (&acxt, sd);else
Signature · 1 line
1.6.5.2.143.g8cc62
Re: [PATCH 0/2] Support untagged symlinks to tagged directories.
In reply to Eric W. BiedermanQuoted 32 lines · Eric W. Biederman
Quoted 14 lines · Greg KH
Quoted 13 lines · Andrew Morton
Quoted 9 lines · On Thu, 8 Jul 2010 16:06:01 -0700 Greg KH <[email protected]> wrote:
Quoted 8 lines · Eric W. Biederman
Quoted 5 lines · Greg KH
With this patch, how does the existing code fail as the drivers aren't fixed up?
I like this change, just worried it will cause problems if it gets into .35, without your RFC patch. Will it?
Quoted 7 lines
Quoted 3 lines
geethanks!
On the FC6 test box I have no networking.
Ick.
Eric, any ideas?
Yes. I just found some time to test my fixes and things are looking good. It really is just two one line fixes.
On the other part of this debug with
SYSFS_DEPRECATEDenabled it withmac80211_hwsimdrivers works fine no problems. I expect the bnep driver will also be fine.
Quoted 6 lines
What is affecting those two is arguably a bug in the non-deprecated sysfs mode.
Regardless here are my fixes. I have split this into a patch for the warning and a patch for
sysfs_delete_link. Because at least thesysfs_delete_linkcode needs to make into 2.6.35 if we can.
Do these patches obsolete the existing one I have in my tree entitled:
Subject: sysfs: Don't allow the creation of symlinks we can't remove
or are they on top of that one?
thanks,
greg k-h
Re: [PATCH 0/2] Support untagged symlinks to tagged directories.
Quoted 36 lines · Greg KH
Quoted 35 lines · Eric W. Biederman
Quoted 14 lines · Greg KH
Quoted 13 lines · Andrew Morton
Quoted 9 lines · On Thu, 8 Jul 2010 16:06:01 -0700 Greg KH <[email protected]> wrote:
Quoted 8 lines · Eric W. Biederman
Quoted 5 lines · Greg KH
With this patch, how does the existing code fail as the drivers aren't fixed up?
I like this change, just worried it will cause problems if it gets into .35, without your RFC patch. Will it?
Quoted 7 lines
Quoted 3 lines
geethanks!
On the FC6 test box I have no networking.
Ick.
Eric, any ideas?
Yes. I just found some time to test my fixes and things are looking good. It really is just two one line fixes.
On the other part of this debug with
SYSFS_DEPRECATEDenabled it withmac80211_hwsimdrivers works fine no problems. I expect the bnep driver will also be fine.What is affecting those two is arguably a bug in the non-deprecated sysfs mode.
Quoted 7 lines
Quoted 3 lines
Regardless here are my fixes. I have split this into a patch for the warning and a patch for
sysfs_delete_link. Because at least thesysfs_delete_linkcode needs to make into 2.6.35 if we can.Do these patches obsolete the existing one I have in my tree entitled:
Subject: sysfs: Don't allow the creation of symlinks we can't remove
or are they on top of that one?
No. These patches are incremental on top of what you have.
In particular patch 1 fixes what is in 2.6.35-rc5, and patch 2 fixes the additional check in:
'sysfs: Don't allow the creation of symlinks we can't remove'
Eric
Re: [PATCH 0/2] Support untagged symlinks to tagged directories.
Quoted 46 lines · Eric W. Biederman
Quoted 36 lines · Greg KH
Quoted 35 lines · Eric W. Biederman
Quoted 14 lines · Greg KH
Quoted 13 lines · Andrew Morton
Quoted 9 lines · On Thu, 8 Jul 2010 16:06:01 -0700 Greg KH <[email protected]> wrote:
Quoted 8 lines · Eric W. Biederman
Quoted 5 lines · Greg KH
With this patch, how does the existing code fail as the drivers aren't fixed up?
I like this change, just worried it will cause problems if it gets into .35, without your RFC patch. Will it?
Quoted 7 lines
Quoted 3 lines
geethanks!
On the FC6 test box I have no networking.
Ick.
Eric, any ideas?
Yes. I just found some time to test my fixes and things are looking good. It really is just two one line fixes.
On the other part of this debug with
SYSFS_DEPRECATEDenabled it withmac80211_hwsimdrivers works fine no problems. I expect the bnep driver will also be fine.What is affecting those two is arguably a bug in the non-deprecated sysfs mode.
Quoted 7 lines
Quoted 3 lines
Regardless here are my fixes. I have split this into a patch for the warning and a patch for
sysfs_delete_link. Because at least thesysfs_delete_linkcode needs to make into 2.6.35 if we can.Do these patches obsolete the existing one I have in my tree entitled:
Subject: sysfs: Don't allow the creation of symlinks we can't remove
or are they on top of that one?
Quoted 5 lines
No. These patches are incremental on top of what you have.
In particular patch 1 fixes what is in 2.6.35-rc5, and patch 2 fixes the additional check in:
'sysfs: Don't allow the creation of symlinks we can't remove'
Ok, thanks, I've now added them to my tree and will send them to Linus in a day or so.
greg k-h
PATCH 1/1 Driver-core: Fix bluetooth network device rename regression
In reply to Eric W. BiedermanWith CONFIG_SYSFS_DEPRECATED_V2 enabled I can rename any network device anything as long as the new name does not conflict with another network device.
With CONFIG_SYSFS_DEPRECATED_V2 disabled without this fix bluetooth benp devices, and the mac80211_hwsim driver can not be renamed to any arbitrary name that happens to conflict with any other name that is used in their parent devices directory.
The device model usage of the bluetooth bnep driver has not changed since
the current device model sysfs laytout was introduced. Making this a latent
and very annoying regression.
This regression was reported at the time it was introduced and apprently a few cases were missed by:
commit 864062457a2e444227bd368ca5f2a2b740de604f
Author: Kay Sievers <[email protected]> Date: Wed Mar 14 03:25:56 2007 +0100
driver core: fix namespace issue with devices assigned to classes
- uses a kset in "struct class" to keep track of all directories belonging to this class
- merges with the /sys/devices/virtual logic.
- removes the namespace-dir if the last member of that class leaves the directory.
There may be locking or refcounting fixes left, I stopped when it seemed to work with network and sound modules. :)
From: Kay Sievers <[email protected]>
That bug fix had a completely undocumented and apparently deliberate omission where it does not apply in some cases. Those omitted cases cover the bluetooth network driver.
What makes this regression a serious issue now is the introduction of network namespace support in sysfs took this from a mild bug to complete driver non-function.
Several reasons have been put forward not to use this one line bug fix in the driver core.
- We don't have special cases in the driver core.
This is non-sense we currently have special cases for block devices scattered all throughout the driver core.
- The driver is at fault.
The bluetooth driver's driver core usage predates the introduction of the current sysfs layout. It has been 3 years and no one has bothered to change the driver in all of this time.
If this was really a driver issue that someone cared about and not simply an academic issue the driver should have been fixed long since.
I offer these reasons to make the change.
- There is not enough information available for sysfs to support rename or delete of symlinks when only the source directory but not the destination directory is tagged.
- All of the proposals put forth will change the sysfs layout slightly in one way or another.
- A network device special case makes sense as network devices are unique in placing a user choosen name sysfs.
- The driver that are affected are different in sysfs from all other network devices which likely already consuses any userspace software that cares about the exact device layout.
- The change is one line that is obviously correct and has no chance of affecting anything that it is not a network device.
- Real users have hit this problem and reported this bug and those users deserve drivers that work.
The fix is a trivial change to get_device_parent to always create the network class directory in any parent of a network device, Ignoring the incorrect, non-obvious and esoteric rules that the driver core is trying to impose on the creation of class directories.
Ideally I would remove those crazy special cases get_device_parent for all devices but in testing it was observed there are other devices such as the input layer that don't create the class directories today, and applying the change broadly is likely to break something in userspace and there is no need to take that risk.
--- drivers/base/core.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/base/core.c +1 -1
@@ -673,7 +673,7 @@ static struct kobject *get_device_parent(struct device *dev,*/ if (parent ==NULL ) parent_kobj =virtual_device_parent (dev); -else if (parent->class) +else if (parent->class && (strcmp (dev->class->name,"net" ) !=0 ))return &parent->kobj;else parent_kobj = &parent->kobj;
Signature · 1 line
1.6.5.2.143.g8cc62
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 9 lines · Eric W. Biederman
With
CONFIG_SYSFS_DEPRECATED_V2enabled I can rename any network device anything as long as the new name does not conflict with another network device.With
CONFIG_SYSFS_DEPRECATED_V2disabled without this fix bluetooth benp devices, and themac80211_hwsimdriver can not be renamed to any arbitrary name that happens to conflict with any other name that is used in their parent devices directory.
This is true for all devices, that their children can not carry names of existing attributes or directories of the parent. These drivers manage the parent-child relation their own and know these limitations very well, because they have created the conflicing names themselves. The class glue directories which separate these namespaces are there to prevent unknown clashes, not clashes originating from the same subsystem.
The real fix is that the drivers should not try to stack classes, but use buses. This is and never was supported by the core, especially not for clashing names.
drivers/base/core.c static struct kobject *get_device_parent(struct device *dev, · line 676
Quoted 8 lines
drivers/base/core.c +1
-1@@ -673,7 +673,7 @@ static struct kobject *get_device_parent(struct device *dev,*/ if (parent == NULL) parent_kobj = virtual_device_parent(dev); - else if (parent->class) + else if (parent->class && (strcmp(dev->class->name, "net") != 0))
Subsystems specific code must not leak into core code. We will never be able to get rid of these hacks. As mentioned in earlier mails, it's just plain wrong to do anything like this. It makes a specific subsystem behave different from all others, just to fix some broken drivers to work with the newly introduced network sysfs namespaces.
Since the issue is not a regression, and not even a bug in the core, it should not be done this way for mainline.
Please try to fix these drivers instead, or mark the broken for namespaces, if nobody can fix them right now.
Thanks,
Kay
Johannes Berg
maintainerRe: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 2 lines · Kay Sievers
Please try to fix these drivers instead, or mark the broken for namespaces, if nobody can fix them right now.
We've tried. Nobody, including you, has been able to suggest how to fix it. And it's not just broken with network namespaces enabled either, as Eric explained. I really don't see why you keep asking us to fix it when clearly we cannot -- even you don't know how and you certainly have more insight into the device model than we do.
johannes
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 7 lines · Johannes Berg
Quoted 3 lines · Kay Sievers
Please try to fix these drivers instead, or mark the broken for namespaces, if nobody can fix them right now.
We've tried. Nobody, including you, has been able to suggest how to fix it.
I did, and several times. Here are the options again:
- Split the driver in two modules, so the auto-cleanup of the netdev can be done by the second module when the device is force-unloaded without taking any references to the code while in use (netdev specif behavior).
- Move the device cleanup code somehow in the core by adding proper functions to bus devices, similar to the completely mis-used device_create() function for class devices. This would also be a proper fix for the weird driver core use.
- Do not allow to force-unload the module while the netdev is in use. You would need some "destruct" command for the device then, which removes the netdev, and to be able to unload the module.
Quoted 1 line
And it's not just broken with network namespaces enabled either, as
So what's the problem without the sysfs ns then? I didn't hear any the last couple of years.
Quoted 3 lines
Eric explained. I really don't see why you keep asking us to fix it when clearly we cannot -- even you don't know how and you certainly have more insight into the device model than we do.
Sure, and I ask again to fix the drivers, instead of sneaking-in dirty hacks into the core, just by calling an expected behavior a "regression". This is not a core bug, and should not be worked around that way in the core.
If there is a new requirement for the core (like possibly point 2 above), we can surely look into making this happen.
We can not add lists of individual subsystems to generic core functions to work around broken drivers. I hope you will understand that.
Thanks,
Kay
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
In reply to Kay SieversKay Sievers <[email protected]> writes:
Kay you are full of it. My argument is that this a clear case where aiming for perfection is the enemy of the good.
Quoted 4 lines
Quoted 4 lines
drivers/base/core.c +0
-0@@ -673,7 +673,7 @@ static struct kobject *get_device_parent(struct device *dev,*/
Quoted 8 lines
drivers/base/core.c static struct kobject *get_device_parent(struct device *dev, · line 676
Quoted 4 lines
drivers/base/core.c +1
-1if (parent == NULL) parent_kobj = virtual_device_parent(dev); - else if (parent->class) + else if (parent->class && (strcmp(dev->class->name, "net") != 0))Since the issue is not a regression, and not even a bug in the core, it should not be done this way for mainline.
Yes the issue is a namespace regression and it has been a regression for the bluetooth network drivers since CONFIG_SYSFS_DEPRECATED was introduced. "ip link set name blarg" Where blarg is an attribute of the bluteooth parent device fails only with CONFIG_SYSFS_DEPRECATED disabled.
I reported that regression for the majority of the network devices and the class directories were introduced to avoid this problem, except convoluted logic of when to create a class directory fails to cover a few flavors of network drivers.
For the mac80211_hwsim driver I don't care. That isn't real hardware and it is only interesting for testing. For the bluetooth network driver that is real hardware that needs to be made to work.
Quoted 2 lines
Please try to fix these drivers instead, or mark the broken for namespaces, if nobody can fix them right now.
Let me get this straight. You believe a non-trival non-obvious complete rewrite of entire subsystems after rc1 is better than a one line bug fix that let's real hardware work for real people?
You have had 3 years to do better a better job with the bluetooth driver.
Furthermore we fixed the rest of this regression in the core I don't see why we can fix the entirety of it this namespace regression in the core. Especially since it is an obviously correct one line change.
Eric
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 6 lines · Eric W. Biederman
Kay Sievers <[email protected]> writes:
Let me get this straight. You believe a non-trival non-obvious complete rewrite of entire subsystems after rc1 is better than a one line bug fix that let's real hardware work for real people?
Yes. The drivers are broken in its core use pattern, and enabling a (non-common) new feature exposes this. Users should just not enable the new feature until the drivers, they might depend on, are fixed properly. That's the rule for all other kernel development too, and I don't see what's different here. It's not a regression, and not a bug in the core, which needs fixing.
I don't see what would justify subsystem-specific hacks in the core, which we will never be able to remove. Your patch exposes sysfs device layout for a single subsystem in an unusual manner, and we can not change that later. We need to avoid such special cases for all reasons.
Quoted 2 lines
You have had 3 years to do better a better job with the bluetooth driver.
I don't know of any real-world problem here.
Quoted 3 lines
Furthermore we fixed the rest of this regression in the core I don't see why we can fix the entirety of it this namespace regression in the core. Especially since it is an obviously correct one line change.
Oh, no. It's obviously wrong to add specific lists of subsystems to generic core code to fix individual drivers. It even implements its own sysfs layout rules for a single subsystem.
Please help fixing the drivers, or improving the core where needed, to support these drivers better, but don't try to throw quick and dirty and wrong fixes at us. Sysfs is already hard to understand and more exceptions like this, and subsystem-specific layout rules would seriously be the wrong direction.
Thanks,
Kay
Johannes Berg
maintainerRe: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 2 lines · Kay Sievers
Yes. The drivers are broken in its core use pattern, and enabling a (non-common) new feature exposes this.
You've been saying this for a long time now, but I still don't buy it.
This stuff WORKED before. Now, years later, you're saying that it has a broken use pattern, and needs to be fixed. That's a pipe dream. In the real world, you can't deliberately break things because years later you decide that the use pattern that was working fine before is broken.
johannes
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 11 lines · Johannes Berg
Quoted 2 lines · Kay Sievers
Yes. The drivers are broken in its core use pattern, and enabling a (non-common) new feature exposes this.
You've been saying this for a long time now, but I still don't buy it.
This stuff WORKED before. Now, years later, you're saying that it has a broken use pattern, and needs to be fixed. That's a pipe dream. In the real world, you can't deliberately break things because years later you decide that the use pattern that was working fine before is broken.
It worked only because no one realized that it was broken with the DEPRECATED option enabled. When that is enabled, it is broken, right?
Eric's changes to sysfs to add namespace support exposed this breakage. That's not a reason to paper over the problem, but it should be driving someone to fix it correctly, as has been pointed out a number of times already.
thanks,
greg k-h
Johannes Berg
maintainerRe: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 2 lines · Greg KH
It worked only because no one realized that it was broken with the DEPRECATED option enabled. When that is enabled, it is broken, right?
I'm pretty sure I always had that enabled, and never had issues. Can't test right now since I don't have that option back yet in the tree I'm using.
Quoted 4 lines
Eric's changes to sysfs to add namespace support exposed this breakage. That's not a reason to paper over the problem, but it should be driving someone to fix it correctly, as has been pointed out a number of times already.
I'm just contesting that that someone should be me. I don't think you get to blame driver developers for doing something that worked and solved the problem they needed to solve. sysfs is largely opaque to most of us already, and it now sure feels like Kay decided to change the rules underneath the code in saying "this was wrong all along".
johannes
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 14 lines · Johannes Berg
Quoted 2 lines · Greg KH
It worked only because no one realized that it was broken with the DEPRECATED option enabled. When that is enabled, it is broken, right?
I'm pretty sure I always had that enabled, and never had issues. Can't test right now since I don't have that option back yet in the tree I'm using.
Quoted 4 lines
Eric's changes to sysfs to add namespace support exposed this breakage. That's not a reason to paper over the problem, but it should be driving someone to fix it correctly, as has been pointed out a number of times already.
Quoted 5 lines
I'm just contesting that that someone should be me. I don't think you get to blame driver developers for doing something that worked and solved the problem they needed to solve. sysfs is largely opaque to most of us already, and it now sure feels like Kay decided to change the rules underneath the code in saying "this was wrong all along".
Well, if it worked before, and it doesn't now, that's due to Eric's changes, nothing Kay and I did here :)
But, in looking at it closer, it does seem that the code is doing things that was not expected to work at all previously, and It's amazing that it did. I thought Kay offered to help fix it all up, and provided 2 different ways to do it. I know they aren't trivial, but then again, your usage of sysfs is not trivial either...
thanks,
greg k-h
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 15 lines · Greg KH
Quoted 14 lines · Johannes Berg
Quoted 2 lines · Greg KH
It worked only because no one realized that it was broken with the DEPRECATED option enabled. When that is enabled, it is broken, right?
I'm pretty sure I always had that enabled, and never had issues. Can't test right now since I don't have that option back yet in the tree I'm using.
Quoted 4 lines
Eric's changes to sysfs to add namespace support exposed this breakage. That's not a reason to paper over the problem, but it should be driving someone to fix it correctly, as has been pointed out a number of times already.
Quoted 8 lines
Quoted 5 lines
I'm just contesting that that someone should be me. I don't think you get to blame driver developers for doing something that worked and solved the problem they needed to solve. sysfs is largely opaque to most of us already, and it now sure feels like Kay decided to change the rules underneath the code in saying "this was wrong all along".
Well, if it worked before, and it doesn't now, that's due to Eric's changes, nothing Kay and I did here :)
It mostly worked before, and it works with CONFIG_SYSFS_DEPRECATED=y (After I my stupid bug is fixed).
With CONFIG_SYSFS_DEPRECATED=y we have in sysfs:
........./net:bnep0 -> /sys/class/net/bnep0/sys/class/net/bnep0 is a directory.
With CONFIG_SYSFS_DEPRECATED=n we have in sysfs:
........./bnep0/sys/class/net/bnep0 -> ......./bnep0
Where for a normal network device we have:
With CONFIG_SYSFS_DEPRECATED=y we have in sysfs:
........./net:eth0 -> /sys/class/net/eth0/sys/class/net/bnep0 is a directory.
With CONFIG_SYSFS_DEPRECATED=n we have in sysfs:
........./net/eth0/sys/class/net/eth0 -> ......./net/eth0
The new sysfs layout loses the net namespace separator for bluetooth devices.
Quoted 5 lines
But, in looking at it closer, it does seem that the code is doing things that was not expected to work at all previously, and It's amazing that it did. I thought Kay offered to help fix it all up, and provided 2 different ways to do it. I know they aren't trivial, but then again, your usage of sysfs is not trivial either...
I don't expect much just the upper levels to work correctly. The fact this works on all but a handful of drivers is what I find dist
Does this version of the change look less bleh worthy? The effect is the same as my previous patch but the test is more abstract so the effect is not strictly limited to /sys/class/net.
drivers/base/core.c +1 -1
@@ -673,7 +673,7 @@ static struct kobject *get_device_parent(struct device *dev,*/ if (parent ==NULL ) parent_kobj =virtual_device_parent (dev); -else if (parent->class) +else if (parent->class && !dev->class->ns_type)return &parent->kobj;else parent_kobj = &parent->kobj;
Eric
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 55 lines · Eric W. Biederman
Quoted 15 lines · Greg KH
Quoted 14 lines · Johannes Berg
Quoted 2 lines · Greg KH
It worked only because no one realized that it was broken with the DEPRECATED option enabled. When that is enabled, it is broken, right?
I'm pretty sure I always had that enabled, and never had issues. Can't test right now since I don't have that option back yet in the tree I'm using.
Quoted 4 lines
Eric's changes to sysfs to add namespace support exposed this breakage. That's not a reason to paper over the problem, but it should be driving someone to fix it correctly, as has been pointed out a number of times already.
Quoted 8 lines
Quoted 5 lines
I'm just contesting that that someone should be me. I don't think you get to blame driver developers for doing something that worked and solved the problem they needed to solve. sysfs is largely opaque to most of us already, and it now sure feels like Kay decided to change the rules underneath the code in saying "this was wrong all along".
Well, if it worked before, and it doesn't now, that's due to Eric's changes, nothing Kay and I did here :)
It mostly worked before, and it works with
CONFIG_SYSFS_DEPRECATED=y(After I my stupid bug is fixed).With
CONFIG_SYSFS_DEPRECATED=ywe have in sysfs:
........./net:bnep0 ->/sys/class/net/bnep0/sys/class/net/bnep0is a directory.With
CONFIG_SYSFS_DEPRECATED=nwe have in sysfs:
........./bnep0/sys/class/net/bnep0-> ......./bnep0Where for a normal network device we have:
WithCONFIG_SYSFS_DEPRECATED=ywe have in sysfs:
........./net:eth0 ->/sys/class/net/eth0/sys/class/net/bnep0is a directory.With
CONFIG_SYSFS_DEPRECATED=nwe have in sysfs:
........./net/eth0/sys/class/net/eth0-> ......./net/eth0The new sysfs layout loses the net namespace separator for bluetooth devices.
Quoted 5 lines
But, in looking at it closer, it does seem that the code is doing things that was not expected to work at all previously, and It's amazing that it did. I thought Kay offered to help fix it all up, and provided 2 different ways to do it. I know they aren't trivial, but then again, your usage of sysfs is not trivial either...
Quoted 6 lines
I don't expect much just the upper levels to work correctly. The fact this works on all but a handful of drivers is what I find dist
Does this version of the change look less bleh worthy? The effect is the same as my previous patch but the test is more abstract so the effect is not strictly limited to
/sys/class/net.
What other class type has a namespace at this point in time? Essentially this is the same exact thing, just in a different format that obfuscates what you are really doing here.
As you have now sent this to Linus and he took it, well, it seems moot, right?
kids these days...
greg k-h
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 7 lines · Greg KH
Quoted 3 lines
Does this version of the change look less bleh worthy? The effect is the same as my previous patch but the test is more abstract so the effect is not strictly limited to
/sys/class/net.What other class type has a namespace at this point in time? Essentially this is the same exact thing, just in a different format that obfuscates what you are really doing here.
The patch still looks broken, and does not belong into the core the way it is done. We denied hacks like this for good reason. But out-of-the-blue it was a bluetooth naming regression to fix in the driver core? Interesting!
If someone is going to add namespaces to 'block' or 'input', the sysfs layout will break, and userspace will be unable to handle the resulting changes.
Kay
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 9 lines · Kay Sievers
Quoted 7 lines · Greg KH
Quoted 3 lines
Does this version of the change look less bleh worthy? The effect is the same as my previous patch but the test is more abstract so the effect is not strictly limited to
/sys/class/net.What other class type has a namespace at this point in time? Essentially this is the same exact thing, just in a different format that obfuscates what you are really doing here.
Quoted 8 lines
The patch still looks broken, and does not belong into the core the way it is done. We denied hacks like this for good reason. But out-of-the-blue it was a bluetooth naming regression to fix in the driver core? Interesting!
If someone is going to add namespaces to 'block' or 'input', the sysfs layout will break, and userspace will be unable to handle the resulting changes.
When that happens, I'm sure Eric will be willing to fix up any problems that are found as he is the one that insisted on pushing it to Linus around our objections.
Right Eric?
And Eric, where are you working on the "real" patches to solve the problems of the bnet and wireless driver problems so we can remove this check from the driver core as soon as possible? Any idea when they will be done?
thanks,
greg k-h
Johannes Berg
maintainerRe: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 4 lines · Greg KH
And Eric, where are you working on the "real" patches to solve the problems of the bnet and wireless driver problems so we can remove this check from the driver core as soon as possible? Any idea when they will be done?
They'll be done as soon as the driver core offers the required functionality, which we discussed elsewhere in this thread.
johannes
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 9 lines · Johannes Berg
Quoted 4 lines · Greg KH
And Eric, where are you working on the "real" patches to solve the problems of the bnet and wireless driver problems so we can remove this check from the driver core as soon as possible? Any idea when they will be done?
They'll be done as soon as the driver core offers the required functionality, which we discussed elsewhere in this thread.
{sigh}
Kay provided a solution for this a number of times already. What, are you really going to make him type it up _again_? That's just completly unfair to him.
And note, Eric, this was and is, completely due to your changes, so I am holding you responsible for fixing it correctly. Please do so.
greg k-h
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 8 lines · Greg KH
Quoted 7 lines · Johannes Berg
Quoted 4 lines · Greg KH
And Eric, where are you working on the "real" patches to solve the problems of the bnet and wireless driver problems so we can remove this check from the driver core as soon as possible? Any idea when they will be done?
Quoted 8 lines
Quoted 2 lines
They'll be done as soon as the driver core offers the required functionality, which we discussed elsewhere in this thread.
{sigh}
Kay provided a solution for this a number of times already. What, are you really going to make him type it up _again_? That's just completly unfair to him.
Sure, I still offer any needed help, if there are new requirements now that need core changes. But hey, they will not land magically, if nobody is working on it. And "as soon as the core offers" is not how we make new features work on Linux. The people who add new features, which uncover old bugs, need to fix the old bugs instead of papering them over, like it happened here.
Quoted 2 lines
And note, Eric, this was and is, completely due to your changes, so I am holding you responsible for fixing it correctly. Please do so.
I guess faking a 'bluetooth renaming regression', which never existed, which wasn't even known to the bluetooth maintainers, was just much easier. :)
Kay
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
In reply to Greg KHQuoted 6 lines · Greg KH
Quoted 5 lines · Kay Sievers
Quoted 4 lines · Greg KH
Quoted 3 lines
Does this version of the change look less bleh worthy? The effect is the same as my previous patch but the test is more abstract so the effect is not strictly limited to
/sys/class/net.
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines
What other class type has a namespace at this point in time? Essentially this is the same exact thing, just in a different format that obfuscates what you are really doing here.
The patch still looks broken, and does not belong into the core the way it is done. We denied hacks like this for good reason. But out-of-the-blue it was a bluetooth naming regression to fix in the driver core? Interesting!
This is hardly out of the blue. I reported a problem 3 years ago which is one of the reasons we have the class directories in the first place.
What is different is simply that it was noticed that there were cases the existing heuristic in get_device_parent did not catch.
Any class that has sysfs namespace support requires the class directory. Which means that the test !dev->class->ns_type is exactly the right test to avoid the magic exception in get_device_parent.
Quoted 3 lines
Quoted 3 lines
If someone is going to add namespaces to 'block' or 'input', the sysfs layout will break, and userspace will be unable to handle the resulting changes.
Yes. We don't even have a namespace that we can put those in, so worrying about namespaces other than the network namespace or the user namespace is looking much too far into the future.
What is in the near term is wireless phys getting network namespace support in sysfs, and in practice the exact same issues apply.
Quoted 5 lines
When that happens, I'm sure Eric will be willing to fix up any problems that are found as he is the one that insisted on pushing it to Linus around our objections.
Right Eric?
Yes. I certainly will. The point of the namespace support in sysfs is to keep these things invisible to userspace. If something is visible it is generally a bug.
Quoted 4 lines
And Eric, where are you working on the "real" patches to solve the problems of the bnet and wireless driver problems so we can remove this check from the driver core as soon as possible? Any idea when they will be done?
I looked at that, and it is arguable in the mac80211_hwsim case as that is just a simulation for testing the infrastructure for wireless developers as best as I understand it. The more a simulation differs from reality the more it matters.
For bluetooth drivers or for any other existing subsystem replacing a class device with a bus device is totally inappropriate.
Let me say this again clearly.
class -> bus BROKEN
In the case of bluetooth it would require changing /sys/class/bluetooth to /sys/bus/bluetooth. Which is user visible in the worst possible way and quick google search confirmed it will break user space scripts.
So as much as I sympathize with the position that buses have a better layout than classes, after having examined the issue I completely oppose changing one into the other with our current code base.
So for the medium term I think we are fine.
For the long term moving the driver core from a mandatory policy of sysfs device node creation, to a default library for sysfs device node creation that can be overridden in whole or in part on a subsystem by subsystem basis looks like the right way to go. Based on my experience with sysfs this is likely to be a multi-year project, and one I'm uncertain how motivated I am to tackle.
Eric
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 9 lines · Eric W. Biederman
Greg KH <[email protected]> writes:
Let me say this again clearly.
class -> bus BROKEN
In the case of bluetooth it would require changing
/sys/class/bluetoothto/sys/bus/bluetooth. Which is user visible in the worst possible way and quick google search confirmed it will break user space scripts.
Clearly, we even have a compat API for that, and nothing would break. If needed, a bus can create compat class links. Did you even check with the bluetooth guys, last time I talked to them, they have been totally fine with such a change.
Kay
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 7 lines · Kay Sievers
Quoted 6 lines · Eric W. Biederman
Greg KH <[email protected]> writes:
Let me say this again clearly.
class -> bus BROKEN
Quoted 8 lines
Quoted 3 lines
In the case of bluetooth it would require changing
/sys/class/bluetoothto/sys/bus/bluetooth. ??Which is user visible in the worst possible way and quick google search confirmed it will break user space scripts.Clearly, we even have a compat API for that, and nothing would break. If needed, a bus can create compat class links. Did you even check with the bluetooth guys, last time I talked to them, they have been totally fine with such a change.
Yeah, I even had patches that almost did this work a few years ago. Most of the bluetooth class stuff (if not all of it) can be safely moved to debugfs, as it's just debug information.
I should dig them out and refresh them in my spare time :)
thanks,
greg k-h
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 8 lines · Greg KH
Quoted 7 lines · Kay Sievers
Quoted 6 lines · Eric W. Biederman
Greg KH <[email protected]> writes:
Let me say this again clearly.
class -> bus BROKEN
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines
In the case of bluetooth it would require changing
/sys/class/bluetoothto/sys/bus/bluetooth. ??Which is user visible in the worst possible way and quick google search confirmed it will break user space scripts.Clearly, we even have a compat API for that, and nothing would break. If needed, a bus can create compat class links. Did you even check with the bluetooth guys, last time I talked to them, they have been totally fine with such a change.
Hmm. It appears I missed the compat API in my review of the bus code. Could I get a pointer?
Quoted 3 lines
Yeah, I even had patches that almost did this work a few years ago. Most of the bluetooth class stuff (if not all of it) can be safely moved to debugfs, as it's just debug information.
That change merged about March, so that is no longer an issue.
Eric
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 13 lines · Eric W. Biederman
Quoted 12 lines · Greg KH
Quoted 11 lines · Kay Sievers
Quoted 9 lines · Eric W. Biederman
Greg KH <[email protected]> writes:
Let me say this again clearly.
class -> bus BROKEN
In the case of bluetooth it would require changing
/sys/class/bluetoothto/sys/bus/bluetooth. ??Which is user visible in the worst possible way and quick google search confirmed it will break user space scripts.
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
Clearly, we even have a compat API for that, and nothing would break. If needed, a bus can create compat class links. Did you even check with the bluetooth guys, last time I talked to them, they have been totally fine with such a change.
Hmm. It appears I missed the compat API in my review of the bus code. Could I get a pointer?
It's:
class_compat_create_link()
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/base/class.c;h=8e231d05b40058c6c3191b4a06a15ceff1714be3;hb=HEAD#l559
I think i2c uses this when the class was converted to a bus.
It should only be used if it's really needed for known used userspace interfaces. A few others that got converted already did not need it.
Kay
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 27 lines · Kay Sievers
Quoted 13 lines · Eric W. Biederman
Quoted 12 lines · Greg KH
Quoted 11 lines · Kay Sievers
Quoted 9 lines · Eric W. Biederman
Greg KH <[email protected]> writes:
Let me say this again clearly.
class -> bus BROKEN
In the case of bluetooth it would require changing
/sys/class/bluetoothto/sys/bus/bluetooth. ??Which is user visible in the worst possible way and quick google search confirmed it will break user space scripts.Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
Clearly, we even have a compat API for that, and nothing would break. If needed, a bus can create compat class links. Did you even check with the bluetooth guys, last time I talked to them, they have been totally fine with such a change.
Hmm. It appears I missed the compat API in my review of the bus code. Could I get a pointer?
It's:
class_compat_create_link()
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/base/class.c;h=8e231d05b40058c6c3191b4a06a15ceff1714be3;hb=HEAD#l559
Quoted 4 lines
I think i2c uses this when the class was converted to a bus.
It should only be used if it's really needed for known used userspace interfaces. A few others that got converted already did not need it.
Interesting. The symlink creation is slightly buggy in that it is created after the uevent for device creation has been sent. Which can lead to some interesting races in userspace.
As for the rest the bus compat code is similar but not quite the same as the class code, so I would be extremely reluctant to deploy it except in extremely limited cases. Backwards compatibility is important, and we should strive our best to maintain backwards compatibility it for the kernel<->userspace ABIs.
Eric
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 8 lines · Eric W. Biederman
Quoted 2 lines · Kay Sievers
It should only be used if it's really needed for known used userspace interfaces. A few others that got converted already did not need it.
Interesting. The symlink creation is slightly buggy in that it is created after the uevent for device creation has been sent. Which can lead to some interesting races in userspace.
At uevent time the 'subsystem' is specified by the 'sybsystem' link and the SUBSYSTEM property in the event environment. The device should not really rely on finding itself linked in class. The class and bus link collections are only to find a group of devices of the same subsystem, and this should not be a real world problem here.
Also there are plans to merge struct class and struct bus_type completely and keep only a few flags around where stuff should show up for compatibility. At that point all stuff will be created at the same time.
Quoted 5 lines
As for the rest the bus compat code is similar but not quite the same as the class code, so I would be extremely reluctant to deploy it except in extremely limited cases. Backwards compatibility is important, and we should strive our best to maintain backwards compatibility it for the kernel<->userspace ABIs.
Today, the only real difference between class and bus devices are these 'collection links', the devices otherwise look completely the same. There should be no important difference.
Buses are very much preferred over classes today, no new stuff should create any class. The bus directories are extendable and have a reasonable layout with the devices/ subdirectory, unlike the flat class directories where people got the silly idea to mix devices lists with attributes to confuse everything.
Kay
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 10 lines · Kay Sievers
Quoted 8 lines · Eric W. Biederman
Quoted 2 lines · Kay Sievers
It should only be used if it's really needed for known used userspace interfaces. A few others that got converted already did not need it.
Interesting. The symlink creation is slightly buggy in that it is created after the uevent for device creation has been sent. Which can lead to some interesting races in userspace.
Quoted 5 lines
At uevent time the 'subsystem' is specified by the 'sybsystem' link and the SUBSYSTEM property in the event environment. The device should not really rely on finding itself linked in class. The class and bus link collections are only to find a group of devices of the same subsystem, and this should not be a real world problem here.
I agree that there should be no real world problems. The bottom line is that every sysfs attribute should be created before we send the uevent or else we get horribly subtle races or we get user space code that starts looping looking for the interface.
Quoted 4 lines
Also there are plans to merge struct class and struct
bus_typecompletely and keep only a few flags around where stuff should show up for compatibility. At that point all stuff will be created at the same time.
That part seems reasonable.
Quoted 9 lines
Quoted 5 lines
As for the rest the bus compat code is similar but not quite the same as the class code, so I would be extremely reluctant to deploy it except in extremely limited cases. Backwards compatibility is important, and we should strive our best to maintain backwards compatibility it for the kernel<->userspace ABIs.
Today, the only real difference between class and bus devices are these 'collection links', the devices otherwise look completely the same. There should be no important difference.
I don't see the class subdirectories created for bus devices, and I don't see any equivalent. At least for the network devices this is a huge difference, because the device namespace is controlled by userspace and it is NOT ok to have namespace conflicts with arbitrary sysfs attributes.
Quoted 5 lines
Buses are very much preferred over classes today, no new stuff should create any class. The bus directories are extendable and have a reasonable layout with the devices/ subdirectory, unlike the flat class directories where people got the silly idea to mix devices lists with attributes to confuse everything.
Which is generally reasonable. However busses appear to have the silly idea that it is ok to mix child child device lists of different kinds of children with attributes and confuse everything.
At the subsystem level bus devices look better.
At the individual device level bus devices stacked on bus devices appear to be a namespace disaster.
Eric
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 17 lines · Eric W. Biederman
Quoted 10 lines · Kay Sievers
Quoted 8 lines · Eric W. Biederman
Quoted 2 lines · Kay Sievers
It should only be used if it's really needed for known used userspace interfaces. A few others that got converted already did not need it.
Interesting. The symlink creation is slightly buggy in that it is created after the uevent for device creation has been sent. Which can lead to some interesting races in userspace.
Quoted 5 lines
At uevent time the 'subsystem' is specified by the 'sybsystem' link and the SUBSYSTEM property in the event environment. The device should not really rely on finding itself linked in class. The class and bus link collections are only to find a group of devices of the same subsystem, and this should not be a real world problem here.
Quoted 5 lines
I agree that there should be no real world problems. The bottom line is that every sysfs attribute should be created before we send the uevent or else we get horribly subtle races or we get user space code that starts looping looking for the interface.
Yeah, but most of these things we should have fixed over the last years. There is no single WAIT_FOR instruction left in udev rules. :)
Quoted 13 lines
Quoted 4 lines
Also there are plans to merge struct class and struct
bus_typecompletely and keep only a few flags around where stuff should show up for compatibility. At that point all stuff will be created at the same time.That part seems reasonable.
Quoted 6 lines
Quoted 5 lines
As for the rest the bus compat code is similar but not quite the same as the class code, so I would be extremely reluctant to deploy it except in extremely limited cases. Backwards compatibility is important, and we should strive our best to maintain backwards compatibility it for the kernel<->userspace ABIs.
Quoted 6 lines
Quoted 3 lines
Today, the only real difference between class and bus devices are these 'collection links', the devices otherwise look completely the same. There should be no important difference.
I don't see the class subdirectories created for bus devices, and I don't see any equivalent.
Sure, I meant, if the class-compat stuff is used when a conversion is done.
Quoted 4 lines
At least for the network devices this is a
huge difference, because the device namespace is controlled by userspace and it is NOT ok to have namespace conflicts with arbitrary sysfs attributes.
Yeah, that's why the class-glue directories exist. But so far we did not support stacking of classes of different types.
Quoted 9 lines
Quoted 5 lines
Buses are very much preferred over classes today, no new stuff should create any class. The bus directories are extendable and have a reasonable layout with the devices/ subdirectory, unlike the flat class directories where people got the silly idea to mix devices lists with attributes to confuse everything.
Which is generally reasonable. However buses appear to have the silly idea that it is ok to mix child child device lists of different kinds of children with attributes and confuse everything.
Classes do the same. They are usually distinguished by its name, like mouseX, inputX, sdX, sdX3, they are all different, but still belong to the same 'subsystem'.
Quoted 3 lines
At the subsystem level bus devices look better.
At the individual device level bus devices stacked on bus devices appear to be a namespace disaster.
They are usually created by the same code, in many cases by the same drivers, and have not been a real problem so far. As you said, network devices are special here, because of the ability to rename them from userspace.
At some time in the future, when buses and classes are merged, I expect stuff can just set a flag to have a 'glue dir' created or not.
For now 'glue dirs' are limited to be created between a bus and a class device. It could possibly be extended to be created between classes of different types to handle issues like this.
Kay
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
Quoted 2 lines · Kay Sievers
Yeah, but most of these things we should have fixed over the last years. There is no single
WAIT_FORinstruction left in udev rules. :)
Last time I looked there were quite a few attributes that were still getting created late. I would not be surprised if the common case works fine, but I know of a least one and I think a couple of weird cases that still have to do unpleasant things.
Still that is a project for another time.
Quoted 9 lines
Quoted 3 lines
At the subsystem level bus devices look better.
At the individual device level bus devices stacked on bus devices appear to be a namespace disaster.They are usually created by the same code, in many cases by the same drivers, and have not been a real problem so far. As you said, network devices are special here, because of the ability to rename them from userspace.
Quoted 6 lines
At some time in the future, when buses and classes are merged, I expect stuff can just set a flag to have a 'glue dir' created or not.
For now 'glue dirs' are limited to be created between a bus and a class device. It could possibly be extended to be created between classes of different types to handle issues like this.
Sounds like a plan. And now I'm off on vacation.
Have a good one.
Eric
PATCH 1/1 Driver-core: Always create class directories for classses that support namespaces.
In reply to Greg KHThis fixes the regression in 2.6.35-rcX where bluetooth network devices would fail to be deleted from sysfs, causing their destruction and recreation to fail. In addition this fixes the mac80211_hwsim driver where it would leave around sysfs files when the driver was removed. This problem is discussed at
https://bugzilla.kernel.org/show_bug.cgi?id=16257
The reason for the regression is that the network namespace support added to sysfs expects and requires that network devices be put in directories that can contain only network devices. Today get_device_parent almost provides that guarantee for all class devices, except for a specific exception when the parent of a class devices is a class device. It would be nice to simply remove that arguably incorrect special case, but apparently the input devices depend on it being there. So I have only removed it for class devices with network namespace support. Which today are the network devices.
It has been suggested that a better fix would be to change the parent device from a class device to a bus device, which in the case of the bluetooth driver would change /sys/class/bluetooth to /sys/bus/bluetoth, I can not see how we would avoid significant userspace breakage if we were to make that change.
Adding an extra directory in the path to the device will also be userspace visible but it is much less likely to break things. Everything is still accessible from /sys/class (for example), and it fixes two bugs. Adding an extra directory fixes a 3 year old regression introduced with the new sysfs layout that makes if impossible to rename bnep0 network devices to names that conflict with hci device attributes like hci_revsion. Adding an additional directory remove the new failure modes introduced by the network namespace code.
If it weren't for the regession in the renaming of network devices I would figure out how to just make the sysfs code deal with this configuration of devices.
In summary this patch fixes regressions by changing:
"/sys/class/bluetooth/hci0/bnep0" to "/sys/class/bluetooth/hci0/net/bnep0".
---
p.s. Linus my apologies for sending this directly but I have gotten an incredible amount of flak trying to fix this problem, and I would like not to leave an accidental regression whose cause is well known in 2.6.35 if I can help it.
drivers/base/core.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/base/core.c +1 -1
@@ -673,7 +673,7 @@ static struct kobject *get_device_parent(struct device *dev,*/ if (parent ==NULL ) parent_kobj =virtual_device_parent (dev); -else if (parent->class) +else if (parent->class && !dev->class->ns_type)return &parent->kobj;else parent_kobj = &parent->kobj;
Signature · 1 line
1.6.5.2.143.g8cc62
Re: [PATCH] Driver-core: Always create class directories for classses that support namespaces.
Quoted 4 lines · Eric W. Biederman
p.s. Linus my apologies for sending this directly but I have gotten an incredible amount of flak trying to fix this problem, and I would like not to leave an accidental regression whose cause is well known in 2.6.35 if I can help it.
"flak"?
{sigh}
No, it's just been an issue of fixing the issue in the correct manner. You sent this patch a mere 2 days prior, and I've been away since then and haven't even _had_ the chance to apply it to my tree and run it through any testing.
Heck, it hasn't even been in the linux-next tree, and seen if it blows up Andrew's notorious machine.
Next time, give me a chance to at least build your patch before getting upset and routing around me. The other 2 patches you have sent me have had testing in linux-next and will go to Linus later today, and I have half-a-mind to revert this one, but I just know you will be willing to fix up any problems found in it now :)
What a nice way to return from a 3day vacation...
greg k-h
Re: [PATCH] Driver-core: Fix bluetooth network device rename regression
In reply to Johannes BergQuoted 6 lines · Johannes Berg
Quoted 2 lines · Kay Sievers
Yes. The drivers are broken in its core use pattern, and enabling a (non-common) new feature exposes this.
You've been saying this for a long time now, but I still don't buy it.
Well, than make different point. Stuff did not change for 3 years in a row, and still does not need special handling of a specific subsystem in the driver core.
Quoted 1 line
This stuff WORKED before.
Sure, and it still does.
Quoted 4 lines
Now, years later, you're saying that it has a
broken use pattern, and needs to be fixed. That's a pipe dream. In the real world, you can't deliberately break things because years later you decide that the use pattern that was working fine before is broken.
Yes, new stuff exposes old bugs, that's normal business. But there is zero reason not to fix the problem then, but to introduce plain wrong hacks in the wrong place.
Btw, I hope you guys stop the personal affronts in your mails, and start focusing on a proper solution for the problem. I'll stop responding if this will not go back to a discussion about technical details.
Thanks,
Kay
Re: [PATCH] sysfs: Don't allow the creation of symlinks we can't remove
In reply to Eric W. BiedermanQuoted 3 lines · Eric W. Biederman
Recently my tagged sysfs support revealed a flaw in the device core that a few rare drivers are running into such that we don't always put network devices in a class subdirectory named
net/.
I've been cc-ed for a reason I suppose -- any network driver I should be
looking into? What's the actual problem?
Maciej
Johannes Berg
maintainerRe: [PATCH] sysfs: Don't allow the creation of symlinks we can't remove
In reply to Eric W. BiedermanQuoted 12 lines · Eric W. Biederman
Recently my tagged sysfs support revealed a flaw in the device core that a few rare drivers are running into such that we don't always put network devices in a class subdirectory named
net/.Since we are not creating the class directory the network devices wind up in a non-tagged directory, but the symlinks to the network devices from
/sys/class/netare in a tagged directory. All of which works until we go to remove or rename the symlink. When we remove or rename a symlink we look in the namespace of the target of the symlink. Since the target of the symlink is in a non-tagged sysfs directory we don't have a namespace to look in, and we fail to remove the symlink.
Quoted 3 lines
Detect this problem up front and simply don't create symlinks we won't be able to remove later. This prevents symlink leakage and fails in a much clearer and more understandable way.
Eric, I was looking into sysfs netns support for wireless, and with this patch applied I just get the warning and no network interfaces.
Was there any patch that was supposed to fix hwsim?
johannes
Re: [PATCH] sysfs: Don't allow the creation of symlinks we can't remove
Quoted 13 lines · Johannes Berg
Quoted 12 lines · Eric W. Biederman
Recently my tagged sysfs support revealed a flaw in the device core that a few rare drivers are running into such that we don't always put network devices in a class subdirectory named
net/.Since we are not creating the class directory the network devices wind up in a non-tagged directory, but the symlinks to the network devices from
/sys/class/netare in a tagged directory. All of which works until we go to remove or rename the symlink. When we remove or rename a symlink we look in the namespace of the target of the symlink. Since the target of the symlink is in a non-tagged sysfs directory we don't have a namespace to look in, and we fail to remove the symlink.
Quoted 6 lines
Quoted 3 lines
Detect this problem up front and simply don't create symlinks we won't be able to remove later. This prevents symlink leakage and fails in a much clearer and more understandable way.
Eric, I was looking into sysfs netns support for wireless, and with this patch applied I just get the warning and no network interfaces.
The warning patch just makes things fail faster. Although I get some of the wireless interfaces for hwsim when I use this one.
Quoted 1 line
Was there any patch that was supposed to fix hwsim?
- If you have my patches that fix CONFIG_SYSFS_DEPRECATED, you should find everything works there.
As for a proper fix I have just resent my one liner to drives/base/core.c I can't think of a better option right now.
For hwsim it is arguable, but the behaviour of sysfs for the bluetooth bnep driver is very clearly a 3 year old regression, and the cause is exactly the same.
Eric
Johannes Berg
maintainerRe: [PATCH] sysfs: Don't allow the creation of symlinks we can't remove
Quoted 9 lines · Eric W. Biederman
Quoted 6 lines
Quoted 3 lines
Detect this problem up front and simply don't create symlinks we won't be able to remove later. This prevents symlink leakage and fails in a much clearer and more understandable way.
Eric, I was looking into sysfs netns support for wireless, and with this patch applied I just get the warning and no network interfaces.
The warning patch just makes things fail faster. Although I get some of the wireless interfaces for hwsim when I use this one.
Hmm, I didn't.
Quoted 4 lines
Quoted 1 line
Was there any patch that was supposed to fix hwsim?
- If you have my patches that fix
CONFIG_SYSFS_DEPRECATED, you should find everything works there.
But then I was carrying those two patches too.
Quoted 6 lines
As for a proper fix I have just resent my one liner to
drives/base/core.cI can't think of a better option right now.For hwsim it is arguable, but the behaviour of sysfs for the bluetooth bnep driver is very clearly a 3 year old regression, and the cause is exactly the same.
Right, it actually starts working again with that patch you sent. However, netns support is really broken:
<create net namespace, put phy0/wlan0 into it>
root@kvm:~# ip link
3: wlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN qlen 1000
link/ether 02:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
7: lo: <LOOPBACK> mtu 16436 qdisc noop state DOWN
link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
root@kvm:~# ls /sys/class/net/
eth0 hwsim0 lo wlan1 wlan2johannes
Re: [PATCH] sysfs: Don't allow the creation of symlinks we can't remove
Quoted 6 lines · Johannes Berg
Quoted 4 lines · Eric W. Biederman
Quoted 4 lines
Quoted 3 lines
Detect this problem up front and simply don't create symlinks we won't be able to remove later. This prevents symlink leakage and fails in a much clearer and more understandable way.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Eric, I was looking into sysfs netns support for wireless, and with this patch applied I just get the warning and no network interfaces.
The warning patch just makes things fail faster. Although I get some of the wireless interfaces for hwsim when I use this one.
Hmm, I didn't.
To be clear I just get hwsim0. Not wlan0 or wlan1.
Quoted 10 lines
Quoted 4 lines
Quoted 1 line
Was there any patch that was supposed to fix hwsim?
- If you have my patches that fix
CONFIG_SYSFS_DEPRECATED, you should find everything works there.But then I was carrying those two patches too.
Quoted 3 lines
As for a proper fix I have just resent my one liner to
drives/base/core.cI can't think of a better option right now.
Quoted 8 lines
Quoted 3 lines
For hwsim it is arguable, but the behaviour of sysfs for the bluetooth bnep driver is very clearly a 3 year old regression, and the cause is exactly the same.
Right, it actually starts working again with that patch you sent. However, netns support is really broken:
<create net namespace, put phy0/wlan0 into it>
Do we have a convenient command line tool to do this? I remember there being a different netlink message from normal network devices.
Quoted 7 lines
root@kvm:~# ip link
3: wlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN qlen 1000
link/ether 02:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
7: lo: <LOOPBACK> mtu 16436 qdisc noop state DOWN
link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
root@kvm:~# ls /sys/class/net/
eth0 hwsim0 lo wlan1 wlan2I think this is actually the output of something working.
I expect after you created a new netns you didn't mount a new instance of /sys. /sys remembers which netns you had when you mounted it. So you have to mount /sys again so you can see the /sys/class/net for the network namespace you are in.
Eric
Johannes Berg
maintainerRe: [PATCH] sysfs: Don't allow the creation of symlinks we can't remove
Quoted 6 lines · Eric W. Biederman
Quoted 4 lines
Quoted 2 lines
The warning patch just makes things fail faster. Although I get some of the wireless interfaces for hwsim when I use this one.
Hmm, I didn't.
To be clear I just get hwsim0. Not wlan0 or wlan1.
Ah, yes, but that's just a regular netdev, you can pretty much ignore it. It just shows all hwsim traffic as it is on the "air" for sniffing.
Quoted 8 lines
Quoted 4 lines
Right, it actually starts working again with that patch you sent. However, netns support is really broken:
<create net namespace, put phy0/wlan0 into it>
Do we have a convenient command line tool to do this? I remember there being a different netlink message from normal network devices.
iw phy0 set netns <pid>
http://git.sipsolutions.net/iw.git
Quoted 8 lines
Quoted 7 lines
root@kvm:~# ip link 3: wlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN qlen 1000 link/ether 02:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff 7: lo: <LOOPBACK> mtu 16436 qdisc noop state DOWN link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00 root@kvm:~# ls /sys/class/net/ eth0 hwsim0 lo wlan1 wlan2
Quoted 7 lines
I think this is actually the output of something working.
I expect after you created a new netns you didn't mount a new instance of /sys. /sys remembers which netns you had when you mounted it. So you have to mount /sys again so you can see the
/sys/class/netfor the network namespace you are in.
Ohh, oops! I saw all the "current->" references in the code and somehow expected the same instance of sysfs to show the right thing.
Yes, it works now. But the patch below doesn't seem to work, am I missing something?
johannes
--- include/linux/netdevice.h | 2 ++ net/core/net-sysfs.c | 3 ++- net/wireless/sysfs.c | 9 +++++++++ 3 files changed, 13 insertions(+), 1 deletion(-)
include/linux/netdevice.h +2 -0
@@ -2148,6 +2148,8 @@ extern void dev_seq_stop(struct seq_fileextern int netdev_class_create_file (struct class_attribute *class_attr);extern void netdev_class_remove_file (struct class_attribute *class_attr); +extern struct kobj_ns_type_operations net_ns_type_operations; +extern char *netdev_drivername (const struct net_device *dev,char *buffer,int len);extern void linkwatch_run_queue (void );
net/core/net-sysfs.c +2 -1
@@ -785,12 +785,13 @@ static const void *net_netlink_ns(structreturn sock_net (sk); } -static struct kobj_ns_type_operations net_ns_type_operations = { +struct kobj_ns_type_operations net_ns_type_operations = { .type =KOBJ_NS_TYPE_NET , .current_ns = net_current_ns, .netlink_ns = net_netlink_ns, .initial_ns = net_initial_ns, }; +EXPORT_SYMBOL_GPL (net_ns_type_operations);static void net_kobj_ns_exit (struct net *net) {
net/wireless/sysfs.c +9 -0
@@ -110,6 +110,13 @@ static int wiphy_resume(struct device *dreturn ret; } +static const void *wiphy_namespace (struct device *d) +{ + struct wiphy *wiphy = container_of(d, struct wiphy, dev); + +return wiphy_net (wiphy); +} +struct class ieee80211_class = { .name ="ieee80211" , .owner =THIS_MODULE ,
@@ -120,6 +127,8 @@ struct class ieee80211_class = {#endif .suspend = wiphy_suspend, .resume = wiphy_resume, + .ns_type = &net_ns_type_operations, + .namespace = wiphy_namespace, };int wiphy_sysfs_init (void )
Re: [PATCH] sysfs: Don't allow the creation of symlinks we can't remove
Quoted 41 lines · Johannes Berg
Quoted 6 lines · Eric W. Biederman
Quoted 4 lines
Quoted 2 lines
The warning patch just makes things fail faster. Although I get some of the wireless interfaces for hwsim when I use this one.
Hmm, I didn't.
To be clear I just get hwsim0. Not wlan0 or wlan1.
Ah, yes, but that's just a regular netdev, you can pretty much ignore it. It just shows all hwsim traffic as it is on the "air" for sniffing.
Quoted 8 lines
Quoted 4 lines
Right, it actually starts working again with that patch you sent. However, netns support is really broken:
<create net namespace, put phy0/wlan0 into it>
Do we have a convenient command line tool to do this? I remember there being a different netlink message from normal network devices.
iw phy0 set netns <pid>
http://git.sipsolutions.net/iw.git
Quoted 8 lines
Quoted 7 lines
root@kvm:~# ip link 3: wlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN qlen 1000 link/ether 02:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff 7: lo: <LOOPBACK> mtu 16436 qdisc noop state DOWN link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00 root@kvm:~# ls /sys/class/net/ eth0 hwsim0 lo wlan1 wlan2Quoted 7 lines
I think this is actually the output of something working.
I expect after you created a new netns you didn't mount a new instance of /sys. /sys remembers which netns you had when you mounted it. So you have to mount /sys again so you can see the
/sys/class/netfor the network namespace you are in.
Quoted 5 lines
Ohh, oops! I saw all the "current->" references in the code and somehow expected the same instance of sysfs to show the right thing.
Yes, it works now. But the patch below doesn't seem to work, am I missing something?
You are trying to move the phy devices as well?
My guess is that at least part of the problem is that you don't have a ieee80211 directory under hwsim.
My apologies for not thinking about the peculiarities of the wireless drivers.
Eric
Quoted 59 lines
--- include/linux/netdevice.h | 2 ++ net/core/net-sysfs.c | 3 ++- net/wireless/sysfs.c | 9 +++++++++ 3 files changed, 13 insertions(+), 1 deletion(-)include/linux/netdevice.h +2
-0@@ -2148,6 +2148,8 @@ extern void dev_seq_stop(struct seq_fileextern int netdev_class_create_file (struct class_attribute *class_attr);extern void netdev_class_remove_file (struct class_attribute *class_attr); +extern struct kobj_ns_type_operations net_ns_type_operations; +extern char *netdev_drivername (const struct net_device *dev,char *buffer,int len);extern void linkwatch_run_queue (void );net/core/net-sysfs.c +2
-1@@ -785,12 +785,13 @@ static const void *net_netlink_ns(structreturn sock_net (sk); } -static struct kobj_ns_type_operations net_ns_type_operations = { +struct kobj_ns_type_operations net_ns_type_operations = { .type =KOBJ_NS_TYPE_NET , .current_ns = net_current_ns, .netlink_ns = net_netlink_ns, .initial_ns = net_initial_ns, }; +EXPORT_SYMBOL_GPL (net_ns_type_operations);static void net_kobj_ns_exit (struct net *net) {net/wireless/sysfs.c +9
-0@@ -110,6 +110,13 @@ static int wiphy_resume(struct device *dreturn ret; } +static const void *wiphy_namespace (struct device *d) +{ + struct wiphy *wiphy = container_of(d, struct wiphy, dev); + +return wiphy_net (wiphy); +} +struct class ieee80211_class = { .name ="ieee80211" , .owner =THIS_MODULE ,@@ -120,6 +127,8 @@ struct class ieee80211_class = {#endif .suspend = wiphy_suspend, .resume = wiphy_resume, + .ns_type = &net_ns_type_operations, + .namespace = wiphy_namespace, };int wiphy_sysfs_init (void )
Johannes Berg
maintainerRe: [PATCH] sysfs: Don't allow the creation of symlinks we can't remove
Quoted 24 lines · Eric W. Biederman
Quoted 24 lines
Quoted 3 lines
Do we have a convenient command line tool to do this? I remember there being a different netlink message from normal network devices.
iw phy0 set netns <pid>
http://git.sipsolutions.net/iw.git
Quoted 15 lines
Quoted 7 lines
root@kvm:~# ip link 3: wlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN qlen 1000 link/ether 02:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff 7: lo: <LOOPBACK> mtu 16436 qdisc noop state DOWN link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00 root@kvm:~# ls /sys/class/net/ eth0 hwsim0 lo wlan1 wlan2I think this is actually the output of something working.
I expect after you created a new netns you didn't mount a new instance of /sys. /sys remembers which netns you had when you mounted it. So you have to mount /sys again so you can see the
/sys/class/netfor the network namespace you are in.
Quoted 7 lines
Quoted 5 lines
Ohh, oops! I saw all the "current->" references in the code and somehow expected the same instance of sysfs to show the right thing.
Yes, it works now. But the patch below doesn't seem to work, am I missing something?
You are trying to move the phy devices as well?
Yes. The intent is that each wireless phy lives in a netns along with all of its child devices.
Quoted 2 lines
My guess is that at least part of the problem is that you don't have a ieee80211 directory under hwsim.
But I should have? 'ieee80211' is a class just like 'net', no?
Quoted 2 lines
My apologies for not thinking about the peculiarities of the wireless drivers.
No worries.
johannes
Quoted 2 lines · Greg KH
Does this only show up if you enable network namespaces? Or is it a problem with the "normal" kernel configuration of no network namespaces?
It is a problem with the normal kernel configuration. In that case we have a single network namespace.
Quoted 3 lines
The only thing that changed here was your network namespace work, what caused this problem to show up? Was it bisectable down to a single patch?
The problem is that when we remove symlinks we look at the target of the symlink to see which tag to remove it in it's parent directory. Because the target of the symlink was not in a class directory because of that crazy class on class stacking exception in get_device_parent we fail to find the symlink when we attempt to remove it.
My one line patch to get_device_parent really does fix this. I have just sent you a patch to prevent the creation of these crazy symlinks in sysfs, which at least clearly isolates this to a handful of drivers.
Quoted 5 lines
Classes on top of classes should never have originally worked, I guess we just let them slide by accident, and we can go and fix them up as they are found. But for now, for .35, it would be good to find the root cause of the problem here. It might be as simple as putting a
CONFIG_BROKENon network namespaces until we get this working, right?
Nope, it isn't as simple as disabling network namespaces.
For the mac80211_hwsim we can just remove the device parent, and all will be well for the moment.
For the bnep code we could also not set the parent but I don't know if that would have undesirable complications for power management or not. I don't have a bluetooth attached network device so I don't even know how to test that code.
My preferred fix is the one liner I set you to get_device_parent. It is no worse than the magic device_is_not_partition checks then we already have in sysfs. Especially when it is a bug that get_device_parent does that early return in the first place. To fix this we will have to introduce the missing subdirectory one way or another.
With SYSFS_DEPRECATED enabled I don't believe this problem can actually happen, as all network devices are placed immediately under /sys/class/net/ in sysfs.
Eric