sysfs class/net/ problem

Hi,

I didn't find anything related in the archives, if I missed it I'd appreciate a pointer. I'm currently experiencing the following:

# ip link
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 16436 qdisc noqueue state UNKNOWN 
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UNKNOWN qlen 1000
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
# ls -l /sys/class/net/
total 0
lrwxrwxrwx 1 root root 0 Jun  2 13:12 eth0 -> ../../devices/pci0000:00/0000:00:03.0/net/eth0
lrwxrwxrwx 1 root root 0 Jun  2 13:12 lo -> ../../devices/virtual/net/lo
# insmod cfg80211.ko ; insmod mac80211.ko ; insmod mac80211_hwsim.ko  radios=3 
# ip link
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 16436 qdisc noqueue state UNKNOWN 
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UNKNOWN qlen 1000
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
5: 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
6: wlan1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN qlen 1000
    link/ether 02:00:00:00:01:00 brd ff:ff:ff:ff:ff:ff
7: wlan2: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN qlen 1000
    link/ether 02:00:00:00:02:00 brd ff:ff:ff:ff:ff:ff
8: hwsim0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN 
    link/ieee802.11/radiotap 12:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
# ls -l /sys/class/net/
total 0
lrwxrwxrwx 1 root root 0 Jun  2 13:12 eth0 -> ../../devices/pci0000:00/0000:00:03.0/net/eth0
lrwxrwxrwx 1 root root 0 Jun  2 13:14 hwsim0 -> ../../devices/virtual/net/hwsim0
lrwxrwxrwx 1 root root 0 Jun  2 13:12 lo -> ../../devices/virtual/net/lo
lrwxrwxrwx 1 root root 0 Jun  2 13:14 wlan0 -> ../../devices/virtual/mac80211_hwsim/hwsim0/wlan0
lrwxrwxrwx 1 root root 0 Jun  2 13:14 wlan1 -> ../../devices/virtual/mac80211_hwsim/hwsim1/wlan1
lrwxrwxrwx 1 root root 0 Jun  2 13:14 wlan2 -> ../../devices/virtual/mac80211_hwsim/hwsim2/wlan2
# rmmod mac80211_hwsim mac80211 cfg80211
# ip link
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 16436 qdisc noqueue state UNKNOWN 
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UNKNOWN qlen 1000

    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
# ls -l /sys/class/net/
total 0
lrwxrwxrwx 1 root root 0 Jun  2 13:12 eth0 -> ../../devices/pci0000:00/0000:00:03.0/net/eth0
lrwxrwxrwx 1 root root 0 Jun  2 13:12 lo -> ../../devices/virtual/net/lo
lrwxrwxrwx 1 root root 0 Jun  2 13:14 wlan0 -> ../../devices/virtual/mac80211_hwsim/hwsim0/wlan0
lrwxrwxrwx 1 root root 0 Jun  2 13:14 wlan1 -> ../../devices/virtual/mac80211_hwsim/hwsim1/wlan1
lrwxrwxrwx 1 root root 0 Jun  2 13:14 wlan2 -> ../../devices/virtual/mac80211_hwsim/hwsim2/wlan2

Quoted 5 lines

# rmmod mac80211_hwsim mac80211 cfg80211
# ip link
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 16436 qdisc noqueue state UNKNOWN 
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UNKNOWN qlen 1000

Quoted 8 lines

    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
# ls -l /sys/class/net/
total 0
lrwxrwxrwx 1 root root 0 Jun  2 13:12 eth0 -> ../../devices/pci0000:00/0000:00:03.0/net/eth0
lrwxrwxrwx 1 root root 0 Jun  2 13:12 lo -> ../../devices/virtual/net/lo
lrwxrwxrwx 1 root root 0 Jun  2 13:14 wlan0 -> ../../devices/virtual/mac80211_hwsim/hwsim0/wlan0
lrwxrwxrwx 1 root root 0 Jun  2 13:14 wlan1 -> ../../devices/virtual/mac80211_hwsim/hwsim1/wlan1
lrwxrwxrwx 1 root root 0 Jun  2 13:14 wlan2 -> ../../devices/virtual/mac80211_hwsim/hwsim2/wlan2

Ah, so the network devices aren't getting removed?

Do you have network namespaces enabled in your kernel or disabled?

And this is 2.6.35-rc1, right?

Quoted 14 lines · Greg KH

Quoted 5 lines

# rmmod mac80211_hwsim mac80211 cfg80211
# ip link
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 16436 qdisc noqueue state UNKNOWN 
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UNKNOWN qlen 1000

Quoted 8 lines

    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
