Re: [Bugme-new] [Bug 16257] New: sysfs changes break hwsim and bnep drivers

Re: [Bugme-new] [Bug 16257] New: sysfs changes break hwsim and bnep drivers

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Sun, 20 Jun 2010 11:23:02 GMT
> bugzilla-daemon@bugzilla.kernel.org wrote:
>
>> https://bugzilla.kernel.org/show_bug.cgi?id=16257
>
> 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.

>From ada3a8448d376000e8cf586f0e4062e2f578c80f Mon Sep 17 00:00:00 2001
From: Eric W. Biederman <ebiederm@xmission.com>
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.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 drivers/base/core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

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;
-- 
1.6.5.2.143.g8cc62

Re: [Bugme-new] [Bug 16257] New: sysfs changes break hwsim and bnep drivers

[Greg KH]

On Mon, Jun 21, 2010 at 03:22:32PM -0700, Eric W. Biederman wrote:
> Andrew Morton <akpm@linux-foundation.org> writes:
> 
> > On Sun, 20 Jun 2010 11:23:02 GMT
> > bugzilla-daemon@bugzilla.kernel.org wrote:
> >
> >> https://bugzilla.kernel.org/show_bug.cgi?id=16257
> >
> > 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.

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

Re: [Bugme-new] [Bug 16257] New: sysfs changes break hwsim and bnep drivers

[Eric W. Biederman]

Greg KH <gregkh@suse.de> writes:

> 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

Re: [Bugme-new] [Bug 16257] New: sysfs changes break hwsim and bnep drivers

[Greg KH]

On Mon, Jun 21, 2010 at 03:55:35PM -0700, Eric W. Biederman wrote:
> Greg KH <gregkh@suse.de> writes:
> 
> > 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

Re: [Bugme-new] [Bug 16257] New: sysfs changes break hwsim and bnep drivers

[Eric W. Biederman]

Greg KH <gregkh@suse.de> writes:

> On Mon, Jun 21, 2010 at 03:55:35PM -0700, Eric W. Biederman wrote:
>> Greg KH <gregkh@suse.de> writes:
>> 
>> > 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?

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

Re: [Bugme-new] [Bug 16257] New: sysfs changes break hwsim and bnep drivers

[Greg KH]

On Mon, Jun 21, 2010 at 05:05:21PM -0700, Eric W. Biederman wrote:
> Greg KH <gregkh@suse.de> writes:
> 
> > On Mon, Jun 21, 2010 at 03:55:35PM -0700, Eric W. Biederman wrote:
> >> Greg KH <gregkh@suse.de> writes:
> >> 
> >> > 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?
> 
> 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.

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] sysfs: Don't allow the creation of symlinks we can't remove

[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/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.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 fs/sysfs/symlink.c |   23 ++++++++++++++++++-----
 1 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index f71246b..44bca5f 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -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)
-- 
1.6.5.2.143.g8cc62

[RFC][PATCH] mac80211_hwsim: No parent is better than an illegimate one.

[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 <ebiederm@xmission.com>
---

Johannes does this change look usable?

 drivers/net/wireless/mac80211_hwsim.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 6f8cb3e..b387222 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -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);
-- 
1.6.5.2.143.g8cc62

Re: [Bugme-new] [Bug 16257] New: sysfs changes break hwsim and bnep drivers

[Eric W. Biederman]

Greg KH <gregkh@suse.de> writes:

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

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

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

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

Re: [PATCH] sysfs: Don't allow the creation of symlinks we can't remove

[Greg KH]

On Thu, Jul 08, 2010 at 09:31:24AM -0700, Eric W. Biederman wrote:
> 
> 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.

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

[Eric W. Biederman]

Greg KH <greg@kroah.com> writes:

> 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

[Greg KH]

On Thu, Jul 08, 2010 at 03:28:53PM -0700, Eric W. Biederman wrote:
> Greg KH <greg@kroah.com> writes:
> 
> > 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.

Fair enough, I've commited it now, let's see what happens :)

