bonding: cannot remove certain named devices

bonding: cannot remove certain named devices

[Bill Nottingham]

2.6.17-rc4+.

Trivial example:

# modprobe bonding (creates bond0)
# ip link set bond0 name "a b"
# echo "-a b" > /sys/class/net/bonding_masters 
bonding: unable to delete non-existent bond a
bash: echo: write error: No such device

Bill

Re: bonding: cannot remove certain named devices

[Mitch Williams]

On Tue, 15 Aug 2006, Bill Nottingham wrote:

> 2.6.17-rc4+.
>
> Trivial example:
>
> # modprobe bonding (creates bond0)
> # ip link set bond0 name "a b"
> # echo "-a b" > /sys/class/net/bonding_masters
> bonding: unable to delete non-existent bond a
> bash: echo: write error: No such device
>

Yuck.  The problem here is the space in the interface name, which the
sysfs code chokes on.  The code just does

	sscanf(buffer, "%16s", command); /* IFNAMSIZ*/

and goes from there.  Because of the space, it only gets the first half of
the interface name.

Suggestions?  Do we just omit the sscanf and copy up to the newline (or
EOF)?  Should there be another delimiter here?

Are spaces allowed in interface names anyway?  I can't believe that
bonding is the only area affected by this.

-Mitch

BTW this will also break if you try to add/remove a slave with a space in
the name through sysfs.

Re: bonding: cannot remove certain named devices

[Bill Nottingham]

Mitch Williams (mitch.a.williams@intel.com) said: 
> Are spaces allowed in interface names anyway?  I can't believe that
> bonding is the only area affected by this.

They're certainly allowed, and the sysfs directory structure, files,
etc. handle it ok. Userspace tends to break in a variety of ways.

I believe the only invalid character in an interface name is '/'.

Bill

Re: bonding: cannot remove certain named devices

[Stephen Hemminger]

On Tue, 15 Aug 2006 16:45:55 -0400
Bill Nottingham <notting@redhat.com> wrote:

> Mitch Williams (mitch.a.williams@intel.com) said: 
> > Are spaces allowed in interface names anyway?  I can't believe that
> > bonding is the only area affected by this.
> 
> They're certainly allowed, and the sysfs directory structure, files,
> etc. handle it ok. Userspace tends to break in a variety of ways.
> 
> I believe the only invalid character in an interface name is '/'.
> 

The names "." and ".." are also verboten.
Names with : in them are for IP aliases.

Re: bonding: cannot remove certain named devices

[Bill Nottingham]

Stephen Hemminger (shemminger@osdl.org) said: 
> > They're certainly allowed, and the sysfs directory structure, files,
> > etc. handle it ok. Userspace tends to break in a variety of ways.
> > 
> > I believe the only invalid character in an interface name is '/'.
> > 
> 
> The names "." and ".." are also verboten.

Right. Well, I suspect they're verboten-because-some-code-breaks-making-the-directory.

> Names with : in them are for IP aliases.

That's certainly not enforced at the kernel level.

Bill

Re: bonding: cannot remove certain named devices

[Mitch Williams]

On Tue, 15 Aug 2006, Bill Nottingham wrote:
>
> Stephen Hemminger (shemminger@osdl.org) said:
> > > They're certainly allowed, and the sysfs directory structure, files,
> > > etc. handle it ok. Userspace tends to break in a variety of ways.
> > >
> > > I believe the only invalid character in an interface name is '/'.
> > >
> >
> > The names "." and ".." are also verboten.
>
> Right. Well, I suspect they're verboten-because-some-code-breaks-making-the-directory.
>
> > Names with : in them are for IP aliases.
>

So can we use
	sscanf(buffer, " %[^\n]", command);
instead?  This should allow for whitespace in the filename.  Bad interface
names will be caught by the call to dev_valid_name().