# ls -l /sys/class/net/
total 0
lrwxrwxrwx 1 root root 0 Jun  2 13:12 eth0 -> ../../devices/pci0000:00/0000:00:03.0/net/eth0
lrwxrwxrwx 1 root root 0 Jun  2 13:12 lo -> ../../devices/virtual/net/lo
lrwxrwxrwx 1 root root 0 Jun  2 13:14 wlan0 -> ../../devices/virtual/mac80211_hwsim/hwsim0/wlan0
lrwxrwxrwx 1 root root 0 Jun  2 13:14 wlan1 -> ../../devices/virtual/mac80211_hwsim/hwsim1/wlan1
lrwxrwxrwx 1 root root 0 Jun  2 13:14 wlan2 -> ../../devices/virtual/mac80211_hwsim/hwsim2/wlan2

Quoted 1 line

Ah, so the network devices aren't getting removed?

Well the netdevs are gone, just the links aren't going away. the mac80211_hwsim directory is gone too.

Quoted 1 line

Do you have network namespaces enabled in your kernel or disabled?

enabled

Quoted 1 line

And this is 2.6.35-rc1, right?

yes.

Come to think of it, maybe somehow it ends up removing mac80211_hwsim/hwsim0 before wlan0, and thus the link stays around? I guess I could make it print messages about that somehow?

Quoted 13 lines

Quoted 1 line

Ah, so the network devices aren't getting removed?

Well the netdevs are gone, just the links aren't going away. the mac80211_hwsim directory is gone too.

Quoted 1 line

Do you have network namespaces enabled in your kernel or disabled?

enabled

Quoted 1 line

And this is 2.6.35-rc1, right?

yes.

Quoted 3 lines

Come to think of it, maybe somehow it ends up removing mac80211_hwsim/hwsim0 before wlan0, and thus the link stays around? I guess I could make it print messages about that somehow?

The wireless drivers are a little different, and come to think of it network namespace support has been added to the wireless drivers since last I looked closely.

Do you know what creates/deletes these links?
Is it the normal register_netdevice -> device_add path?

I definitely changed the symlink code a little making things network namespace aware so it is reasonable to assume that something in my changes affected the wireless drivers.

I took a quick look and with my patches against 2.6.33 I'm not seeing this.

I take a closer look at 2.6.35-rc1 and see if I can figure out what is going on. It would definitely be wrong if hwsim0 is removed before wlan0.

Quoted 13 lines · Eric W. Biederman

Quoted 13 lines

Quoted 1 line

Ah, so the network devices aren't getting removed?

Well the netdevs are gone, just the links aren't going away. the mac80211_hwsim directory is gone too.

Quoted 1 line

Do you have network namespaces enabled in your kernel or disabled?

enabled

Quoted 1 line

And this is 2.6.35-rc1, right?

yes.

Quoted 7 lines

Quoted 3 lines

Come to think of it, maybe somehow it ends up removing mac80211_hwsim/hwsim0 before wlan0, and thus the link stays around? I guess I could make it print messages about that somehow?

The wireless drivers are a little different, and come to think of it network namespace support has been added to the wireless drivers since last I looked closely.

Yeah, I now need to go add tagged sysfs support to it too.

Quoted 2 lines

Do you know what creates/deletes these links?
Is it the normal register_netdevice -> device_add path?

Yes, they aren't done specially.

Quoted 5 lines

I definitely changed the symlink code a little making things network namespace aware so it is reasonable to assume that something in my changes affected the wireless drivers.

I took a quick look and with my patches against 2.6.33 I'm not seeing this.

Hmm. I'm also not seeing it with veth, it would seem that ought to be similar?

Quoted 3 lines

I take a closer look at 2.6.35-rc1 and see if I can figure out what is going on. It would definitely be wrong if hwsim0 is removed before wlan0.

I don't know if that's happening .. just guessing that it might cause such a problem, and maybe some things are deferred somehow? Since netdev destruction can be deferred, but the wifi sysfs destruction isn't. But then that link there should cause the refcount to not go down until the link goes away>?

Quoted 2 lines

Hmm. I'm also not seeing it with veth, it would seem that ought to be similar?

Since it is the register_netdev path it should be exactly the same.

The big change I made is I in some instances I replaced sysfs_remove_link with sysfs_delete_link so I could have enough information to infer which network namespace the link was in. Since wlan0 is a netdevice all of that information should already be there.

Quoted 5 lines

I don't know if that's happening .. just guessing that it might cause such a problem, and maybe some things are deferred somehow? Since netdev destruction can be deferred, but the wifi sysfs destruction isn't. But then that link there should cause the refcount to not go down until the link goes away>?

unregister_netdevice will defer the final destruction but it does not defer netdev_unregister_kobject -> device_del.

