sysfs warnings, reserved names

sysfs warnings, reserved names

[Denys Fedoryshchenko]

Hi

Just did a test:

ip link set dev eth1 name default

And got a lot of (expected) sysfs warnings:
[1068625.677143] sysctl table check failed: 
/net/ipv4/conf/default/promote_secondaries  Sysctl already exists
[1068625.677151] Pid: 18106, comm: ip Not tainted 3.2.4-centaur #1
and etc

Kernel 3.2.4, but i guess it doesn't matter much.
Maybe such names (as all and default) should be "reserved"?
Or it is ok like that?

Re: sysfs warnings, reserved names

[Eric Dumazet]

Le vendredi 17 février 2012 à 12:14 +0200, Denys Fedoryshchenko a
écrit :
> Hi
> 
> Just did a test:
> 
> ip link set dev eth1 name default
> 
> And got a lot of (expected) sysfs warnings:
> [1068625.677143] sysctl table check failed: 
> /net/ipv4/conf/default/promote_secondaries  Sysctl already exists
> [1068625.677151] Pid: 18106, comm: ip Not tainted 3.2.4-centaur #1
> and etc
> 
> Kernel 3.2.4, but i guess it doesn't matter much.
> Maybe such names (as all and default) should be "reserved"?
> Or it is ok like that?
> 

You're right, we should forbid this to happen.

Re: sysfs warnings, reserved names

[Eric Dumazet]

Le vendredi 17 février 2012 à 11:37 +0100, Eric Dumazet a écrit :
> Le vendredi 17 février 2012 à 12:14 +0200, Denys Fedoryshchenko a
> écrit :
> > Hi
> > 
> > Just did a test:
> > 
> > ip link set dev eth1 name default
> > 
> > And got a lot of (expected) sysfs warnings:
> > [1068625.677143] sysctl table check failed: 
> > /net/ipv4/conf/default/promote_secondaries  Sysctl already exists
> > [1068625.677151] Pid: 18106, comm: ip Not tainted 3.2.4-centaur #1
> > and etc
> > 
> > Kernel 3.2.4, but i guess it doesn't matter much.
> > Maybe such names (as all and default) should be "reserved"?
> > Or it is ok like that?
> > 
> 
> You're right, we should forbid this to happen.
> 
> 

Following is a quick hack, I CC Eric because its probaly better to
address this in sysfs.


diff --git a/net/core/dev.c b/net/core/dev.c
index 763a0ed..ec68f89 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -856,7 +856,11 @@ int dev_valid_name(const char *name)
 		return 0;
 	if (!strcmp(name, ".") || !strcmp(name, ".."))
 		return 0;