(I think I'm reading the man page correctly.)

This could have the effect of making the parser way more finicky, though,
since we would allow trailing whitespace.  Technically I suppose it's
legal, but it's sure hard to see on the screen.

Anybody have a better solution?

-Mitch

Re: bonding: cannot remove certain named devices

[Stephen Hemminger]

On Tue, 15 Aug 2006 15:41:08 -0700
Mitch Williams <mitch.a.williams@intel.com> wrote:

> On Tue, 15 Aug 2006, Bill Nottingham wrote:
> >
> > Stephen Hemminger (shemminger@osdl.org) said:
> > > > They're certainly allowed, and the sysfs directory structure, files,
> > > > etc. handle it ok. Userspace tends to break in a variety of ways.
> > > >
> > > > I believe the only invalid character in an interface name is '/'.
> > > >
> > >
> > > The names "." and ".." are also verboten.
> >
> > Right. Well, I suspect they're verboten-because-some-code-breaks-making-the-directory.
> >
> > > Names with : in them are for IP aliases.
> >
> 
> So can we use
> 	sscanf(buffer, " %[^\n]", command);
> instead?  This should allow for whitespace in the filename.  Bad interface
> names will be caught by the call to dev_valid_name().
> 
> (I think I'm reading the man page correctly.)
> 
> This could have the effect of making the parser way more finicky, though,
> since we would allow trailing whitespace.  Technically I suppose it's
> legal, but it's sure hard to see on the screen.
> 
> Anybody have a better solution?
> 
> -Mitch

IMHO idiots who put space's in filenames should be ignored. As long as the
bonding code doesn't throw a fatal error, it has every right to return
"No such device" to the fool.

Re: bonding: cannot remove certain named devices

[David Miller]

From: Stephen Hemminger <shemminger@osdl.org>
Date: Tue, 15 Aug 2006 15:44:44 -0700

> IMHO idiots who put space's in filenames should be ignored. As long
> as the bonding code doesn't throw a fatal error, it has every right
> to return "No such device" to the fool.

I agree that whitespace in device names push the limits of sanity.

But if we believe that, we should enforce it in dev_valid_name().

Does anyone really mind if I add the whitespace check there?

Re: bonding: cannot remove certain named devices

[Stephen Hemminger]

On Tue, 15 Aug 2006 15:56:12 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@osdl.org>
> Date: Tue, 15 Aug 2006 15:44:44 -0700
> 
> > IMHO idiots who put space's in filenames should be ignored. As long
> > as the bonding code doesn't throw a fatal error, it has every right
> > to return "No such device" to the fool.
> 
> I agree that whitespace in device names push the limits of sanity.
> 
> But if we believe that, we should enforce it in dev_valid_name().
> 
> Does anyone really mind if I add the whitespace check there?

That is a good idea since it protects other interfaces from possible problems.

Re: bonding: cannot remove certain named devices

[Mitch Williams]

On Tue, 15 Aug 2006, David Miller wrote:
>
> I agree that whitespace in device names push the limits of sanity.
>
> But if we believe that, we should enforce it in dev_valid_name().
>
> Does anyone really mind if I add the whitespace check there?
>

I have no objections.  It would certainly make life easier.

OTOH, "push[ing] the limits of sanity" probably describes half of the
subscribers to netdev.  But that's neither here nor there.

-Mitch

Re: bonding: cannot remove certain named devices

[Bodo Eggert]

Stephen Hemminger <shemminger@osdl.org> wrote:

> IMHO idiots who put space's in filenames should be ignored. As long as the
> bonding code doesn't throw a fatal error, it has every right to return
> "No such device" to the fool.

Maybe you should limit device names to eight uppercase characters and up to
three characters extension, too. NOT! There is no reason to artificially
impose limitations on device names, so don't do that.
-- 
Ich danke GMX dafür, die Verwendung meiner Adressen mittels per SPF
verbreiteten Lügen zu sabotieren.

http://david.woodhou.se/why-not-spf.html

Re: bonding: cannot remove certain named devices

[David Miller]

From: Bodo Eggert <7eggert@elstempel.de>
Date: Wed, 16 Aug 2006 02:02:03 +0200

> Stephen Hemminger <shemminger@osdl.org> wrote:
> 
> > IMHO idiots who put space's in filenames should be ignored. As long as the
> > bonding code doesn't throw a fatal error, it has every right to return
> > "No such device" to the fool.
> 
> Maybe you should limit device names to eight uppercase characters and up to
> three characters extension, too. NOT! There is no reason to artificially
> impose limitations on device names, so don't do that.

Are you willing to work to add the special case code necessary to
handle whitespace characters in the device name over all of the kernel
code and also all of the userland tools too?

No?  Great, I'm glad that's settled.

Re: bonding: cannot remove certain named devices

[Giacomo A. Catenazzi]

David Miller wrote:
> From: Bodo Eggert <7eggert@elstempel.de>
> Date: Wed, 16 Aug 2006 02:02:03 +0200
> 
>> Stephen Hemminger <shemminger@osdl.org> wrote:
>>
>>> IMHO idiots who put space's in filenames should be ignored. As long as the
>>> bonding code doesn't throw a fatal error, it has every right to return
>>> "No such device" to the fool.
>> Maybe you should limit device names to eight uppercase characters and up to
>> three characters extension, too. NOT! There is no reason to artificially
>> impose limitations on device names, so don't do that.
> 
> Are you willing to work to add the special case code necessary to
> handle whitespace characters in the device name over all of the kernel
> code and also all of the userland tools too?

But if you don't handle spaces in userspace, you handle *, ?, [, ], $,
", ', \  in userspace? Should kernel disable also these (insane device
chars) chars?

ciao
	cate
> 
> No?  Great, I'm glad that's settled.

Re: bonding: cannot remove certain named devices

[Bill Nottingham]

Giacomo A. Catenazzi (cate@debian.org) said: 
> > Are you willing to work to add the special case code necessary to
> > handle whitespace characters in the device name over all of the kernel
> > code and also all of the userland tools too?
> 
> But if you don't handle spaces in userspace, you handle *, ?, [, ], $,
> ", ', \  in userspace? Should kernel disable also these (insane device
> chars) chars?