What happens if in exit_mac80211_hwsim you call unregister_netdev before mac80211_hwsim_free?

At a quick glance it simply looks like you have the ordering reversed in your module cleanup, and this is not network namespace related at all.

Quoted 9 lines · Eric W. Biederman

Quoted 2 lines

Hmm. I'm also not seeing it with veth, it would seem that ought to be similar?

Since it is the register_netdev path it should be exactly the same.

The big change I made is I in some instances I replaced sysfs_remove_link with sysfs_delete_link so I could have enough information to infer which network namespace the link was in. Since wlan0 is a netdevice all of that information should already be there.

I have no idea :)

Quoted 12 lines

Quoted 5 lines

I don't know if that's happening .. just guessing that it might cause such a problem, and maybe some things are deferred somehow? Since netdev destruction can be deferred, but the wifi sysfs destruction isn't. But then that link there should cause the refcount to not go down until the link goes away>?

unregister_netdevice will defer the final destruction but it does not defer netdev_unregister_kobject -> device_del.

What happens if in exit_mac80211_hwsim you call unregister_netdev before mac80211_hwsim_free?

Quoted 2 lines

At a quick glance it simply looks like you have the ordering reversed in your module cleanup, and this is not network namespace related at all.

Nah, the unregister_netdev there removes the "hwsim0" device, while the mac80211_hwsim_free() will remove all the others, so the ordering of those two doesn't matter.

So far that hypothesis that the target of the symlink is being removed before the actual actual link looks like it could cause this.

Are there any other left overs in sysfs, besides just /sys/class/net/wlan0?

Quoted 2 lines · Eric W. Biederman

So far that hypothesis that the target of the symlink is being removed before the actual actual link looks like it could cause this.

Yeah though I'm not sure how that would happen? Wouldn't the symlink cause the target kobject to still be referenced, and thus stay around until the symlink goes away?

Quoted 1 line

Are there any other left overs in sysfs, besides just /sys/class/net/wlan0?

No, not based on find /sys and diffing before/after anyway.

If you want to dig into this look at sysfs_delete_link. instrument it so that you can see if it is called for wlan{0,1,2} and see what ns it is called for.

My current hypothesis is something is causing us to try and delete the symlink from the wrong namespace, so we just skip that part of it.

Quoted 4 lines · Eric W. Biederman

If you want to dig into this look at sysfs_delete_link. instrument it so that you can see if it is called for wlan{0,1,2} and see what ns it is called for.

Quoted 2 lines

My current hypothesis is something is causing us to try and delete the symlink from the wrong namespace, so we just skip that part of it.

[   78.253128] create link wlan0 ns=ffff88001ce1e600
...
[   93.462268] delete link wlan0 ns=ffff88001ce1e600

My current hypothesis is something is causing us to try and delete the symlink from the wrong namespace, so we just skip that part of it.

Quoted 2 lines · Eric W. Biederman

My current hypothesis is something is causing us to try and delete the symlink from the wrong namespace, so we just skip that part of it.

Hmm... ok:

[   70.338274] create link wlan2 ns=(null)

...

[   71.881775] delete link wlan2 ns=(null)
[   71.881777] hash_and_remove ffff88001f9563c0, (null), wlan2
[   71.881782] sd=ffff88001ce2d9c0, sdns=ffffffff8271c260

and thus we skip sysfs_remove_one() in sysfs_hash_and_remove() because sysfs_find_dirent() return an sd with a different ns than we passed in. Why is the ns we pass in NULL, shouldn't it be init_ns?

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]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

In the last painful restructuring of sysfs we created started creating class directories under normal devices so we could place devices such as network devices directly under their the hardware that implements them instead of in their class directories like /sys/class/net/.  This creation of class directories avoids the need to worry about namespace clonflicts if something is renamed.

A special exception was made for devices that were still placed directly in their class directory.  Looking at how this interacts with the wireless network devices it appears this special exception is either completely unneeded or at least needs to be restricted to a parent device with the same class as the child device.  Certainly in the case of unrelated classes we very much have the possibility of namespace classes and we should be creating the subdirectory.

Quoted 8 lines · Eric W. Biederman

In the last painful restructuring of sysfs we created started creating class directories under normal devices so we could place devices such as network devices directly under their the hardware that implements them instead of in their class directories like /sys/class/net/.  This creation of class directories avoids the need to worry about namespace clonflicts if something is renamed.

Quoted 7 lines

A special exception was made for devices that were still placed directly in their class directory.  Looking at how this interacts with the wireless network devices it appears this special exception is either completely unneeded or at least needs to be restricted to a parent device with the same class as the child device.  Certainly in the case of unrelated classes we very much have the possibility of namespace classes and we should be creating the subdirectory.

