Patchwork [net-next,01/19] net: Implement register_net_sysctl.

login
register
mail settings
Submitter Eric W. Biederman
Date April 19, 2012, 11:18 p.m.
Message ID <m162cvia3s.fsf@fess.ebiederm.org>
Download mbox | patch
Permalink /patch/153881/
State Accepted
Delegated to: David Miller
Headers show

Comments

Eric W. Biederman - April 19, 2012, 11:18 p.m.
Right now all of the networking sysctl registrations are running in a
compatibiity mode.  The natvie sysctl registration api takes a cstring
for a path and a simple ctl_table.  Implement register_net_sysctl so
that we can register network sysctls without needing to use
compatiblity code in the sysctl core.

Switching from a ctl_path to a cstring results in less boiler plate
and denser code that is a little easier to read.

I would simply have changed the arguments to register_net_sysctl_table
instead of keeping two functions in parallel but gcc will allow a
ctl_path pointer to be passed to a char * pointer with only issuing a
warning resulting in completely incorrect code can be built.  Since I
have to change the function name I am taking advantage of the situation
to let both register_net_sysctl and register_net_sysctl_table live for a
short time in parallel which makes clean conversion patches a bit easier
to read and write.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 include/net/net_namespace.h |    2 ++
 net/sysctl_net.c            |    7 +++++++
 2 files changed, 9 insertions(+), 0 deletions(-)
