| Message ID | m1zkxj27xp.fsf_-_@fess.ebiederm.org |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Jul 22, 2010 at 11:16, Eric W. Biederman <ebiederm@xmission.com> wrote: > > With 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. 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. > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2010-07-22 at 15:38 +0200, Kay Sievers wrote: > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 22, 2010 at 16:10, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2010-07-22 at 15:38 +0200, Kay Sievers wrote: >> >> 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. > 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. > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Kay Sievers <kay.sievers@vrfy.org> 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. >> --- a/drivers/base/core.c >> +++ b/drivers/base/core.c >> @@ -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)) > > 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. > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 22, 2010 at 19:18, Eric W. Biederman <ebiederm@xmission.com> wrote: > Kay Sievers <kay.sievers@vrfy.org> 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. > 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. > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2010-07-22 at 19:44 +0200, Kay Sievers wrote: > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 22, 2010 at 08:20:28PM +0200, Johannes Berg wrote: > On Thu, 2010-07-22 at 19:44 +0200, Kay Sievers wrote: > > > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2010-07-22 at 11:28 -0700, Greg KH wrote: > 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. > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 22, 2010 at 08:36:41PM +0200, Johannes Berg wrote: > On Thu, 2010-07-22 at 11:28 -0700, Greg KH wrote: > > > 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. > > > 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". 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 22, 2010 at 20:20, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2010-07-22 at 19:44 +0200, Kay Sievers wrote: > >> 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. > This stuff WORKED before. Sure, and it still does. > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/base/core.c b/drivers/base/core.c index 9630fbd..ffb8443 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -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;
With 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 <kay.sievers@vrfy.org> 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 <kay.sievers@vrfy.org> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> 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. Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- drivers/base/core.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)