The class-glue-directories are only created between a bus-parent and and a class device. Class devices usually don't have other class devices as parents, that's why it wasn't done that way.

If people use class devices from other classes as parents, they should definitely convert the class that acts as a parent to a bus, to fit into the usual model. All that was really never meant to be used that way. The current behavior, to not to create the glue-directory is at least the intended one from the driver core's perspective.

What kind of classes do this, where this change would help or would be needed?

I don't mind trying if that change will work for people, I can't tell if there are any other users doing things like this which could break with such a change. Stuff like udev will be fine with directories inserted, but there are many things out there, that just access their parents attributes with ../../foo, which might no longer work when we insert directories.

Johannes this should fix your issue with mac80211_hwsim, where the device symlink were not destroyed when the driver was removed.

Quoted 2 lines · Eric W. Biederman

Johannes this should fix your issue with mac80211_hwsim, where the device symlink were not destroyed when the driver was removed.

It does, thank you.

FWIW, I'm happy changing hwsim too, but I don't think I quite understand what you're proposing in your other email so I'll leave it up to you since you now know what is causing the problem :)

Quoted 7 lines · Johannes Berg

Quoted 2 lines · Eric W. Biederman

Johannes this should fix your issue with mac80211_hwsim, where the device symlink were not destroyed when the driver was removed.

It does, thank you.

Quoted 3 lines

FWIW, I'm happy changing hwsim too, but I don't think I quite understand what you're proposing in your other email so I'll leave it up to you since you now know what is causing the problem :)

Assuming that hwsim is th parent of the network interface, it should us a "struct bus_type" not a "struct class" for the subsystem it assigns the devices to.

Classes should not be used for anything completely simple, at best not be used at all, they are just too simple. We never know about future requirements, which usually all go wrong with the non-extendable class logic.

The difference in the code to switch from class to bus should be minimal.

Quoted 8 lines · Kay Sievers

Quoted 7 lines · Johannes Berg

Quoted 2 lines · Eric W. Biederman

Johannes this should fix your issue with mac80211_hwsim, where the device symlink were not destroyed when the driver was removed.

It does, thank you.

Quoted 7 lines

Quoted 3 lines

FWIW, I'm happy changing hwsim too, but I don't think I quite understand what you're proposing in your other email so I'll leave it up to you since you now know what is causing the problem :)

Assuming that hwsim is th parent of the network interface, it should us a "struct bus_type" not a "struct class" for the subsystem it assigns the devices to.

It's all virtual, so yeah, I guess it is the parent? It currently creates a virtual struct device in the hwsim class and assigns that to the netdev parent indirectly via the wiphy or something like that.

Quoted 5 lines

Classes should not be used for anything completely simple, at best not be used at all, they are just too simple. We never know about future requirements, which usually all go wrong with the non-extendable class logic.

Quoted 1 line

The difference in the code to switch from class to bus should be minimal.

Does that mean cfg80211 (net/wireless/) should also not use a struct class? I'm not familiar with any of these details, mind helping me out?

Quoted 13 lines

Quoted 3 lines

Assuming that hwsim is th parent of the network interface, it should us a "struct bus_type" not a "struct class" for the subsystem it assigns the devices to.

It's all virtual, so yeah, I guess it is the parent? It currently creates a virtual struct device in the hwsim class and assigns that to the netdev parent indirectly via the wiphy or something like that.

Quoted 5 lines

Classes should not be used for anything completely simple, at best not be used at all, they are just too simple. We never know about future requirements, which usually all go wrong with the non-extendable class logic.

Quoted 4 lines

Quoted 1 line

The difference in the code to switch from class to bus should be minimal.

Does that mean cfg80211 (net/wireless/) should also not use a struct class? I'm not familiar with any of these details, mind helping me out?

Everything that might ever have a child device, or might ever need a sysfs attribute gobal to the subsystem must convert to a bus.

Actually there is not much reason to ever use "struct class" today. The layout for classes in /sys is not extendable like it is for buses which put all devices in a devices/ subdir and have the main subsystem directory to add custom things.

Quoted 4 lines · Kay Sievers

Actually there is not much reason to ever use "struct class" today. The layout for classes in /sys is not extendable like it is for buses which put all devices in a devices/ subdir and have the main subsystem directory to add custom things.

Ok, I don't get it.

For hwsim, we create entirely virtual "struct device"s. I think we just need that so userspace like network-manager doesn't get completely confused. BUT: device_create() needs a struct class parameter as the first parameter. So should we have a virtual class _and_ a virtual bus??