Pavel Emelyanov - April 20, 2012, 5:27 a.m.
> @@ -117,6 +117,13 @@ struct ctl_table_header *register_net_sysctl_rotable(const
>  }
>  EXPORT_SYMBOL_GPL(register_net_sysctl_rotable);
>  
> +struct ctl_table_header *register_net_sysctl(struct net *net,
> +	const char *path, struct ctl_table *table)
> +{
> +	return __register_sysctl_table(&net->sysctls, path, table);

Eric, am I right, that after this all sysctl-s registered in init_net will
not be even visible in the non-init net namespaces?

If I'm not mistaken, before this all non-virtualized, i.e. "global" sysctls
were read-only in sub net namespaces and that solved lots of problems for us.

> +}
> +EXPORT_SYMBOL_GPL(register_net_sysctl);
> +
>  void unregister_net_sysctl_table(struct ctl_table_header *header)
>  {
>  	unregister_sysctl_table(header);

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric W. Biederman - April 20, 2012, 8:11 a.m.
Pavel Emelyanov <xemul@parallels.com> writes:

>> @@ -117,6 +117,13 @@ struct ctl_table_header *register_net_sysctl_rotable(const
>>  }
>>  EXPORT_SYMBOL_GPL(register_net_sysctl_rotable);
>>  
>> +struct ctl_table_header *register_net_sysctl(struct net *net,
>> +	const char *path, struct ctl_table *table)
>> +{
>> +	return __register_sysctl_table(&net->sysctls, path, table);
>
> Eric, am I right, that after this all sysctl-s registered in init_net will
> not be even visible in the non-init net namespaces?

Yes.

> If I'm not mistaken, before this all non-virtualized, i.e. "global" sysctls
> were read-only in sub net namespaces and that solved lots of problems for us.

Nope.  There are only 4 sysctls that were both global and read only, and
coincidentally I shoved them all into the initial network namespace in
patch 4.

So this part of the discussion really belongs about patch 4 but whatever.

In principle I don't mind the technique of sysctls that are writable
in the initial network namespace and readable everywhere else.  I hate
the name register_net_sysctl_rotable because it suggests that every
sysctl in the table will all be read-only or something like that.

In practice I think where we are at with converting and looking at
sysctls is disaster.

- People complain and want bad hacks so they can avoid writing to
  sysctls in containers but don't seem to work on the clean solutions.

- It is not discoverable which sysctls are per network namespace.

- We have only made a grand total 4 sysctls (in 3 tables) writable
  in the initial network namespace readable everywhere else.

So I think the best path forward is to just shove all sysctls that
aren't per network namespace into the initial network namespace so that
it is abundantly clear that they are not per network namespace, and
the fix the sysctls that people care about to be per network namespace.

I do admit their is actual interest in fixing some of the non-converted
netfliter sysctls.  So my perception of the situation may be wrong, but
right now I honestly think we have been too clever and no one knows what
is going on or cares enough to pay detailed attention.

Eric

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pavel Emelyanov - April 20, 2012, 8:45 a.m.
On 04/20/2012 12:11 PM, Eric W. Biederman wrote:
> Pavel Emelyanov <xemul@parallels.com> writes:
> 
>>> @@ -117,6 +117,13 @@ struct ctl_table_header *register_net_sysctl_rotable(const
>>>  }
>>>  EXPORT_SYMBOL_GPL(register_net_sysctl_rotable);
>>>  
>>> +struct ctl_table_header *register_net_sysctl(struct net *net,
>>> +	const char *path, struct ctl_table *table)
>>> +{
>>> +	return __register_sysctl_table(&net->sysctls, path, table);
>>
>> Eric, am I right, that after this all sysctl-s registered in init_net will
>> not be even visible in the non-init net namespaces?
> 
> Yes.
> 
>> If I'm not mistaken, before this all non-virtualized, i.e. "global" sysctls
>> were read-only in sub net namespaces and that solved lots of problems for us.
> 
> Nope.  There are only 4 sysctls that were both global and read only, and
> coincidentally I shoved them all into the initial network namespace in
> patch 4.

OK, thanks.

> So this part of the discussion really belongs about patch 4 but whatever.
> 
> In principle I don't mind the technique of sysctls that are writable
> in the initial network namespace and readable everywhere else.  I hate
> the name register_net_sysctl_rotable because it suggests that every
> sysctl in the table will all be read-only or something like that.
> 
> In practice I think where we are at with converting and looking at
> sysctls is disaster.
> 
> - People complain and want bad hacks so they can avoid writing to
>   sysctls in containers but don't seem to work on the clean solutions.
> 
> - It is not discoverable which sysctls are per network namespace.
> 
> - We have only made a grand total 4 sysctls (in 3 tables) writable
>   in the initial network namespace readable everywhere else.
> 
> So I think the best path forward is to just shove all sysctls that
> aren't per network namespace into the initial network namespace so that
> it is abundantly clear that they are not per network namespace, and
> the fix the sysctls that people care about to be per network namespace.

Agree.

> I do admit their is actual interest in fixing some of the non-converted
> netfliter sysctls.  So my perception of the situation may be wrong, but
> right now I honestly think we have been too clever and no one knows what
> is going on or cares enough to pay detailed attention.

We constantly see two types of problems with proc files and sysctls.
Various apps fail to work if they cannot do either of two

1. find some sysctl without trying to do anything with it
2. write to some sysctl without checking for the actual result

I don't see the ways of fixing any of the above in the generic way. However
step #1 can be ... work-around-ed by making all non-virtualized sysctls RO
in containers, but this is also not a perfect solution.

> Eric
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index ee547c1..446245e 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -287,6 +287,8 @@  extern struct ctl_table_header *register_net_sysctl_table(struct net *net,
 	const struct ctl_path *path, struct ctl_table *table);
 extern struct ctl_table_header *register_net_sysctl_rotable(
 	const struct ctl_path *path, struct ctl_table *table);
+extern struct ctl_table_header *register_net_sysctl(struct net *net,
+	const char *path, struct ctl_table *table);
 extern void unregister_net_sysctl_table(struct ctl_table_header *header);
 
 #endif /* __NET_NET_NAMESPACE_H */
diff --git a/net/sysctl_net.c b/net/sysctl_net.c
index c3e65ae..3865c4f 100644
--- a/net/sysctl_net.c
+++ b/net/sysctl_net.c
@@ -117,6 +117,13 @@  struct ctl_table_header *register_net_sysctl_rotable(const
 }
 EXPORT_SYMBOL_GPL(register_net_sysctl_rotable);
 
+struct ctl_table_header *register_net_sysctl(struct net *net,
+	const char *path, struct ctl_table *table)
+{
+	return __register_sysctl_table(&net->sysctls, path, table);
+}
+EXPORT_SYMBOL_GPL(register_net_sysctl);
+
 void unregister_net_sysctl_table(struct ctl_table_header *header)
 {
 	unregister_sysctl_table(header);