Patchwork Driver-core: Fix bluetooth network device rename regression

login
register
mail settings
Submitter Eric W. Biederman
Date July 22, 2010, 9:16 a.m.
Message ID <m1zkxj27xp.fsf_-_@fess.ebiederm.org>
Download mbox | patch
Permalink /patch/59552/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Eric W. Biederman - July 22, 2010, 9:16 a.m.
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(-)
Kay Sievers - July 22, 2010, 1:38 p.m.
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
Johannes Berg - July 22, 2010, 2:10 p.m.
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
Kay Sievers - July 22, 2010, 3:30 p.m.
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
Eric W. Biederman - July 22, 2010, 5:18 p.m.
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
Kay Sievers - July 22, 2010, 5:44 p.m.
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
Johannes Berg - July 22, 2010, 6:20 p.m.
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
gregkh@suse.de - July 22, 2010, 6:28 p.m.
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
Johannes Berg - July 22, 2010, 6:36 p.m.
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
gregkh@suse.de - July 22, 2010, 6:54 p.m.
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
Kay Sievers - July 22, 2010, 11:03 p.m.
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

Patch

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;