Similarly for wireless itself (net/wireless/), we use device_initialize() and set up the dev.class and dev.platform_data (not sure why platform data?) since this is a virtual abstraction. Basically we want to show in sysfs that

physical device

 +--- (virtual) wireless phy device 1
    +--- netdev 1
    +--- netdev 2
    +--- netdev 3
 +--- (virtual) wireless phy device 2
    +--- netdev 4

Although the second wireless phy device is only done by ath9k and will be removed soon. Still we should show that those netdevs all belong to the given wireless phy device.

So ... what should I do? How can I do device_create/device_initialize without a struct class?

And why a bus instead? It's not like there are devices that need to be discovered and bound to it etc.

There is no real difference between classes and buses. Actually we're working on merging them completely inside the kernel. Just declare a "struct bus_type" instead of a "struct class".

Quoted 3 lines · Kay Sievers

There is no real difference between classes and buses. Actually we're working on merging them completely inside the kernel. Just declare a "struct bus_type" instead of a "struct class".

Can you please tell me then how to device_create() without a class? I cannot seem to create devices without a class at all, even using manual allocation (yuck) and device_register crashes the kernel.

Quoted 3 lines

Can you please tell me then how to device_create() without a class? I cannot seem to create devices without a class at all, even using manual allocation (yuck) and device_register crashes the kernel.

Right, this "convenience API" does not exist for buses. It's not doing much, just allocates a "struct device" and fills in the few values and calls device_register().

Does your device create a device node? If not, device_create() should not be used anyway, because the corresponding device_destroy() will not do anything.

Quoted 11 lines · Kay Sievers

Quoted 3 lines

Can you please tell me then how to device_create() without a class? I cannot seem to create devices without a class at all, even using manual allocation (yuck) and device_register crashes the kernel.

Right, this "convenience API" does not exist for buses. It's not doing much, just allocates a "struct device" and fills in the few values and calls device_register().

Does your device create a device node? If not, device_create() should not be used anyway, because the corresponding device_destroy() will not do anything.

No, it doesn't need a dev node. I tried this:

+               data->dev = kzalloc(sizeof(struct device), GFP_KERNEL);
+               if (!data->dev) {
                       err = -ENOMEM;
                       goto failed_drvdata;
               }
+
+               dev_set_name(data->dev, "hwsim%d", i);
+               data->dev->bus = &hwsim_bus; data->dev->driver = &mac80211_hwsim_driver;

+               err = device_register(data->dev);
+               if (err) {
+                       printk(KERN_DEBUG
+                              "mac80211_hwsim: device_register failed (%d)\n",
+                              err);
+                       goto failed_drvdata;
+               }

(ignore the pluses, snipped from a patch) but it ran into a null ptr deref?

Quoted 18 lines

(quoted diff) +12 -0

+               data->dev = kzalloc(sizeof(struct device), GFP_KERNEL);
+               if (!data->dev) {
                       err = -ENOMEM;
                       goto failed_drvdata;
               }
+
+               dev_set_name(data->dev, "hwsim%d", i);
+               data->dev->bus = &hwsim_bus;
               data->dev->driver = &mac80211_hwsim_driver;
 
+               err = device_register(data->dev);
+               if (err) {
+                       printk(KERN_DEBUG
+                              "mac80211_hwsim: device_register failed (%d)\n",
+                              err);
+                       goto failed_drvdata;
+               }

Quoted 2 lines

(ignore the pluses, snipped from a patch) but it ran into a null ptr deref?

Oh, I see. It's probably something nobody ever did before. You try to register a bus device which has no parent. Seems that's something nobody ever expected to happen. :)

Your driver/subsystem is completely virtual, does not depend on any hardware, right? If we create a virtual parent, like:
parent = kzalloc(sizeof(struct device), GFP_KERNEL); dev_set_name(parent, "mac80211_hwsim"); device_register(parent);

An in your code:
data->dev->parent = parent;

That should give you a /sys/devices/mac80211_hwsim/ directory where all the devices you create should show up.

(mind you, I think we probably need to have the bus/driver assignment, but I wanted to try out your suggestion first)

So I removed bus/driver assignment from the above code just to try it out, and got

Device 'hwsim0' does not have a release() function, it is broken and must be fixed.

This has evolved far too much for me right now. Can we just apply the initial patch from Eric and be happier for a while? I can't justify spending this much time on it right now. Alternatively, you could look at hwsim too, since it's all virtual, nothing special is required, I do testing in a virtual machine ...

current patch is at
http://johannes.sipsolutions.net/patches/kernel/all/2010-06-07-11:41/hwsim-bus.patch

Quoted 2 lines · Johannes Berg