> 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 ;(

Yeah, good luck with that...

greg k-h

Re: [PATCH] sysfs: Don't allow the creation of symlinks we can't remove

[Maciej W. Rozycki]

On Thu, 8 Jul 2010, Eric W. Biederman wrote:

> 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

Re: [RFC][PATCH] mac80211_hwsim: No parent is better than an illegimate one.

[Johannes Berg]

On Thu, 2010-07-08 at 09:37 -0700, Eric W. Biederman wrote:
> 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 <ebiederm@xmission.com>
> ---
> 
> 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

>  drivers/net/wireless/mac80211_hwsim.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> index 6f8cb3e..b387222 100644
> --- a/drivers/net/wireless/mac80211_hwsim.c
> +++ b/drivers/net/wireless/mac80211_hwsim.c
> @@ -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.

[Eric W. Biederman]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Thu, 2010-07-08 at 09:37 -0700, Eric W. Biederman wrote:
>> 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 <ebiederm@xmission.com>
>> ---
>> 
>> 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.

> 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

Re: [RFC][PATCH] mac80211_hwsim: No parent is better than an illegimate one.

[Johannes Berg]

On Mon, 2010-07-12 at 07:23 -0700, Eric W. Biederman wrote:

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

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

[Andrew Morton]

On Thu, 8 Jul 2010 16:06:01 -0700
Greg KH <greg@kroah.com> wrote:

> On Thu, Jul 08, 2010 at 03:28:53PM -0700, Eric W. Biederman wrote:
> > Greg KH <greg@kroah.com> writes:
> > 
> > > 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.
> 
> 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 .config 
CONFIG_E1000=y
CONFIG_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

[Greg KH]

On Mon, Jul 19, 2010 at 01:34:51PM -0700, Andrew Morton wrote:
> On Thu, 8 Jul 2010 16:06:01 -0700
> Greg KH <greg@kroah.com> wrote:
> 
> > On Thu, Jul 08, 2010 at 03:28:53PM -0700, Eric W. Biederman wrote:
> > > Greg KH <greg@kroah.com> writes:
> > > 
> > > > 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.
> > 
> > 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?

[PATCH 0/2] Support untagged symlinks to tagged directories.

[Eric W. Biederman]

Greg KH <gregkh@suse.de> writes:

> On Mon, Jul 19, 2010 at 01:34:51PM -0700, Andrew Morton wrote:
>> On Thu, 8 Jul 2010 16:06:01 -0700
>> Greg KH <greg@kroah.com> wrote:
>> 
>> > On Thu, Jul 08, 2010 at 03:28:53PM -0700, Eric W. Biederman wrote:
>> > > Greg KH <greg@kroah.com> writes:
>> > > 
>> > > > 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?
>> > > 
>> 
>> 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.

[Eric W. Biederman]

This happens for network devices when SYSFS_DEPRECATED is enabled.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 fs/sysfs/symlink.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index 44bca5f..6603833 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -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);
-- 
1.6.5.2.143.g8cc62

[PATCH 2/2] sysfs: allow creating symlinks from untagged to tagged directories

[Eric W. Biederman]

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.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 fs/sysfs/symlink.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index 6603833..a7ac78f 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -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
-- 
1.6.5.2.143.g8cc62

Re: [PATCH 0/2] Support untagged symlinks to tagged directories.

[Greg KH]

On Tue, Jul 20, 2010 at 10:08:27PM -0700, Eric W. Biederman wrote:
> 
> Greg KH <gregkh@suse.de> writes:
> 
> > On Mon, Jul 19, 2010 at 01:34:51PM -0700, Andrew Morton wrote:
> >> On Thu, 8 Jul 2010 16:06:01 -0700
> >> Greg KH <greg@kroah.com> wrote:
> >> 
> >> > On Thu, Jul 08, 2010 at 03:28:53PM -0700, Eric W. Biederman wrote:
> >> > > Greg KH <greg@kroah.com> writes:
> >> > > 
> >> > > > 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?
> >> > > 
> >> 
> >> 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.

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.

[Eric W. Biederman]

Greg KH <greg@kroah.com> writes:

