IPVS: Fix sysctl warnings about missing strategy

PATCH In review Christian Borntraeger 5 participants 10 messages Nov 13, 2007 – Nov 15, 2007

Subsystems (5)

Networking Network drivers IPv4/IPv6 Proc sysctl +1 more subsystems

lore

Share & tools

lore

Christian Borntraeger

author

2007-11-13 10:30 UTC #0 lore raw

PATCH 1/1 IPVS: Fix sysctl warnings about missing strategy

Running the latest git code I get the following messages during boot:
sysctl table check failed: /net/ipv4/vs/drop_entry .3.5.21.4 Missing strategy [...]
sysctl table check failed: /net/ipv4/vs/drop_packet .3.5.21.5 Missing strategy [...]
sysctl table check failed: /net/ipv4/vs/secure_tcp .3.5.21.6 Missing strategy [...]
sysctl table check failed: /net/ipv4/vs/sync_threshold .3.5.21.24 Missing strategy

I removed the binary sysctl handler for those messages and also removed the definitions in ip_vs.h. The alternative would be to implement a proper strategy handler, but syscall sysctl is deprecated.

There are other sysctl definitions that are commented out or work with the default sysctl_data strategy. I did not touch these.

Eric, IPVS team, are you ok with that change?

CC Eric W. Biederman <[email protected]>

CC Wensong Zhang <[email protected]>

CC Simon Horman <[email protected]>

CC Julian Anastasov <[email protected]>

Signed-off-by Christian Borntraeger <[email protected]>

---
 include/net/ip_vs.h       |    4 ----
 kernel/sysctl_check.c     |    4 ----
 net/ipv4/ipvs/ip_vs_ctl.c |    4 ----
 3 files changed, 12 deletions(-)

include/net/ip_vs.h +0 -4