(mind you, I think we probably need to have the bus/driver assignment, but I wanted to try out your suggestion first)

Class device can never have a driver. And unregistered drivers can not be used, what you try to do here. That all should just be removed or properly registered with the core if needed.

Quoted 5 lines

So I removed bus/driver assignment from the above code just to try it out, and got

Device 'hwsim0' does not have a release() function, it is broken and must be fixed.

The driver core expects the resources of the device to be freed on release. You can provide an empty release function because you do this from a global list of devices.

Quoted 8 lines

This has evolved far too much for me right now. Can we just apply the initial patch from Eric and be happier for a while? I can't justify spending this much time on it right now. Alternatively, you could look at hwsim too, since it's all virtual, nothing special is required, I do testing in a virtual machine ...

current patch is at
http://johannes.sipsolutions.net/patches/kernel/all/2010-06-07-11:41/hwsim-bus.patch

Here is something that seems to work for me.

Quoted 7 lines · Kay Sievers

Quoted 2 lines · Johannes Berg

(mind you, I think we probably need to have the bus/driver assignment, but I wanted to try out your suggestion first)

Class device can never have a driver. And unregistered drivers can not be used, what you try to do here. That all should just be removed or properly registered with the core if needed.

Yeah but it shouldn't influence the operation?

Quoted 1 line

Here is something that seems to work for me.

I see you remove the driver. Does this mean that in sysfs these devices wouldn't have a driver symlink? ISTR that we needed that for userspace, but I'm not entirely sure, and I don't have all the relevant userspace in my test setup.

Quoted 4 lines

I see you remove the driver. Does this mean that in sysfs these devices wouldn't have a driver symlink? ISTR that we needed that for userspace, but I'm not entirely sure, and I don't have all the relevant userspace in my test setup.

Yeah, if you need the driver, you need to use bus device, as you do now. The driver needs to be registered with the core, and the probe function needs to tell to bind to the devices while registered. You can not manually assign this.

Quoted 5 lines · Kay Sievers

Quoted 4 lines

I see you remove the driver. Does this mean that in sysfs these devices wouldn't have a driver symlink? ISTR that we needed that for userspace, but I'm not entirely sure, and I don't have all the relevant userspace in my test setup.

Quoted 4 lines

Yeah, if you need the driver, you need to use bus device, as you do now. The driver needs to be registered with the core, and the probe function needs to tell to bind to the devices while registered. You can not manually assign this.

So I spoke to Jouni and he says that he doesn't remember needing the driver, so I guess we don't care. Can I get you to submit that patch properly? Send it to [email protected] please.

Quoted 12 lines · Johannes Berg

Quoted 9 lines · Kay Sievers

Quoted 4 lines

I see you remove the driver. Does this mean that in sysfs these devices wouldn't have a driver symlink? ISTR that we needed that for userspace, but I'm not entirely sure, and I don't have all the relevant userspace in my test setup.

Yeah, if you need the driver, you need to use bus device, as you do now. The driver needs to be registered with the core, and the probe function needs to tell to bind to the devices while registered. You can not manually assign this.

Quoted 3 lines

So I spoke to Jouni and he says that he doesn't remember needing the driver, so I guess we don't care. Can I get you to submit that patch properly? Send it to [email protected] please.

Sure, you mean the patch that changes the hwsim code, or the one I talked about to properly create a virtual bus parent?

Quoted 13 lines · Kay Sievers

Quoted 12 lines · Johannes Berg

Quoted 9 lines · Kay Sievers

Quoted 4 lines

I see you remove the driver. Does this mean that in sysfs these devices wouldn't have a driver symlink? ISTR that we needed that for userspace, but I'm not entirely sure, and I don't have all the relevant userspace in my test setup.

Yeah, if you need the driver, you need to use bus device, as you do now. The driver needs to be registered with the core, and the probe function needs to tell to bind to the devices while registered. You can not manually assign this.

Quoted 6 lines

Quoted 3 lines

So I spoke to Jouni and he says that he doesn't remember needing the driver, so I guess we don't care. Can I get you to submit that patch properly? Send it to [email protected] please.

Sure, you mean the patch that changes the hwsim code, or the one I talked about to properly create a virtual bus parent?

The hwsim one you pasted earlier.

Ok, this needs testing.

Please let me know, if that works for you so far.

Quoted 3 lines · Kay Sievers

Ok, this needs testing.

Please let me know, if that works for you so far.

Will do. Just a question: none of this seems to pin the module? So can I be sure if I rmmod the module that the release function will still be around etc.?

Quoted 6 lines · Johannes Berg

Quoted 3 lines · Kay Sievers

Ok, this needs testing.

Please let me know, if that works for you so far.