> On Tue, Jul 20, 2010 at 10:08:27PM -0700, Eric W. Biederman wrote:
>> 
>> Greg KH <gregkh@suse.de> writes:
>> 
>> > On Mon, Jul 19, 2010 at 01:34:51PM -0700, Andrew Morton wrote:
>> >> On Thu, 8 Jul 2010 16:06:01 -0700
>> >> Greg KH <greg@kroah.com> wrote:
>> >> 
>> >> > On Thu, Jul 08, 2010 at 03:28:53PM -0700, Eric W. Biederman wrote:
>> >> > > Greg KH <greg@kroah.com> writes:
>> >> > > 
>> >> > > > 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?
>> >> > > 
>> >> 
>> >> 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.
>
> 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.

[Greg KH]

On Wed, Jul 21, 2010 at 01:20:28PM -0700, Eric W. Biederman wrote:
> Greg KH <greg@kroah.com> writes:
> 
> > On Tue, Jul 20, 2010 at 10:08:27PM -0700, Eric W. Biederman wrote:
> >> 
> >> Greg KH <gregkh@suse.de> writes:
> >> 
> >> > On Mon, Jul 19, 2010 at 01:34:51PM -0700, Andrew Morton wrote:
> >> >> On Thu, 8 Jul 2010 16:06:01 -0700
> >> >> Greg KH <greg@kroah.com> wrote:
> >> >> 
> >> >> > On Thu, Jul 08, 2010 at 03:28:53PM -0700, Eric W. Biederman wrote:
> >> >> > > Greg KH <greg@kroah.com> writes:
> >> >> > > 
> >> >> > > > 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?
> >> >> > > 
> >> >> 
> >> >> 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.
> >
> > 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'

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] Driver-core: Fix bluetooth network device rename regression

[Eric W. Biederman]

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(-)

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;
-- 
1.6.5.2.143.g8cc62

Re: [PATCH] sysfs: Don't allow the creation of symlinks we can't remove

[Johannes Berg]

On Thu, 2010-07-08 at 09:31 -0700, Eric W. Biederman wrote:
> 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.

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

[Eric W. Biederman]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Thu, 2010-07-08 at 09:31 -0700, Eric W. Biederman wrote:
>> 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.
>
> 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.

> 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

Re: [PATCH] sysfs: Don't allow the creation of symlinks we can't remove

[Johannes Berg]

On Thu, 2010-07-22 at 03:05 -0700, Eric W. Biederman wrote:

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

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

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

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  wlan2

johannes

Re: [PATCH] sysfs: Don't allow the creation of symlinks we can't remove

[Eric W. Biederman]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Thu, 2010-07-22 at 03:05 -0700, Eric W. Biederman wrote:
>
>> >> 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.

To be clear I just get hwsim0.  Not wlan0 or wlan1.

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

> 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

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/net for the network namespace
you are in.

Eric

Re: [PATCH] sysfs: Don't allow the creation of symlinks we can't remove

[Johannes Berg]

On Thu, 2010-07-22 at 03:35 -0700, Eric W. Biederman wrote:

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

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

> > 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
> 
> 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/net for 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(-)

--- wireless-testing.orig/include/linux/netdevice.h	2010-07-22 10:01:22.000000000 +0200
+++ wireless-testing/include/linux/netdevice.h	2010-07-22 10:11:00.000000000 +0200
@@ -2148,6 +2148,8 @@ extern void dev_seq_stop(struct seq_file
 extern 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);