-
+#if defined(CONFIG_SYSCTL) && defined(CONFIG_INET)
+	/* /proc/sys/net/ipv{4|6}/conf/{default|all} are reserved */
+	if (!strcmp(name, "default") || !strcmp(name, "all"))
+		return 0;
+#endif
 	while (*name) {
 		if (*name == '/' || isspace(*name))
 			return 0;

Re: sysfs^H^H^H^H^Hsysctl warnings, reserved names

[Eric W. Biederman]

Eric Dumazet <eric.dumazet@gmail.com> writes:

> Le vendredi 17 février 2012 à 11:37 +0100, Eric Dumazet a écrit :
>> Le vendredi 17 février 2012 à 12:14 +0200, Denys Fedoryshchenko a
>> écrit :
>> > Hi
>> > 
>> > Just did a test:
>> > 
>> > ip link set dev eth1 name default
>> > 
>> > And got a lot of (expected) sysfs warnings:
>> > [1068625.677143] sysctl table check failed: 
>> > /net/ipv4/conf/default/promote_secondaries  Sysctl already exists
>> > [1068625.677151] Pid: 18106, comm: ip Not tainted 3.2.4-centaur #1
>> > and etc
>> > 
>> > Kernel 3.2.4, but i guess it doesn't matter much.
>> > Maybe such names (as all and default) should be "reserved"?
>> > Or it is ok like that?
>> > 
>> 
>> You're right, we should forbid this to happen.
>> 
>> 
>
> Following is a quick hack, I CC Eric because its probaly better to
> address this in sysfs.

Well we are talking about sysctl not sysfs but same difference, I keep
an on eye on them.

I expect renaming a network device to "all" or "default" would be a
problem in any kernel supporting renaming of networking devices.

At the basic level of handling it.  sysctl checks for this sometimes
now and as soon as my sysctl tree is merged the checks will become
unconditionally present.  In what sense were you thinking it would
be better to address this in the sysctl?

My feel on the situation is that since this is how the network stack
has choosen to use /proc/sys the names "all" and "default" should just
be outlawed unconditionally.

The CONFIG_INET check seems wrong, and CONFIG_SYSCTL seems unnecessary.
I think we should just reserve those names.

As for the rest the sysctl registration that happens during the rename
is failing so if you want to wire up catching that failure in the
networking code we can handle it that way but that seems like a lot
of work for very little benefit.  We don't test those failure paths
so if we put in the work to handle the error the code will probably
just bitrot and do something strange by the next time someone tries
to use "default" or "all" as device names.

By contrast outlawing the names as you do in dev_valid_name below is
absolutely trivial, and much less likely to bitrot.

Eric

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 763a0ed..ec68f89 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -856,7 +856,11 @@ int dev_valid_name(const char *name)
>  		return 0;
>  	if (!strcmp(name, ".") || !strcmp(name, ".."))
>  		return 0;
> -
> +#if defined(CONFIG_SYSCTL) && defined(CONFIG_INET)
> +	/* /proc/sys/net/ipv{4|6}/conf/{default|all} are reserved */
> +	if (!strcmp(name, "default") || !strcmp(name, "all"))
> +		return 0;
> +#endif
>  	while (*name) {
>  		if (*name == '/' || isspace(*name))
>  			return 0;
>
>

Re: sysfs^H^H^H^H^Hsysctl warnings, reserved names

[Eric Dumazet]

Le vendredi 17 février 2012 à 04:07 -0800, Eric W. Biederman a écrit :
> Eric Dumazet <eric.dumazet@gmail.com> writes:
> 
> > Le vendredi 17 février 2012 à 11:37 +0100, Eric Dumazet a écrit :
> >> Le vendredi 17 février 2012 à 12:14 +0200, Denys Fedoryshchenko a
> >> écrit :
> >> > Hi
> >> > 
> >> > Just did a test:
> >> > 
> >> > ip link set dev eth1 name default
> >> > 
> >> > And got a lot of (expected) sysfs warnings:
> >> > [1068625.677143] sysctl table check failed: 
> >> > /net/ipv4/conf/default/promote_secondaries  Sysctl already exists
> >> > [1068625.677151] Pid: 18106, comm: ip Not tainted 3.2.4-centaur #1
> >> > and etc
> >> > 
> >> > Kernel 3.2.4, but i guess it doesn't matter much.
> >> > Maybe such names (as all and default) should be "reserved"?
> >> > Or it is ok like that?
> >> > 
> >> 
> >> You're right, we should forbid this to happen.
> >> 
> >> 
> >
> > Following is a quick hack, I CC Eric because its probaly better to
> > address this in sysfs.
> 
> Well we are talking about sysctl not sysfs but same difference, I keep
> an on eye on them.
> 
> I expect renaming a network device to "all" or "default" would be a
> problem in any kernel supporting renaming of networking devices.
> 
> At the basic level of handling it.  sysctl checks for this sometimes
> now and as soon as my sysctl tree is merged the checks will become
> unconditionally present.  In what sense were you thinking it would
> be better to address this in the sysctl?
> 

Because problem is : it seems a rename() of "eth3" to "default" is
allowed by sysctl, yet "default" is already in directory.

It seems sysfs_rename_link() / sysfs_rename_dir() / sysfs_rename() might
have a problem ?

Sorry, I leave for vacations and can check all this before 5 days.

Re: sysfs^H^H^H^H^Hsysctl warnings, reserved names

[Eric W. Biederman]

Eric Dumazet <eric.dumazet@gmail.com> writes:

>> Well we are talking about sysctl not sysfs but same difference, I keep
>> an on eye on them.
>> 
>> I expect renaming a network device to "all" or "default" would be a
>> problem in any kernel supporting renaming of networking devices.
>> 
>> At the basic level of handling it.  sysctl checks for this sometimes
>> now and as soon as my sysctl tree is merged the checks will become
>> unconditionally present.  In what sense were you thinking it would
>> be better to address this in the sysctl?
>> 
>
> Because problem is : it seems a rename() of "eth3" to "default" is
> allowed by sysctl, yet "default" is already in directory.
>
> It seems sysfs_rename_link() / sysfs_rename_dir() / sysfs_rename() might
> have a problem ?

The files are under /proc/sys so we are talking about sysctls, which
means that the error is reported from register_net_sysctl_table.

The path is:
NETDEV_CHANGENAME
	devinet_sysctl_unregister() -> void
	devinet_sysctl_register() -> void
return NOTIFY_DONE;

Eric