Quoted 3 lines

Will do. Just a question: none of this seems to pin the module? So can I be sure if I rmmod the module that the release function will still be around etc.?

Oh, sorry, that's something very specific to network devices, that the module can be unloaded while the devices it has created are still in use. I am not sure right now what's needed to make this work in a single module.

Quoted 7 lines · Kay Sievers

Quoted 6 lines · Johannes Berg

Quoted 3 lines · Kay Sievers

Ok, this needs testing.

Please let me know, if that works for you so far.

Quoted 8 lines

Quoted 3 lines

Will do. Just a question: none of this seems to pin the module? So can I be sure if I rmmod the module that the release function will still be around etc.?

Oh, sorry, that's something very specific to network devices, that the module can be unloaded while the devices it has created are still in use. I am not sure right now what's needed to make this work in a single module.

Well they will be unregistered and everything, but once all the netdevs are gone etc. the devices you create in the patch might stick around because somebody has them open in sysfs, and I see nothing that would pin the module in that case?

Quoted 4 lines

Well they will be unregistered and everything, but once all the netdevs are gone etc. the devices you create in the patch might stick around because somebody has them open in sysfs, and I see nothing that would pin the module in that case?

That's what I mean, I have no idea how to solve that with network devices. I don't think any other subsytem allows to unload the module, when devices, the module has created, are in use.

The current code uses the in-core class_create() logic, which was only meant for devices with a device node, and which is cleaned up by the core itself. That's why this issue never appeared before.

But as said, I have no idea how to solve that with a single module. It might not work at all without moving stuff into the core. That people use device_create() with no major/minor might indicate that something else is needed here. :)

Quoted 8 lines · Kay Sievers

Quoted 4 lines

Well they will be unregistered and everything, but once all the netdevs are gone etc. the devices you create in the patch might stick around because somebody has them open in sysfs, and I see nothing that would pin the module in that case?

That's what I mean, I have no idea how to solve that with network devices. I don't think any other subsytem allows to unload the module, when devices, the module has created, are in use.

You're right ... the would only be unregistered from your release, which would happen after the module is long gone ...

Quoted 4 lines

The current code uses the in-core class_create() logic, which was only meant for devices with a device node, and which is cleaned up by the core itself. That's why this issue never appeared before.

Quoted 4 lines

But as said, I have no idea how to solve that with a single module. It might not work at all without moving stuff into the core. That people use device_create() with no major/minor might indicate that something else is needed here. :)

So ... can we apply Eric's patch for now then?

Quoted 1 line

So ... can we apply Eric's patch for now then?

It might break other stuff we don't know about yet. Just like we did not know about what things hwsim is doing here. :)

The hwsim issues are caused by the current hwsim code, by doing things it should not do. Class devices of different classes must never be stacked (the core should not allow that in the first place). Class devices must never have a driver assigned behind its back. Also device_create() should not be used for devices without a major/minor (but that seems to be done in several other places too).

To fix the hwsim driver core interaction, core changes will probably be needed to allow network modules to be removed while their devices are active. That's something which seems not to work for bus devices currently.

Quoted 4 lines · Kay Sievers

Quoted 1 line

So ... can we apply Eric's patch for now then?

It might break other stuff we don't know about yet. Just like we did not know about what things hwsim is doing here. :)

True.

Quoted 6 lines

The hwsim issues are caused by the current hwsim code, by doing things it should not do. Class devices of different classes must never be stacked (the core should not allow that in the first place). Class devices must never have a driver assigned behind its back. Also device_create() should not be used for devices without a major/minor (but that seems to be done in several other places too).

Back when hwsim was written that would have been useful feedback to whoever did ... now, not so much.

Quoted 4 lines

To fix the hwsim driver core interaction, core changes will probably be needed to allow network modules to be removed while their devices are active. That's something which seems not to work for bus devices currently.

Well it just needs to pin the module refcount from the bus and/or device struct. That seems not too hard?

Quoted 7 lines

Quoted 4 lines

To fix the hwsim driver core interaction, core changes will probably be needed to allow network modules to be removed while their devices are active. That's something which seems not to work for bus devices currently.

Well it just needs to pin the module refcount from the bus and/or device struct. That seems not too hard?

How? If we ever get a reference of the module, it will not be able to be unloaded while stuff is active, right?

Quoted 8 lines · Kay Sievers

Quoted 7 lines

Quoted 4 lines

To fix the hwsim driver core interaction, core changes will probably be needed to allow network modules to be removed while their devices are active. That's something which seems not to work for bus devices currently.

Well it just needs to pin the module refcount from the bus and/or device struct. That seems not too hard?

Quoted 2 lines