Index: linux-2.6/include/net/ip_vs.h
===================================================================
@@ -336,9 +336,6 @@ enum {
 	NET_IPV4_VS_DEBUG_LEVEL=1,
 	NET_IPV4_VS_AMEMTHRESH=2,
 	NET_IPV4_VS_AMDROPRATE=3,
-	NET_IPV4_VS_DROP_ENTRY=4,
-	NET_IPV4_VS_DROP_PACKET=5,
-	NET_IPV4_VS_SECURE_TCP=6,
 	NET_IPV4_VS_TO_ES=7,
 	NET_IPV4_VS_TO_SS=8,
 	NET_IPV4_VS_TO_SR=9,

@@ -355,7 +352,6 @@ enum {
 	NET_IPV4_VS_LBLCR_EXPIRE=20,
 	NET_IPV4_VS_CACHE_BYPASS=22,
 	NET_IPV4_VS_EXPIRE_NODEST_CONN=23,
-	NET_IPV4_VS_SYNC_THRESHOLD=24,
 	NET_IPV4_VS_NAT_ICMP_SEND=25,
 	NET_IPV4_VS_EXPIRE_QUIESCENT_TEMPLATE=26,
 	NET_IPV4_VS_LAST

net/ipv4/ipvs/ip_vs_ctl.c +0 -4

Index: linux-2.6/net/ipv4/ipvs/ip_vs_ctl.c
===================================================================
@@ -1451,7 +1451,6 @@ static struct ctl_table vs_vars[] = {
 		.proc_handler	= &proc_dointvec,
 	},
 	{
-		.ctl_name	= NET_IPV4_VS_DROP_ENTRY,
 		.procname	= "drop_entry",
 		.data		= &sysctl_ip_vs_drop_entry,
 		.maxlen		= sizeof(int),

@@ -1459,7 +1458,6 @@ static struct ctl_table vs_vars[] = {
 		.proc_handler	= &proc_do_defense_mode,
 	},
 	{
-		.ctl_name	= NET_IPV4_VS_DROP_PACKET,
 		.procname	= "drop_packet",
 		.data		= &sysctl_ip_vs_drop_packet,
 		.maxlen		= sizeof(int),

@@ -1467,7 +1465,6 @@ static struct ctl_table vs_vars[] = {
 		.proc_handler	= &proc_do_defense_mode,
 	},
 	{
-		.ctl_name	= NET_IPV4_VS_SECURE_TCP,
 		.procname	= "secure_tcp",
 		.data		= &sysctl_ip_vs_secure_tcp,
 		.maxlen		= sizeof(int),

@@ -1597,7 +1594,6 @@ static struct ctl_table vs_vars[] = {
 		.proc_handler	= &proc_dointvec,
 	},
 	{
-		.ctl_name	= NET_IPV4_VS_SYNC_THRESHOLD,
 		.procname	= "sync_threshold",
 		.data		= &sysctl_ip_vs_sync_threshold,
 		.maxlen		= sizeof(sysctl_ip_vs_sync_threshold),

kernel/sysctl_check.c +0 -4

Index: linux-2.6/kernel/sysctl_check.c
===================================================================
@@ -242,9 +242,6 @@ static struct trans_ctl_table trans_net_
 	{ NET_IPV4_VS_AMEMTHRESH,	"amemthresh" },
 	{ NET_IPV4_VS_DEBUG_LEVEL,	"debug_level" },
 	{ NET_IPV4_VS_AMDROPRATE,	"am_droprate" },
-	{ NET_IPV4_VS_DROP_ENTRY,	"drop_entry" },
-	{ NET_IPV4_VS_DROP_PACKET,	"drop_packet" },
-	{ NET_IPV4_VS_SECURE_TCP,	"secure_tcp" },
 	{ NET_IPV4_VS_TO_ES,		"timeout_established" },
 	{ NET_IPV4_VS_TO_SS,		"timeout_synsent" },
 	{ NET_IPV4_VS_TO_SR,		"timeout_synrecv" },

@@ -260,7 +257,6 @@ static struct trans_ctl_table trans_net_
 	{ NET_IPV4_VS_CACHE_BYPASS,	"cache_bypass" },
 	{ NET_IPV4_VS_EXPIRE_NODEST_CONN,	"expire_nodest_conn" },
 	{ NET_IPV4_VS_EXPIRE_QUIESCENT_TEMPLATE,	"expire_quiescent_template" },
-	{ NET_IPV4_VS_SYNC_THRESHOLD,		"sync_threshold" },
 	{ NET_IPV4_VS_NAT_ICMP_SEND,	"nat_icmp_send" },
 	{ NET_IPV4_VS_LBLC_EXPIRE,		"lblc_expiration" },
 	{ NET_IPV4_VS_LBLCR_EXPIRE,		"lblcr_expiration" },

David Miller

maintainer

2007-11-13 10:45 UTC #1 lore raw

From: Christian Borntraeger <[email protected]>
Date: Tue, 13 Nov 2007 11:29:58 +0100

Quoted 16 lines

Running the latest git code I get the following messages during boot:
sysctl table check failed: /net/ipv4/vs/drop_entry .3.5.21.4 Missing strategy [...]
sysctl table check failed: /net/ipv4/vs/drop_packet .3.5.21.5 Missing strategy [...]
sysctl table check failed: /net/ipv4/vs/secure_tcp .3.5.21.6 Missing strategy [...]
sysctl table check failed: /net/ipv4/vs/sync_threshold .3.5.21.24 Missing strategy
I removed the binary sysctl handler for those messages and also removed the definitions in ip_vs.h. The alternative would be to implement a proper strategy handler, but syscall sysctl is deprecated.
There are other sysctl definitions that are commented out or work with the default sysctl_data strategy. I did not touch these.

Quoted 7 lines

Eric, IPVS team, are you ok with that change?
CC Eric W. Biederman <[email protected]>
CC Wensong Zhang <[email protected]>
CC Simon Horman <[email protected]>
CC Julian Anastasov <[email protected]>
Signed-off-by Christian Borntraeger <[email protected]>

Simon planned to make a similar change to dice all of this stuff up. He is travelling currently, and I think it's reasonable to give him some time to get to it.

Simon Horman

maintainer

2007-11-14 15:22 UTC #2 lore raw

Quoted 27 lines · David Miller

From: Christian Borntraeger <[email protected]>
Date: Tue, 13 Nov 2007 11:29:58 +0100
Quoted 16 lines
Running the latest git code I get the following messages during boot:
sysctl table check failed: /net/ipv4/vs/drop_entry .3.5.21.4 Missing strategy [...]
sysctl table check failed: /net/ipv4/vs/drop_packet .3.5.21.5 Missing strategy [...]
sysctl table check failed: /net/ipv4/vs/secure_tcp .3.5.21.6 Missing strategy [...]
sysctl table check failed: /net/ipv4/vs/sync_threshold .3.5.21.24 Missing strategy
I removed the binary sysctl handler for those messages and also removed the definitions in ip_vs.h. The alternative would be to implement a proper strategy handler, but syscall sysctl is deprecated.
There are other sysctl definitions that are commented out or work with the default sysctl_data strategy. I did not touch these.
Quoted 7 lines
Eric, IPVS team, are you ok with that change?
CC Eric W. Biederman <[email protected]>
CC Wensong Zhang <[email protected]>
CC Simon Horman <[email protected]>
CC Julian Anastasov <[email protected]>
Signed-off-by Christian Borntraeger <[email protected]>

Quoted 3 lines

Simon planned to make a similar change to dice all of this stuff up. He is travelling currently, and I think it's reasonable to give him some time to get to it.

Hi Christian, Hi Dave,

I have indeed been looking into this of late. Assuming that you use of CTL_UNNUMBERED is correct, this patch looks fine to me. Acked.

I was planning to do the same and also switch over all the other entries over to use CTL_UNNUMBERED, as its hard to imagine that anyone is using the sys_sysctl interface to IPVS.

As for the commented out entries. They are supposed to be exposed by some other means - I believe the thinking was to comply with the don't expose stuff in proc any more idea. Where is the best place to expose this kind of stuff?

Lastly, as Dave mentions, I'm travelling this week, so please excuse any slowness.

Signature · 3 lines

Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/

Eric W. Biederman

2007-11-14 16:01 UTC #3 lore raw

Quoted 10 lines · Simon Horman

Hi Christian, Hi Dave,
I have indeed been looking into this of late. Assuming that you use of CTL_UNNUMBERED is correct, this patch looks fine to me. Acked.
I was planning to do the same and also switch over all the other entries over to use CTL_UNNUMBERED, as its hard to imagine that anyone is using the sys_sysctl interface to IPVS.

Quoted 7 lines

As for the commented out entries. They are supposed to be exposed by some other means - I believe the thinking was to comply with the don't expose stuff in proc any more idea. Where is the best place to expose this kind of stuff?
Lastly, as Dave mentions, I'm travelling this week, so please excuse any slowness.

Looking at this patch it looks sane enough. Either removing ctl_name or explicitly using CTL_UNNUMBERED is fine. It may be wise to leave the binary entries in ip_vs.h and sysctl_check.c as documentation, but even there it doesn't much matter since we don't plan on adding more.

Eric

Julian Anastasov

maintainer

2007-11-15 00:38 UTC #4 lore raw

In reply to Simon Horman

Hello,

Quoted 21 lines · Simon Horman

Quoted 9 lines
Quoted 9 lines
Running the latest git code I get the following messages during boot:
sysctl table check failed: /net/ipv4/vs/drop_entry .3.5.21.4 Missing strategy [...]
sysctl table check failed: /net/ipv4/vs/drop_packet .3.5.21.5 Missing strategy [...]
sysctl table check failed: /net/ipv4/vs/secure_tcp .3.5.21.6 Missing strategy [...]
sysctl table check failed: /net/ipv4/vs/sync_threshold .3.5.21.24 Missing strategy
Quoted 7 lines
Quoted 7 lines
I removed the binary sysctl handler for those messages and also removed the definitions in ip_vs.h. The alternative would be to implement a proper strategy handler, but syscall sysctl is deprecated.
There are other sysctl definitions that are commented out or work with the default sysctl_data strategy. I did not touch these.
Hi Christian, Hi Dave,
I have indeed been looking into this of late. Assuming that you use of CTL_UNNUMBERED is correct, this patch looks fine to me. Acked.

Quoted 8 lines

I was planning to do the same and also switch over all the other entries over to use CTL_UNNUMBERED, as its hard to imagine that anyone is using the sys_sysctl interface to IPVS.
As for the commented out entries. They are supposed to be exposed by some other means - I believe the thinking was to comply with the don't expose stuff in proc any more idea. Where is the best place to expose this kind of stuff?

I assume /proc/sys is still valid place, only sysctl interface
is scheduled for removal. So, as long as these entries are not accessible from sysctl it is safe to run without strategy handler but if values can be changed then we will need strategy handler to properly call update_defense_level() as done in proc_do_defense_mode() as proc_handler. There could be side effects if new mode is not applied.

Regards

--
Julian Anastasov <[email protected]>

Eric W. Biederman

2007-11-15 01:12 UTC #5 lore raw

Quoted 2 lines · Julian Anastasov

I assume /proc/sys is still valid place, only sysctl interface
is scheduled for removal.

Yes. The ascii versions of the sysctls that show up in /proc/sys are definitely still valid.

Quoted 5 lines

So, as long as these entries are not
accessible from sysctl it is safe to run without strategy handler but if values can be changed then we will need strategy handler to properly call update_defense_level() as done in proc_do_defense_mode() as proc_handler. There could be side effects if new mode is not applied.

Yes. The current mode of 0644 allows them to be both read and updated with sys_sysctl. By removing the ctl_name entry those entries become inaccessible from the /proc/sys interface. Which is some easier then writing a strategy routine.

Eric

Simon Horman

maintainer

2007-11-15 01:14 UTC #6 lore raw

In reply to Julian Anastasov

On Thu, Nov 15, 2007 at 02:38:32AM +0200, Julian Anastasov wrote:

Hi Julian,

Quoted 1 line · Simon Horman

[snip]

Quoted 7 lines

Quoted 4 lines
As for the commented out entries. They are supposed to be exposed by some other means - I believe the thinking was to comply with the don't expose stuff in proc any more idea. Where is the best place to expose this kind of stuff?
I assume /proc/sys is still valid place, only sysctl interface
is scheduled for removal.

I'm happy to add them there, so long as that is a good place.

Quoted 5 lines

So, as long as these entries are not
accessible from sysctl it is safe to run without strategy handler but if values can be changed then we will need strategy handler to properly call update_defense_level() as done in proc_do_defense_mode() as proc_handler. There could be side effects if new mode is not applied.

I'm not sure what you are getting at there. I did write a stratergy for update_defense_level(), but I didn't post it, as I thought that it would not be needed if CTL_UNNUMBERED is used.

Signature · 1 line

Horms, California Edition

Eric W. Biederman

2007-11-15 01:25 UTC #7 lore raw

Quoted 13 lines · Simon Horman

On Thu, Nov 15, 2007 at 02:38:32AM +0200, Julian Anastasov wrote:
Hi Julian,
Quoted 1 line · Simon Horman
[snip]
Quoted 5 lines
Quoted 4 lines
As for the commented out entries. They are supposed to be exposed by some other means - I believe the thinking was to comply with the don't expose stuff in proc any more idea. Where is the best place to expose this kind of stuff?

Quoted 4 lines

Quoted 2 lines
I assume /proc/sys is still valid place, only sysctl interface
is scheduled for removal.
I'm happy to add them there, so long as that is a good place.

For simple integer values /proc/sys (ala the ascii sysctl interface) seems as good as any to me.

The binary interface is problematic because it doesn't get used and so we don't show proper discipline with binary integers leading to silent ABI changes, and the actual implementation of the handler routines get out of sync with the proc side giving us different meanings.

Quoted 9 lines

Quoted 5 lines
So, as long as these entries are not
accessible from sysctl it is safe to run without strategy handler but if values can be changed then we will need strategy handler to properly call update_defense_level() as done in proc_do_defense_mode() as proc_handler. There could be side effects if new mode is not applied.
I'm not sure what you are getting at there. I did write a stratergy for update_defense_level(), but I didn't post it, as I thought that it would not be needed if CTL_UNNUMBERED is used.

Strategy routines are never called if CTL_UNNUMBERED is used. So you should be safe just killing the ctl_name field or setting it explicitly to CTL_UNNUMBERED.

Eric

Simon Horman

maintainer

2007-11-15 01:33 UTC #8 lore raw

Quoted 15 lines · Eric W. Biederman

Quoted 13 lines · Simon Horman
On Thu, Nov 15, 2007 at 02:38:32AM +0200, Julian Anastasov wrote:
Hi Julian,
Quoted 1 line · Simon Horman
[snip]
Quoted 5 lines
Quoted 4 lines
As for the commented out entries. They are supposed to be exposed by some other means - I believe the thinking was to comply with the don't expose stuff in proc any more idea. Where is the best place to expose this kind of stuff?

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
I assume /proc/sys is still valid place, only sysctl interface
is scheduled for removal.
I'm happy to add them there, so long as that is a good place.
For simple integer values /proc/sys (ala the ascii sysctl interface) seems as good as any to me.

Understood.

Quoted 12 lines

The binary interface is problematic because it doesn't get used and so we don't show proper discipline with binary integers leading to silent ABI changes, and the actual implementation of the handler routines get out of sync with the proc side giving us different meanings.
Quoted 6 lines
Quoted 5 lines
So, as long as these entries are not
accessible from sysctl it is safe to run without strategy handler but if values can be changed then we will need strategy handler to properly call update_defense_level() as done in proc_do_defense_mode() as proc_handler. There could be side effects if new mode is not applied.

Quoted 7 lines

Quoted 3 lines
I'm not sure what you are getting at there. I did write a stratergy for update_defense_level(), but I didn't post it, as I thought that it would not be needed if CTL_UNNUMBERED is used.
Strategy routines are never called if CTL_UNNUMBERED is used. So you should be safe just killing the ctl_name field or setting it explicitly to CTL_UNNUMBERED.

Thanks Eric, thats more or less what I thought.

Signature · 1 line

Horms, California Edition

Julian Anastasov

maintainer

2007-11-15 22:53 UTC #9 lore raw

In reply to Simon Horman

Hello,

Quoted 9 lines · Simon Horman

Quoted 5 lines
So, as long as these entries are not
accessible from sysctl it is safe to run without strategy handler but if values can be changed then we will need strategy handler to properly call update_defense_level() as done in proc_do_defense_mode() as proc_handler. There could be side effects if new mode is not applied.
I'm not sure what you are getting at there. I did write a stratergy for update_defense_level(), but I didn't post it, as I thought that it would not be needed if CTL_UNNUMBERED is used.

Thanks everyone! I now see that by using CTL_UNNUMBERED
for ctl_name stops any writes to 'data', so there is no need for 'strategy' handler.

Regards

--
Julian Anastasov <[email protected]>