Don't forget unicode characters!

Seriously, while it might be insane to use some of these, I'm wondering
if trying to filter names is more work than fixing the tools.

Bill

Re: bonding: cannot remove certain named devices

[Giacomo A. Catenazzi]

Bill Nottingham wrote:
> Giacomo A. Catenazzi (cate@debian.org) said: 
>>> Are you willing to work to add the special case code necessary to
>>> handle whitespace characters in the device name over all of the kernel
>>> code and also all of the userland tools too?
>> But if you don't handle spaces in userspace, you handle *, ?, [, ], $,
>> ", ', \  in userspace? Should kernel disable also these (insane device
>> chars) chars?
> 
> Don't forget unicode characters!
> 
> Seriously, while it might be insane to use some of these, I'm wondering
> if trying to filter names is more work than fixing the tools.

Fixing tools in always the good approach, and I think in this case wrong
code is really a security problem. IMHO kernel cannot filter all bad 
strings.
So, if for the kernel part it is better to filter spaces, ok!
But we should use this user space problems as the motivation to filter
names.

ciao
	cate

Re: bonding: cannot remove certain named devices

[Bodo Eggert]

On Wed, 16 Aug 2006, Bill Nottingham wrote:
> Giacomo A. Catenazzi (cate@debian.org) said: 

> > > Are you willing to work to add the special case code necessary to
> > > handle whitespace characters in the device name over all of the kernel
> > > code and also all of the userland tools too?
> > 
> > But if you don't handle spaces in userspace, you handle *, ?, [, ], $,
> > ", ', \  in userspace? Should kernel disable also these (insane device
> > chars) chars?
> 
> Don't forget unicode characters!

And long names or control characters.

> Seriously, while it might be insane to use some of these, I'm wondering
> if trying to filter names is more work than fixing the tools.

I think it's sane to avoid control characters and unicode/iso*, since they
can interfere with log output or analysis. I only thought about the kernel
itself and the corresponding userspace tools, which should handle any
character sequence just fine or could be easily fixed.

-- 
Top 100 things you don't want the sysadmin to say:
14. Any more trouble from you and your account gets moved to the 750

Re: bonding: cannot remove certain named devices

[Xavier Bestel]

On Wed, 2006-08-16 at 17:11, Bodo Eggert wrote:
> On Wed, 16 Aug 2006, Bill Nottingham wrote:
> > Giacomo A. Catenazzi (cate@debian.org) said: 
> 
> > > > Are you willing to work to add the special case code necessary to
> > > > handle whitespace characters in the device name over all of the kernel
> > > > code and also all of the userland tools too?
> > > 
> > > But if you don't handle spaces in userspace, you handle *, ?, [, ], $,
> > > ", ', \  in userspace? Should kernel disable also these (insane device
> > > chars) chars?
> > 
> > Don't forget unicode characters!
> 
> And long names or control characters.
> 
> > Seriously, while it might be insane to use some of these, I'm wondering
> > if trying to filter names is more work than fixing the tools.
> 
> I think it's sane to avoid control characters and unicode/iso*, since they
> can interfere with log output or analysis. I only thought about the kernel
> itself and the corresponding userspace tools, which should handle any
> character sequence just fine or could be easily fixed.