How? If we ever get a reference of the module, it will not be able to be unloaded while stuff is active, right?

Hmm, true, but how does this work with class devices it has now? It seems to work fine now. It's because we have a generic release function there I guess?

That all works if you have two modules, like almost all buses have. That's what I meant, that we need to add stuff to the core to be able to cleanup bus devices internally too, if we use everything in a single module, which is also supposed to cleanup on unload, like the network devices like to do.

Quoted 5 lines · Kay Sievers

That all works if you have two modules, like almost all buses have. That's what I meant, that we need to add stuff to the core to be able to cleanup bus devices internally too, if we use everything in a single module, which is also supposed to cleanup on unload, like the network devices like to do.

Or some "wait for bus to be cleaned up" we can call in the module exit maybe?

That would block the rmmod process until the resources are cleaned up, wouldn't it?

The network devices can do this because the cleanup code is always compiled-in, for a module cleaning up itself, this is kind of complicated, isn't it?

Quoted 2 lines · Kay Sievers

That would block the rmmod process until the resources are cleaned up, wouldn't it?

Yes, would that be so bad?

It just needs a wait_for_bus_exit() function that the module calls in _exit?

In get_device_parent there is a 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.  Currently the only known way that we get a class device under a class device is due to the rearrangement of devices that happened when the new sysfs layout was introduced.

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 conservative bit of code.  Let's kill that bit of code.

There have been other proposals to fix this but they all have been more complicated, and none of them have actually resulted in working code.

Any userspace that works with both the old and the new sysfs layouts should not be affected by this change, and even if someone depends on it we are talking a very small number of drivers overall that are affected.

My apologoies for not fully catching this hole in the logic the when this code was originally added.

As mentioned earlier, It's pretty fragile to change things in this area, and I prefer the broken network driver-core interactions to be fixed instead - even when they are more complicated.

Quoted 3 lines · Kay Sievers

As mentioned earlier, It's pretty fragile to change things in this area, and I prefer the broken network driver-core interactions to be fixed instead - even when they are more complicated.

Can you _please_ offer a proper way to fix it then?

Quoted 6 lines · Johannes Berg

Quoted 3 lines · Kay Sievers

As mentioned earlier, It's pretty fragile to change things in this area, and I prefer the broken network driver-core interactions to be fixed instead - even when they are more complicated.

Quoted 1 line

Can you _please_ offer a proper way to fix it then?

Sorry, I have no real experience with the issues created by the assumption that network driver need to be able to get unloaded while in use. That's very special, always requires a compiled-into-the-kernel part of the subsystem, and makes it hard to work with, as we can not use any of the usual core infrastructure to solve that.

Quoted 7 lines · Kay Sievers

Quoted 6 lines · Johannes Berg

Quoted 3 lines · Kay Sievers

As mentioned earlier, It's pretty fragile to change things in this area, and I prefer the broken network driver-core interactions to be fixed instead - even when they are more complicated.

Quoted 8 lines

Quoted 1 line

Can you _please_ offer a proper way to fix it then?

Sorry, I have no real experience with the issues created by the assumption that network driver need to be able to get unloaded while in use. That's very special, always requires a compiled-into-the-kernel part of the subsystem, and makes it hard to work with, as we can not use any of the usual core infrastructure to solve that.

So please look at https://bugzilla.kernel.org/show_bug.cgi?id=16215

That simply creates and destroys the network device as things come and go.

I think the bnep case is much more serious because it is real hardware not a testing simulation, and it is the second instance of this.

Calling the change broken when I can boot up and run X in that configuration just fine is a vast overstatement.

Especially
when you don't acknowledge that the device layer is broken.

I will agree that insane amounts of backwards compatibility are a good idea.  So I will cook up a version of my patch that adds a hack to the device layer to only apply this change to devices of class net.

That should save let us postpone the architectural dreams for another day.

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 kludge.

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.

Signed-off-by Eric W. Biederman <[email protected]>

---
 drivers/base/core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/base/core.c +0 -0

index 9630fbd..ffb8443 100644

Quoted 5 lines

drivers/base/core.c +0 -0

index 9630fbd..ffb8443 100644
@@ -673,7 +673,7 @@ static struct kobject *get_device_parent(struct device *dev,

drivers/base/core.c static struct kobject *get_device_parent(struct device *dev, · line 676

Quoted 5 lines

drivers/base/core.c +1 -1

 		 */
 		if (parent == NULL)
 			parent_kobj = virtual_device_parent(dev);
-		else if (parent->class)
+		else if (parent->class && (strcmp(dev->class->name, "net") != 0))

No, we will not have subsystem specific hacks in the driver core like this. What would cause us to ever be able to delete this line?