--- wireless-testing.orig/net/core/net-sysfs.c	2010-07-22 10:01:22.000000000 +0200
+++ wireless-testing/net/core/net-sysfs.c	2010-07-22 10:11:51.000000000 +0200
@@ -785,12 +785,13 @@ static const void *net_netlink_ns(struct
 	return 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)
 {
--- wireless-testing.orig/net/wireless/sysfs.c	2010-07-22 10:01:22.000000000 +0200
+++ wireless-testing/net/wireless/sysfs.c	2010-07-22 10:13:08.000000000 +0200
@@ -110,6 +110,13 @@ static int wiphy_resume(struct device *d
 	return 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

[Eric W. Biederman]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Thu, 2010-07-22 at 03:35 -0700, Eric W. Biederman wrote:
>
>> >> 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.
>
>> > 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
>
>> > 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
>> 
>> 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/net for 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?

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


> ---
>  include/linux/netdevice.h |    2 ++
>  net/core/net-sysfs.c      |    3 ++-
>  net/wireless/sysfs.c      |    9 +++++++++
>  3 files changed, 13 insertions(+), 1 deletion(-)
>
> --- wireless-testing.orig/include/linux/netdevice.h	2010-07-22 10:01:22.000000000 +0200
> +++ wireless-testing/include/linux/netdevice.h	2010-07-22 10:11:00.000000000 +0200
> @@ -2148,6 +2148,8 @@ extern void dev_seq_stop(struct seq_file
>  extern 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);
> --- wireless-testing.orig/net/core/net-sysfs.c	2010-07-22 10:01:22.000000000 +0200
> +++ wireless-testing/net/core/net-sysfs.c	2010-07-22 10:11:51.000000000 +0200
> @@ -785,12 +785,13 @@ static const void *net_netlink_ns(struct
>  	return 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)
>  {
> --- wireless-testing.orig/net/wireless/sysfs.c	2010-07-22 10:01:22.000000000 +0200
> +++ wireless-testing/net/wireless/sysfs.c	2010-07-22 10:13:08.000000000 +0200
> @@ -110,6 +110,13 @@ static int wiphy_resume(struct device *d
>  	return 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

[Johannes Berg]

On Thu, 2010-07-22 at 04:27 -0700, Eric W. Biederman wrote:

> >> 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
> >
> >> > 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
> >> 
> >> 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/net for 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?
> 
> 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.

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

> My apologies for not thinking about the peculiarities of the wireless
> drivers.

No worries.

johannes

Re: [PATCH] Driver-core: Fix bluetooth network device rename regression

[Kay Sievers]

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

Re: [PATCH] Driver-core: Fix bluetooth network device rename regression

[Johannes Berg]

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

Re: [PATCH] Driver-core: Fix bluetooth network device rename regression

[Kay Sievers]

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

Re: [PATCH] Driver-core: Fix bluetooth network device rename regression

[Eric W. Biederman]

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

Re: [PATCH] Driver-core: Fix bluetooth network device rename regression

[Kay Sievers]

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

Re: [PATCH] Driver-core: Fix bluetooth network device rename regression

[Johannes Berg]

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

Re: [PATCH] Driver-core: Fix bluetooth network device rename regression

[Greg KH]

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

Re: [PATCH] Driver-core: Fix bluetooth network device rename regression

[Johannes Berg]

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

Re: [PATCH] Driver-core: Fix bluetooth network device rename regression

[Greg KH]

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

Re: [PATCH] Driver-core: Fix bluetooth network device rename regression

[Kay Sievers]

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

Re: [PATCH] Driver-core: Fix bluetooth network device rename regression

[Eric W. Biederman]

Greg KH <gregkh@suse.de> writes:

> 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 :)

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.

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

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 9630fbd..9b9d3bd 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 && !dev->class->ns_type)
 			return &parent->kobj;
 		else
 			parent_kobj = &parent->kobj;


Eric

[PATCH] Driver-core: Always create class directories for classses that support namespaces.

[Eric W. Biederman]

This 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".

Reported-by: Johannes Berg <johannes@sipsolutions.net>
Reported-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---

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(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 9630fbd..9b9d3bd 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 && !dev->class->ns_type)
 			return &parent->kobj;
 		else
 			parent_kobj = &parent->kobj;
-- 
1.6.5.2.143.g8cc62

Re: [PATCH] Driver-core: Always create class directories for classses that support namespaces.

[Greg KH]

On Sat, Jul 24, 2010 at 10:43:35PM -0700, Eric W. Biederman wrote:
> 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

[Greg KH]

On Thu, Jul 22, 2010 at 06:31:52PM -0700, Eric W. Biederman wrote:
> Greg KH <gregkh@suse.de> writes:
> 
> > 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 :)
> 
> 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.
> 
> > 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.

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

[Kay Sievers]

On Mon, Jul 26, 2010 at 20:09, Greg KH <greg@kroah.com> wrote:
>> 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

[Greg KH]

On Tue, Jul 27, 2010 at 11:10:26AM +0200, Kay Sievers wrote:
> On Mon, Jul 26, 2010 at 20:09, Greg KH <greg@kroah.com> wrote:
> >> 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.

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

Re: [PATCH] Driver-core: Fix bluetooth network device rename regression

[Johannes Berg]

On Tue, 2010-07-27 at 06:49 -0700, Greg KH wrote:

> 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

[Greg KH]

On Tue, Jul 27, 2010 at 03:59:50PM +0200, Johannes Berg wrote:
> On Tue, 2010-07-27 at 06:49 -0700, Greg KH wrote:
> 
> > 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

[Kay Sievers]

On Tue, Jul 27, 2010 at 17:09, Greg KH <greg@kroah.com> wrote:
> On Tue, Jul 27, 2010 at 03:59:50PM +0200, Johannes Berg wrote:
>> On Tue, 2010-07-27 at 06:49 -0700, Greg KH wrote:
>>
>> > 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.

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.

> 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

[Eric W. Biederman]

Greg KH <greg@kroah.com> writes:

> On Tue, Jul 27, 2010 at 11:10:26AM +0200, Kay Sievers wrote:
>> On Mon, Jul 26, 2010 at 20:09, Greg KH <greg@kroah.com> wrote:
>> >> 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!

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.

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

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

> 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

[Kay Sievers]

On Tue, Jul 27, 2010 at 20:17, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Greg KH <greg@kroah.com> writes:

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

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

[Greg KH]

On Tue, Jul 27, 2010 at 08:24:31PM +0200, Kay Sievers wrote:
> On Tue, Jul 27, 2010 at 20:17, Eric W. Biederman <ebiederm@xmission.com> wrote:
> > Greg KH <greg@kroah.com> writes:
> 
> > 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.
> 
> 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

[Eric W. Biederman]

Greg KH <greg@kroah.com> writes:

> On Tue, Jul 27, 2010 at 08:24:31PM +0200, Kay Sievers wrote:
>> On Tue, Jul 27, 2010 at 20:17, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> > Greg KH <greg@kroah.com> writes:
>> 
>> > 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.
>> 
>> 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?

> 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

[Kay Sievers]

On Tue, Jul 27, 2010 at 20:44, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Greg KH <greg@kroah.com> writes:
>> On Tue, Jul 27, 2010 at 08:24:31PM +0200, Kay Sievers wrote:
>>> On Tue, Jul 27, 2010 at 20:17, Eric W. Biederman <ebiederm@xmission.com> wrote:
>>> > Greg KH <greg@kroah.com> writes:
>>>
>>> > 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.
>>>
>>> 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

[Eric W. Biederman]

Kay Sievers <kay.sievers@vrfy.org> writes:

> On Tue, Jul 27, 2010 at 20:44, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> Greg KH <greg@kroah.com> writes:
>>> On Tue, Jul 27, 2010 at 08:24:31PM +0200, Kay Sievers wrote:
>>>> On Tue, Jul 27, 2010 at 20:17, Eric W. Biederman <ebiederm@xmission.com> wrote:
>>>> > Greg KH <greg@kroah.com> writes:
>>>>
>>>> > 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.
>>>>
>>>> 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.

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

[Kay Sievers]

On Tue, Jul 27, 2010 at 22:53, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Kay Sievers <kay.sievers@vrfy.org> writes:

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

> 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

[Eric W. Biederman]

Kay Sievers <kay.sievers@vrfy.org> writes:

> On Tue, Jul 27, 2010 at 22:53, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> Kay Sievers <kay.sievers@vrfy.org> writes:
>
>>> 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.

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.

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

That part seems reasonable.

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

> 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

[Kay Sievers]

On Wed, Jul 28, 2010 at 07:12, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Kay Sievers <kay.sievers@vrfy.org> writes:
>> On Tue, Jul 27, 2010 at 22:53, Eric W. Biederman <ebiederm@xmission.com> wrote:
>>> Kay Sievers <kay.sievers@vrfy.org> writes:
>>
>>>> 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.
>
> 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. :)

>> 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.
>
> That part seems reasonable.
>
>>> 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.

Sure, I meant, if the class-compat stuff is used when a conversion is done.

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

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

> 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

[Eric W. Biederman]

Kay Sievers <kay.sievers@vrfy.org> writes:

> 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. :)

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.


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

Sounds like a plan.  And now I'm off on vacation.

Have a good one.

Eric