Why not simply retricting chars to isalnum() ones ?

	Xav

Re: bonding: cannot remove certain named devices

[Bill Nottingham]

Xavier Bestel (xavier.bestel@free.fr) said: 
> > I think it's sane to avoid control characters and unicode/iso*, since they
> > can interfere with log output or analysis. I only thought about the kernel
> > itself and the corresponding userspace tools, which should handle any
> > character sequence just fine or could be easily fixed.
> 
> Why not simply retricting chars to isalnum() ones ?

People might want to use things like '-', '_', etc. 

Realistically, any filtering that is done with names has the chance of
breaking 'working' configs that date back to 2.4.

Bill

Re: bonding: cannot remove certain named devices

[David Miller]

From: Xavier Bestel <xavier.bestel@free.fr>
Date: Thu, 17 Aug 2006 09:29:43 +0200

> Why not simply retricting chars to isalnum() ones ?

As Bill said that would block things like "-" and "_" which are fine.

Bill also mentioned something about "breaking configs going back to
2.4.x" which is bogus because nothing broke when we started blocking
"/" and "." and ".." in networking device names during the addition of
sysfs support for net devices.

Nobody in their right mind puts a space in their network device name.

All you "name purists", go rename the block device name that is used
for your root partition to something with a space in it, and watch how
many startup scripts and command line invocations just explode.

There is absolutely no valid argument for allowing spaces in network
device names.

Re: bonding: cannot remove certain named devices

[Alan Cox]

Ar Iau, 2006-08-17 am 16:23 -0700, ysgrifennodd David Miller:
> Nobody in their right mind puts a space in their network device name.

It works fine. Been there done that. I'm probably not in my right mind
but it causes no problems. Nor btw does UTF-8 naming which is handy if
you want to name your devices in Japanese or Arabic...

> All you "name purists", go rename the block device name that is used
> for your root partition to something with a space in it

Works fine. It doesn't work fine for non root volumes (except by label)
because of the fstab format but root is ok !

Alan

Re: bonding: cannot remove certain named devices

[David Miller]

From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date: Fri, 18 Aug 2006 01:34:43 +0100

> Ar Iau, 2006-08-17 am 16:23 -0700, ysgrifennodd David Miller:
> > All you "name purists", go rename the block device name that is used
> > for your root partition to something with a space in it
> 
> Works fine. It doesn't work fine for non root volumes (except by label)
> because of the fstab format but root is ok !

Check out how your root device would be fsck'd.  The command line run
by the /etc/init* scripts either doesn't quote the device argument or
it consults fstab which as you said has limitations when dealing with
spaces.

It's either going to do something like $device (this is what debian
derived systems do), unquoted, or it will do "fsck -A" which runs into
said fstab format limitations (which is what fedora does).

Either way the point is that this issue is scattered all over the
place.

Preventing spaces in the name doesn't prevent the use of names in non-
romanized languages any moreso than preventing ".", "..", and "/"
already does.

Re: bonding: cannot remove certain named devices

[Bill Nottingham]

David Miller (davem@davemloft.net) said: 
> From: Xavier Bestel <xavier.bestel@free.fr>
> Date: Thu, 17 Aug 2006 09:29:43 +0200
> 
> > Why not simply retricting chars to isalnum() ones ?
> 
> As Bill said that would block things like "-" and "_" which are fine.
> 
> Bill also mentioned something about "breaking configs going back to
> 2.4.x" which is bogus because nothing broke when we started blocking
> "/" and "." and ".." in networking device names during the addition of
> sysfs support for net devices.

I was mainly referring to if we started to filter it out to isalnum() -
spaces/tab/CR etc. certainly could be filtered. (No idea what would
happen with unicode nbsp or other silly things.)

Bill

[PATCH] net: restrict device names from having whitespace

[Stephen Hemminger]

Don't allow spaces in network device names because it makes
it difficult to provide text interfaces via sysfs.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
---
 net/core/dev.c |   24 ++++++++++++++++++++----
 1 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index d95e262..56c8afb 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -79,6 +79,7 @@ #include <linux/capability.h>
 #include <linux/cpu.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
+#include <linux/ctype.h>
 #include <linux/sched.h>
 #include <linux/mutex.h>
 #include <linux/string.h>
@@ -636,10 +637,25 @@ struct net_device * dev_get_by_flags(uns
  */
 int dev_valid_name(const char *name)
 {
-	return !(*name == '\0' 
-		 || !strcmp(name, ".")
-		 || !strcmp(name, "..")
-		 || strchr(name, '/'));
+	if (*name == '\0')	 /* null string */
+		return 0;
+
+	if (*name == '.') {
+		if (name[1] == '\0')	/* can't have . in directory */
+			return 0;
+		if (name[1] == '.' && name[2] == '\0')
+			return 0;	/* or .. */
+	}
+
+	/* Check for blanks and slash because it confuses sysfs interfaces */
+	do {
+		if (*name == '/')
+			return 0;
+		if (isspace(*name))
+			return 0;
+	} while (*++name);
+
+	return 1;
 }
 
 /**
-- 
1.4.0

Re: [PATCH] net: restrict device names from having whitespace

[David Miller]

From: Stephen Hemminger <shemminger@osdl.org>
Date: Thu, 17 Aug 2006 23:11:27 -0700

> Don't allow spaces in network device names because it makes
> it difficult to provide text interfaces via sysfs.
> 
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

I have a patch which does this in my tree already Stephen.

Re: [PATCH] net: restrict device names from having whitespace

[Xavier Bestel]

On Fri, 2006-08-18 at 08:11, Stephen Hemminger wrote:
> Don't allow spaces in network device names because it makes
> it difficult to provide text interfaces via sysfs.

Personally I would at least avoid all chars <= ' ', because an interface
name is meant to be displayed and these control chars do no good on a
console nor in X.

	Xav

Re: [PATCH] net: restrict device names from having whitespace

[Xavier Bestel]

On Fri, 2006-08-18 at 09:17, Xavier Bestel wrote:
> On Fri, 2006-08-18 at 08:11, Stephen Hemminger wrote:
> > Don't allow spaces in network device names because it makes
> > it difficult to provide text interfaces via sysfs.
> 
> Personally I would at least avoid all chars <= ' ', because an interface
> name is meant to be displayed and these control chars do no good on a
> console nor in X.

Something like the following patch (short of a full in-kernel utf8
validator). That said it starts looking like policy in the kernel. Maybe
the "no space in devname" should just be enforced by some userspace
tool, not by the kernel itself ?

	Xav

diff --git a/net/core/dev.c b/net/core/dev.c
index d95e262..906cbf3 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -636,10 +636,23 @@ struct net_device * dev_get_by_flags(uns
  */
 int dev_valid_name(const char *name)
 {
-	return !(*name == '\0' 
-		 || !strcmp(name, ".")
-		 || !strcmp(name, "..")
-		 || strchr(name, '/'));
+	if(!*name)		/* empty string */
+		return 0;
+	if(*name == '.') {	/* . or .. */
+		if(!name[1])
+			return 0;
+		if(name[1] == '.' && !name[2])
+			return 0;
+	}
+	/* control char, space or slash */
+	while(*name) {
+		if(*name == '/')
+			return 0;
+		if(*name <= ' ')
+			return 0;
+		++name;
+	}
+	return 1;
 }
 
 /**

Re: bonding: cannot remove certain named devices

[Stephen Hemminger]

Bill Nottingham wrote:
> David Miller (davem@davemloft.net) said: 
>   
>> From: Xavier Bestel <xavier.bestel@free.fr>
>> Date: Thu, 17 Aug 2006 09:29:43 +0200
>>
>>     
>>> Why not simply retricting chars to isalnum() ones ?
>>>       
>> As Bill said that would block things like "-" and "_" which are fine.
>>
>> Bill also mentioned something about "breaking configs going back to
>> 2.4.x" which is bogus because nothing broke when we started blocking
>> "/" and "." and ".." in networking device names during the addition of
>> sysfs support for net devices.
>>     
>
> I was mainly referring to if we started to filter it out to isalnum() -
> spaces/tab/CR etc. certainly could be filtered. (No idea what would
> happen with unicode nbsp or other silly things.)
>
> Bill
>   
How just restrictiting to !isspace()