Skip to contentSkip to conversation

net: ipv4: add IPPROTO_ICMP socket kind

PATCH Superseded Vasiliy Kulikov 8 participants 24 messages Apr 9, 2011 – May 22, 2011

Subsystems (4)
Networking Network drivers IPv4/IPv6 Man-pages: manual pages for Linux -- Sections 2, 3, 4, 5, and 7
lore
Share & tools
lore

Vasiliy Kulikov

author
#0 lore raw

PATCH 1/1 net: ipv4: add IPPROTO_ICMP socket kind

This patch adds IPPROTO_ICMP socket kind. It makes it possible to send ICMP_ECHO messages and receive the corresponding ICMP_ECHOREPLY messages without any special privileges. In other words, the patch makes it possible to implement setuid-less and CAP_NET_RAW-less /bin/ping. In order not to increase the kernel's attack surface (in case of vulnerabilities in the newly added code), the new functionality is disabled by default, but is enabled at bootup by supporting Linux distributions, optionally with restriction to a group or a group range (see below).

Similar functionality is implemented in Mac OS X:
http://www.manpagez.com/man/4/icmp/

A new ping socket is created with

socket(PF_INET, SOCK_DGRAM, PROT_ICMP)

Message identifiers (octets 4-5 of ICMP header) are interpreted as local ports. Addresses are stored in struct sockaddr_in. No port numbers are reserved for privileged processes, port 0 is reserved for API ("let the kernel pick a free number"). There is no notion of remote ports, remote port numbers provided by the user (e.g. in connect()) are ignored.

Data sent and received include ICMP headers. This is deliberate to:
1) Avoid the need to transport headers values like sequence numbers by other means.
2) Make it easier to port existing programs using raw sockets.

ICMP headers given to send() are checked and sanitized. The type must be ICMP_ECHO and the code must be zero (future extensions might relax this, see below). The id is set to the number (local port) of the socket, the checksum is always recomputed.

ICMP reply packets received from the network are demultiplexed according to their id's, and are returned by recv() without any modifications. IP header information and ICMP errors of those packets may be obtained via ancillary data (IP_RECVTTL, IP_RETOPTS, and IP_RECVERR). ICMP source quenches and redirects are reported as fake errors via the error queue (IP_RECVERR); the next hop address for redirects is saved to ee_info (in network order).

socket(2) is restricted to the group range specified in "/proc/sys/net/ipv4/ping_group_range". It is "1 0" by default, meaning that nobody (not even root) may create ping sockets. Setting it to "100 100" would grant permissions to the single group, "0 65535" would enable it for the world.

The existing code might be (in the unlikely case anyone needs it) extended rather easily to handle other similar pairs of ICMP messages (Timestamp/Reply, Information Request/Reply, Address Mask Request/Reply etc.).

Userspace ping util & patch for it:
http://openwall.info/wiki/segoon/ping

A revision of this patch (for RHEL5/OpenVZ kernels) is in use in Owl-current, such as in the 2011/03/12 LiveCD ISOs:
http://mirrors.kernel.org/openwall/Owl/current/iso/

For Openwall GNU/*/Linux it is the last step on the road to the setuid-less distro.

Initially this functionality was written by Pavel Kankovsky (CC'ed him) for linux 2.4.32, but unfortunately it was never made public.

Reference to the previous discussion:
http://lwn.net/Articles/420801/

All ping options (-b, -p, -Q, -R, -s, -t, -T, -M, -I), are tested with the patch.

Changes since RFCv2:
- fixed checksumming bug.
- CAP_NET_RAW may not create icmp sockets anymore.

Changes since RFCv1:
- minor cleanups.
- introduced sysctl'able group range to restrict socket(2).

Signed-off-by Vasiliy Kulikov <[email protected]>
---
 include/net/netns/ipv4.h   |    2 +
 include/net/ping.h         |   69 ++++
 net/ipv4/Kconfig           |   21 +
 net/ipv4/Makefile          |    1 +
 net/ipv4/af_inet.c         |   36 ++
 net/ipv4/icmp.c            |   14 +-
 net/ipv4/ping.c            |  933 ++++++++++++++++++++++++++++++++++++++++++++
 net/ipv4/sysctl_net_ipv4.c |   90 +++++
 8 files changed, 1165 insertions(+), 1 deletions(-)
 create mode 100644 include/net/ping.h
 create mode 100644 net/ipv4/ping.c
include/net/netns/ipv4.h +2 -0
index d68c3f1..ff3bb61 100644
@@ -55,6 +55,8 @@ struct netns_ipv4 {
 	int sysctl_rt_cache_rebuild_count;
 	int current_rt_cache_rebuild_count;
 
+	unsigned int sysctl_ping_group_range[2];
+
 	atomic_t rt_genid;
 
 #ifdef CONFIG_IP_MROUTE
include/net/ping.h new file +69 -0

69 lines not on this page · raw message

net/ipv4/Kconfig +21 -0
index a5a1050..cf64f35 100644
@@ -14,6 +14,27 @@ config IP_MULTICAST
 	  <file:Documentation/networking/multicast.txt>. For most people, it's
 	  safe to say N.
 
+config IP_PING
+	bool "IP: ping socket"
+	depends on EXPERIMENTAL
+	help
+	  This option introduces a new kind of sockets - "ping sockets".
+
+	  A ping socket makes it possible to send ICMP Echo messages and receive
+	  corresponding ICMP Echo Reply messages without any special privileges.
+	  In other words, it makes is possible to implement setuid-less /bin/ping.
+
+	  A new ping socket is created with socket(PF_INET, SOCK_DGRAM, PROT_ICMP).
+
+config IP_PING_DEBUG
+	bool "IP: ping socket debug output"
+	depends on IP_PING
+	default n
+	help
+	  Enable the inclusion of debug code in the ICMP ping sockets.
+	  Be aware that doing this will impact performance.
+	  If unsure say N.
+
 config IP_ADVANCED_ROUTER
 	bool "IP: advanced router"
 	---help---
net/ipv4/Makefile +1 -0
index 4978d22..3a37479 100644
@@ -19,6 +19,7 @@ obj-$(CONFIG_IP_FIB_TRIE) += fib_trie.o
 obj-$(CONFIG_PROC_FS) += proc.o
 obj-$(CONFIG_IP_MULTIPLE_TABLES) += fib_rules.o
 obj-$(CONFIG_IP_MROUTE) += ipmr.o
+obj-$(CONFIG_IP_PING) += ping.o
 obj-$(CONFIG_NET_IPIP) += ipip.o
 obj-$(CONFIG_NET_IPGRE_DEMUX) += gre.o
 obj-$(CONFIG_NET_IPGRE) += ip_gre.o
net/ipv4/af_inet.c +36 -0
index 45b89d7..a707d3e 100644
@@ -105,6 +105,7 @@
 #include <net/tcp.h>
 #include <net/udp.h>
 #include <net/udplite.h>
+#include <net/ping.h>
 #include <linux/skbuff.h>
 #include <net/sock.h>
 #include <net/raw.h>

@@ -1008,6 +1009,16 @@ static struct inet_protosw inetsw_array[] =
 		.flags =      INET_PROTOSW_PERMANENT,
        },
 
+#ifdef CONFIG_IP_PING
+       {
+		.type =       SOCK_DGRAM,
+		.protocol =   IPPROTO_ICMP,
+		.prot =       &ping_prot,
+		.ops =        &inet_dgram_ops,
+		.no_check =   UDP_CSUM_DEFAULT,
+		.flags =      INET_PROTOSW_REUSE,
+       },
+#endif
 
        {
 	       .type =       SOCK_RAW,

@@ -1528,6 +1539,9 @@ static const struct net_protocol udp_protocol = {
 
 static const struct net_protocol icmp_protocol = {
 	.handler =	icmp_rcv,
+#ifdef CONFIG_IP_PING
+	.err_handler =	ping_err,
+#endif
 	.no_policy =	1,
 	.netns_ok =	1,
 };

@@ -1643,6 +1657,12 @@ static int __init inet_init(void)
 	if (rc)
 		goto out_unregister_udp_proto;
 
+#ifdef CONFIG_IP_PING
+	rc = proto_register(&ping_prot, 1);
+	if (rc)
+		goto out_unregister_raw_proto;
+#endif
+
 	/*
 	 *	Tell SOCKET that we are alive...
 	 */

@@ -1698,6 +1718,10 @@ static int __init inet_init(void)
 	/* Add UDP-Lite (RFC 3828) */
 	udplite4_register();
 
+#ifdef CONFIG_IP_PING
+	ping_init();
+#endif
+
 	/*
 	 *	Set the ICMP layer up
 	 */

@@ -1728,6 +1752,10 @@ static int __init inet_init(void)
 	rc = 0;
 out:
 	return rc;
+#ifdef CONFIG_IP_PING
+out_unregister_raw_proto:
+	proto_unregister(&raw_prot);
+#endif
 out_unregister_udp_proto:
 	proto_unregister(&udp_prot);
 out_unregister_tcp_proto:

@@ -1752,11 +1780,19 @@ static int __init ipv4_proc_init(void)
 		goto out_tcp;
 	if (udp4_proc_init())
 		goto out_udp;
+#ifdef CONFIG_IP_PING
+	if (ping_proc_init())
+		goto out_ping;
+#endif
 	if (ip_misc_proc_init())
 		goto out_misc;
 out:
 	return rc;
 out_misc:
+#ifdef CONFIG_IP_PING
+	ping_proc_exit();
+out_ping:
+#endif
 	udp4_proc_exit();
 out_udp:
 	tcp4_proc_exit();
net/ipv4/icmp.c +13 -1
index 4aa1b7f..7a52374 100644
@@ -83,6 +83,7 @@
 #include <net/tcp.h>
 #include <net/udp.h>
 #include <net/raw.h>
+#include <net/ping.h>
 #include <linux/skbuff.h>
 #include <net/sock.h>
 #include <linux/errno.h>

@@ -798,6 +799,17 @@ static void icmp_redirect(struct sk_buff *skb)
 			       iph->saddr, skb->dev);
 		break;
 	}
+
+#ifdef CONFIG_IP_PING
+	/* Ping wants to see redirects.
+         * Let's pretend they are errors of sorts... */
+	if (iph->protocol == IPPROTO_ICMP &&
+	    iph->ihl >= 5 &&
+	    pskb_may_pull(skb, (iph->ihl<<2)+8)) {
+		ping_err(skb, icmp_hdr(skb)->un.gateway);
+	}
+#endif
+
 out:
 	return;
 out_err:

@@ -1058,7 +1070,7 @@ error:
  */
 static const struct icmp_control icmp_pointers[NR_ICMP_TYPES + 1] = {
 	[ICMP_ECHOREPLY] = {
-		.handler = icmp_discard,
+		.handler = icmp_echoreply,
 	},
 	[1] = {
 		.handler = icmp_discard,
net/ipv4/ping.c new file +933 -0

933 lines not on this page · raw message

net/ipv4/sysctl_net_ipv4.c +90 -0

128 lines not on this page · raw message

Signature · 1 line
1.7.0.4
Acked-by on v1 1/1
#1 lore raw
Quoted 8 lines · Vasiliy Kulikov

This patch adds IPPROTO_ICMP socket kind. It makes it possible to send ICMP_ECHO messages and receive the corresponding ICMP_ECHOREPLY messages without any special privileges. In other words, the patch makes it possible to implement setuid-less and CAP_NET_RAW-less /bin/ping. In order not to increase the kernel's attack surface (in case of vulnerabilities in the newly added code), the new functionality is disabled by default, but is enabled at bootup by supporting Linux distributions, optionally with restriction to a group or a group range

...

Quoted 2 lines

For Openwall GNU/*/Linux it is the last step on the road to the setuid-less distro.

More correctly, it _was_ the last step - we've already taken it, so a revision of the patch (against OpenVZ/RHEL5 kernels) is currently in use.

We would really like this accepted into mainline, which is why Vasiliy spends extra effort to keep the patch updated to current mainline kernels and re-test it. If there are any comments/concerns/objections, we'd be happy to hear those.

Quoted 1 line
Signed-off-by Vasiliy Kulikov <[email protected]>
Acked-by Solar Designer <[email protected]>
Quoted 9 lines
 include/net/netns/ipv4.h   |    2 +
 include/net/ping.h         |   69 ++++
 net/ipv4/Kconfig           |   21 +
 net/ipv4/Makefile          |    1 +
 net/ipv4/af_inet.c         |   36 ++
 net/ipv4/icmp.c            |   14 +-
 net/ipv4/ping.c            |  933 ++++++++++++++++++++++++++++++++++++++++++++
 net/ipv4/sysctl_net_ipv4.c |   90 +++++
 8 files changed, 1165 insertions(+), 1 deletions(-)

Thanks,

Alexander

maintainer
#2 lore raw

From: Solar Designer <[email protected]>
Date: Tue, 12 Apr 2011 09:06:59 +0400

Quoted 16 lines · Vasiliy Kulikov
Quoted 8 lines

This patch adds IPPROTO_ICMP socket kind. It makes it possible to send ICMP_ECHO messages and receive the corresponding ICMP_ECHOREPLY messages without any special privileges. In other words, the patch makes it possible to implement setuid-less and CAP_NET_RAW-less /bin/ping. In order not to increase the kernel's attack surface (in case of vulnerabilities in the newly added code), the new functionality is disabled by default, but is enabled at bootup by supporting Linux distributions, optionally with restriction to a group or a group range

...

Quoted 2 lines

For Openwall GNU/*/Linux it is the last step on the road to the setuid-less distro.

More correctly, it _was_ the last step - we've already taken it, so a revision of the patch (against OpenVZ/RHEL5 kernels) is currently in use.

Quoted 8 lines

We would really like this accepted into mainline, which is why Vasiliy spends extra effort to keep the patch updated to current mainline kernels and re-test it. If there are any comments/concerns/objections, we'd be happy to hear those.

Quoted 1 line
Signed-off-by Vasiliy Kulikov <[email protected]>
Acked-by Solar Designer <[email protected]>

I have no fundamental objections to this change and I'll be happy to apply it after we iron out a few details.

First, please get rid of the debug option, we have pr_debug() which can be dynamically turned on and off at run time these days.

Second, if this is a bonafide core facility we'd like everyone to use, let's make it so. I want it so that every ping binary can expect this facility to be there if the kernel is new enough.

So let's get rid of the config option.

Third, either we trust this code or we do not. If we are OK with a user application spamming whatever they wish out of a datagram UDP socket, they can do no more harm with this thing unless there are bugs.

The group range thing I also consider hackish. In my opinion two other approaches seem more reasonable:

1) On/Off sysctl, default to ON. This is to handle the "oh crap there's a really bad bug discovered in this thing" situations.

2) A single group ID, if zero it means "all groups" else it limits the facility to specific groups.

I would mention capabilities, but probably that's undesirable for something like this as it creeps us back to the original problem this is trying to resolve.

Finally, longer term, I'd really like to see ipv6 support for this feature as well. I absolutely am not requiring that ipv6 get worked on right now just to apply the ipv4 variant.

So let's sort out the ipv4 side issues so I can get this into the net-next-2.6 tree and people can start testing it.

Thanks.

author
#3 lore raw
Quoted 2 lines · David Miller

I have no fundamental objections to this change and I'll be happy to apply it after we iron out a few details.

Great!

Quoted 2 lines

First, please get rid of the debug option, we have pr_debug() which can be dynamically turned on and off at run time these days.

OK.

Quoted 5 lines

Second, if this is a bonafide core facility we'd like everyone to use, let's make it so. I want it so that every ping binary can expect this facility to be there if the kernel is new enough.

So let's get rid of the config option.

OK.

Quoted 2 lines

Finally, longer term, I'd really like to see ipv6 support for this feature as well.

Definitely.

For ICMPv6 we should recognize what message types are OK to send by non-root users and what types are privileged. We just didn't do this yet.

Thank you for the review,

Signature · 2 lines
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
author
#4 lore raw
In reply to David Miller
Quoted 4 lines · David Miller

Third, either we trust this code or we do not. If we are OK with a user application spamming whatever they wish out of a datagram UDP socket, they can do no more harm with this thing unless there are bugs.

It is true in theory, but wrong in practice. I have a cheap router which can be made almost fully hang up with simple ping flood. And I almost sure many not very widespread implementations of IPv6 would react not very clever way on non-echo ICMPv6 flood (I'd want to make more than ICMPv6 Echo Request/Reply types available to nonroot).

Quoted 1 line

The group range thing I also consider hackish.

Why hackish? We'd want to leave group range sysctl. With this thing you may restrict icmp according to different policies:

1) 0 4294967295 - We trust all users in the system.

2) 0 0 - We don't trust users, root only.

3) 101 4294967295 - We trust real users, but don't trust daemons.

4) 109 109 - We trust a signle group. Either /sbin/ping is g+s and owned by this group (like in Owl) or it is a group of "network admins" who is allowed to flood.

5) 200 300 - We trust users in this range. Little sense because of (4), but possible.

Minor note about sgid'ed /sbin/ping: in case of a vulnerability in this kernel code one has to find additional bug in ping binary to exploit this vulnerability (unless it is somehow triggerable with ping arguments overflow or remotely).

Thank you,

Signature · 2 lines
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
author
#5 lore raw

PATCH v2 1/1 net: ipv4: add IPPROTO_ICMP socket kind

In reply to David Miller

This patch adds IPPROTO_ICMP socket kind. It makes it possible to send ICMP_ECHO messages and receive the corresponding ICMP_ECHOREPLY messages without any special privileges. In other words, the patch makes it possible to implement setuid-less and CAP_NET_RAW-less /bin/ping. In order not to increase the kernel's attack surface (in case of vulnerabilities in the newly added code), the new functionality is disabled by default, but is enabled at bootup by supporting Linux distributions, optionally with restriction to a group or a group range (see below).

Similar functionality is implemented in Mac OS X:
http://www.manpagez.com/man/4/icmp/

A new ping socket is created with

socket(PF_INET, SOCK_DGRAM, PROT_ICMP)

Message identifiers (octets 4-5 of ICMP header) are interpreted as local ports. Addresses are stored in struct sockaddr_in. No port numbers are reserved for privileged processes, port 0 is reserved for API ("let the kernel pick a free number"). There is no notion of remote ports, remote port numbers provided by the user (e.g. in connect()) are ignored.

Data sent and received include ICMP headers. This is deliberate to:
1) Avoid the need to transport headers values like sequence numbers by other means.
2) Make it easier to port existing programs using raw sockets.

ICMP headers given to send() are checked and sanitized. The type must be ICMP_ECHO and the code must be zero (future extensions might relax this, see below). The id is set to the number (local port) of the socket, the checksum is always recomputed.

ICMP reply packets received from the network are demultiplexed according to their id's, and are returned by recv() without any modifications. IP header information and ICMP errors of those packets may be obtained via ancillary data (IP_RECVTTL, IP_RETOPTS, and IP_RECVERR). ICMP source quenches and redirects are reported as fake errors via the error queue (IP_RECVERR); the next hop address for redirects is saved to ee_info (in network order).

socket(2) is restricted to the group range specified in "/proc/sys/net/ipv4/ping_group_range". It is "1 0" by default, meaning that nobody (not even root) may create ping sockets. Setting it to "100 100" would grant permissions to the single group (to either make /sbin/ping g+s and owned by this group or to grant permissions to the "netadmins" group), "0 4294967295" would enable it for the world, "100 4294967295" would enable it for the users, but not daemons.

The existing code might be (in the unlikely case anyone needs it) extended rather easily to handle other similar pairs of ICMP messages (Timestamp/Reply, Information Request/Reply, Address Mask Request/Reply etc.).

Userspace ping util & patch for it:
http://openwall.info/wiki/people/segoon/ping

For Openwall GNU/*/Linux it was the last step on the road to the setuid-less distro. A revision of this patch (for RHEL5/OpenVZ kernels) is in use in Owl-current, such as in the 2011/03/12 LiveCD ISOs:
http://mirrors.kernel.org/openwall/Owl/current/iso/

Initially this functionality was written by Pavel Kankovsky for Linux 2.4.32, but unfortunately it was never made public.

All ping options (-b, -p, -Q, -R, -s, -t, -T, -M, -I), are tested with the patch.

PATCH v2:
- changed ping_debug() to pr_debug().
- removed CONFIG_IP_PING.
- removed ping_seq_fops.owner field (unused for procfs).
- switched to proc_net_fops_create().
- switched to %pK.

PATCH v1:
- fixed checksumming bug.
- CAP_NET_RAW may not create icmp sockets anymore.

RFC v2:
- minor cleanups.
- introduced sysctl'able group range to restrict socket(2).

Signed-off-by Vasiliy Kulikov <[email protected]>
Acked-by Solar Designer <[email protected]>
---
 include/net/netns/ipv4.h   |    2 +
 include/net/ping.h         |   57 +++
 net/ipv4/Makefile          |    2 +-
 net/ipv4/af_inet.c         |   22 +
 net/ipv4/icmp.c            |   12 +-
 net/ipv4/ping.c            |  929 ++++++++++++++++++++++++++++++++++++++++++++
 net/ipv4/sysctl_net_ipv4.c |   80 ++++
 7 files changed, 1102 insertions(+), 2 deletions(-)
 create mode 100644 include/net/ping.h
 create mode 100644 net/ipv4/ping.c
include/net/netns/ipv4.h +2 -0
index d68c3f1..ff3bb61 100644
@@ -55,6 +55,8 @@ struct netns_ipv4 {
 	int sysctl_rt_cache_rebuild_count;
 	int current_rt_cache_rebuild_count;
 
+	unsigned int sysctl_ping_group_range[2];
+
 	atomic_t rt_genid;
 
 #ifdef CONFIG_IP_MROUTE
include/net/ping.h new file +57 -0

57 lines not on this page · raw message

net/ipv4/Makefile +1 -1
index 4978d22..01b0349 100644
@@ -11,7 +11,7 @@ obj-y     := route.o inetpeer.o protocol.o \
 	     datagram.o raw.o udp.o udplite.o \
 	     arp.o icmp.o devinet.o af_inet.o  igmp.o \
 	     fib_frontend.o fib_semantics.o \
-	     inet_fragment.o
+	     inet_fragment.o ping.o
 
 obj-$(CONFIG_SYSCTL) += sysctl_net_ipv4.o
 obj-$(CONFIG_IP_FIB_HASH) += fib_hash.o
net/ipv4/af_inet.c +22 -0
index 45b89d7..d2b225e 100644
@@ -105,6 +105,7 @@
 #include <net/tcp.h>
 #include <net/udp.h>
 #include <net/udplite.h>
+#include <net/ping.h>
 #include <linux/skbuff.h>
 #include <net/sock.h>
 #include <net/raw.h>

@@ -1008,6 +1009,14 @@ static struct inet_protosw inetsw_array[] =
 		.flags =      INET_PROTOSW_PERMANENT,
        },
 
+       {
+		.type =       SOCK_DGRAM,
+		.protocol =   IPPROTO_ICMP,
+		.prot =       &ping_prot,
+		.ops =        &inet_dgram_ops,
+		.no_check =   UDP_CSUM_DEFAULT,
+		.flags =      INET_PROTOSW_REUSE,
+       },
 
        {
 	       .type =       SOCK_RAW,

@@ -1528,6 +1537,7 @@ static const struct net_protocol udp_protocol = {
 
 static const struct net_protocol icmp_protocol = {
 	.handler =	icmp_rcv,
+	.err_handler =	ping_err,
 	.no_policy =	1,
 	.netns_ok =	1,
 };

@@ -1643,6 +1653,10 @@ static int __init inet_init(void)
 	if (rc)
 		goto out_unregister_udp_proto;
 
+	rc = proto_register(&ping_prot, 1);
+	if (rc)
+		goto out_unregister_raw_proto;
+
 	/*
 	 *	Tell SOCKET that we are alive...
 	 */

@@ -1698,6 +1712,8 @@ static int __init inet_init(void)
 	/* Add UDP-Lite (RFC 3828) */
 	udplite4_register();
 
+	ping_init();
+
 	/*
 	 *	Set the ICMP layer up
 	 */

@@ -1728,6 +1744,8 @@ static int __init inet_init(void)
 	rc = 0;
 out:
 	return rc;
+out_unregister_raw_proto:
+	proto_unregister(&raw_prot);
 out_unregister_udp_proto:
 	proto_unregister(&udp_prot);
 out_unregister_tcp_proto:

@@ -1752,11 +1770,15 @@ static int __init ipv4_proc_init(void)
 		goto out_tcp;
 	if (udp4_proc_init())
 		goto out_udp;
+	if (ping_proc_init())
+		goto out_ping;
 	if (ip_misc_proc_init())
 		goto out_misc;
 out:
 	return rc;
 out_misc:
+	ping_proc_exit();
+out_ping:
 	udp4_proc_exit();
 out_udp:
 	tcp4_proc_exit();
net/ipv4/icmp.c +11 -1
index 4aa1b7f..51e5c41 100644
@@ -83,6 +83,7 @@
 #include <net/tcp.h>
 #include <net/udp.h>
 #include <net/raw.h>
+#include <net/ping.h>
 #include <linux/skbuff.h>
 #include <net/sock.h>
 #include <linux/errno.h>

@@ -798,6 +799,15 @@ static void icmp_redirect(struct sk_buff *skb)
 			       iph->saddr, skb->dev);
 		break;
 	}
+
+	/* Ping wants to see redirects.
+         * Let's pretend they are errors of sorts... */
+	if (iph->protocol == IPPROTO_ICMP &&
+	    iph->ihl >= 5 &&
+	    pskb_may_pull(skb, (iph->ihl<<2)+8)) {
+		ping_err(skb, icmp_hdr(skb)->un.gateway);
+	}
+
 out:
 	return;
 out_err:

@@ -1058,7 +1068,7 @@ error:
  */
 static const struct icmp_control icmp_pointers[NR_ICMP_TYPES + 1] = {
 	[ICMP_ECHOREPLY] = {
-		.handler = icmp_discard,
+		.handler = ping_rcv,
 	},
 	[1] = {
 		.handler = icmp_discard,
net/ipv4/ping.c new file +929 -0

929 lines not on this page · raw message

net/ipv4/sysctl_net_ipv4.c +80 -0

118 lines not on this page · raw message

Signature · 1 line
1.7.0.4
maintainer
#6 lore raw

From: Vasiliy Kulikov <[email protected]>
Date: Tue, 10 May 2011 22:09:59 +0400

In net-next-2.6 we're trying to get rid of uses of route identity information, and also the types used for flow lookup keys are completely different. This code won't compile as-is.

net/ipv4/ping.c line 534

Quoted 2 lines
net/ipv4/ping.c new file +2 -0
+	{
+		struct flowi fl = { .oif = ipc.oif,

This should be "struct flowi4 fl4", declare it at the top level of the function so you can get at the fully resolved key values later in this function.

Then use "flowi4_init_output(...) to initialize the flow instead of this explicit assignment.

net/ipv4/ping.c line 565

Quoted 2 lines
net/ipv4/ping.c new file +2 -0
+	if (!ipc.addr)
+		ipc.addr = rt->rt_dst;

Replase rt->rt_dst with fl4.daddr

net/ipv4/ping.c line 579

Quoted 3 lines
net/ipv4/ping.c new file +3 -0
+	err = ip_append_data(sk, ping_getfrag, &pfh, len,
+			0, &ipc, &rt,
+			msg->msg_flags);

ip_append_data() now takes a flowi4 key pointer as an argument, so you'll need to pass "&fl4" in.

A lot has changed in this area, your code won't even compile, so please adjust your patch to fit net-next-2.6 as needed, perhaps using net/ipv4/raw.c and net/ipv4/udp.c as a guide.

Thanks.

author
#7 lore raw
Quoted 3 lines · David Miller

A lot has changed in this area, your code won't even compile, so please adjust your patch to fit net-next-2.6 as needed, perhaps using net/ipv4/raw.c and net/ipv4/udp.c as a guide.

Sure, will do it.

Thanks!

Signature · 2 lines
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
author
#8 lore raw

PATCH v3 1/1 net: ipv4: add IPPROTO_ICMP socket kind

In reply to David Miller

This patch adds IPPROTO_ICMP socket kind. It makes it possible to send ICMP_ECHO messages and receive the corresponding ICMP_ECHOREPLY messages without any special privileges. In other words, the patch makes it possible to implement setuid-less and CAP_NET_RAW-less /bin/ping. In order not to increase the kernel's attack surface, the new functionality is disabled by default, but is enabled at bootup by supporting Linux distributions, optionally with restriction to a group or a group range (see below).

Similar functionality is implemented in Mac OS X:
http://www.manpagez.com/man/4/icmp/

A new ping socket is created with

socket(PF_INET, SOCK_DGRAM, PROT_ICMP)

Message identifiers (octets 4-5 of ICMP header) are interpreted as local ports. Addresses are stored in struct sockaddr_in. No port numbers are reserved for privileged processes, port 0 is reserved for API ("let the kernel pick a free number"). There is no notion of remote ports, remote port numbers provided by the user (e.g. in connect()) are ignored.

Data sent and received include ICMP headers. This is deliberate to:
1) Avoid the need to transport headers values like sequence numbers by other means.
2) Make it easier to port existing programs using raw sockets.

ICMP headers given to send() are checked and sanitized. The type must be ICMP_ECHO and the code must be zero (future extensions might relax this, see below). The id is set to the number (local port) of the socket, the checksum is always recomputed.

ICMP reply packets received from the network are demultiplexed according to their id's, and are returned by recv() without any modifications. IP header information and ICMP errors of those packets may be obtained via ancillary data (IP_RECVTTL, IP_RETOPTS, and IP_RECVERR). ICMP source quenches and redirects are reported as fake errors via the error queue (IP_RECVERR); the next hop address for redirects is saved to ee_info (in network order).

socket(2) is restricted to the group range specified in "/proc/sys/net/ipv4/ping_group_range". It is "1 0" by default, meaning that nobody (not even root) may create ping sockets. Setting it to "100 100" would grant permissions to the single group (to either make /sbin/ping g+s and owned by this group or to grant permissions to the "netadmins" group), "0 4294967295" would enable it for the world, "100 4294967295" would enable it for the users, but not daemons.

The existing code might be (in the unlikely case anyone needs it) extended rather easily to handle other similar pairs of ICMP messages (Timestamp/Reply, Information Request/Reply, Address Mask Request/Reply etc.).

Userspace ping util & patch for it:
http://openwall.info/wiki/people/segoon/ping

For Openwall GNU/*/Linux it was the last step on the road to the setuid-less distro. A revision of this patch (for RHEL5/OpenVZ kernels) is in use in Owl-current, such as in the 2011/03/12 LiveCD ISOs:
http://mirrors.kernel.org/openwall/Owl/current/iso/

Initially this functionality was written by Pavel Kankovsky for Linux 2.4.32, but unfortunately it was never made public.

All ping options (-b, -p, -Q, -R, -s, -t, -T, -M, -I), are tested with the patch.

PATCH v3:
- switched to flowi4.
- minor changes to be consistent with raw sockets code.

PATCH v2:
- changed ping_debug() to pr_debug().
- removed CONFIG_IP_PING.
- removed ping_seq_fops.owner field (unused for procfs).
- switched to proc_net_fops_create().
- switched to %pK in seq_printf().

PATCH v1:
- fixed checksumming bug.
- CAP_NET_RAW may not create icmp sockets anymore.

RFC v2:
- minor cleanups.
- introduced sysctl'able group range to restrict socket(2).

Signed-off-by Vasiliy Kulikov <[email protected]>
---
 include/net/netns/ipv4.h   |    2 +
 include/net/ping.h         |   57 +++
 net/ipv4/Makefile          |    2 +-
 net/ipv4/af_inet.c         |   22 +
 net/ipv4/icmp.c            |   12 +-
 net/ipv4/ping.c            |  937 ++++++++++++++++++++++++++++++++++++++++++++
 net/ipv4/sysctl_net_ipv4.c |   80 ++++
 7 files changed, 1110 insertions(+), 2 deletions(-)
 create mode 100644 include/net/ping.h
 create mode 100644 net/ipv4/ping.c
include/net/netns/ipv4.h +2 -0
index 542195d..d786b4f 100644
@@ -54,6 +54,8 @@ struct netns_ipv4 {
 	int sysctl_rt_cache_rebuild_count;
 	int current_rt_cache_rebuild_count;
 
+	unsigned int sysctl_ping_group_range[2];
+
 	atomic_t rt_genid;
 	atomic_t dev_addr_genid;
include/net/ping.h new file +57 -0

57 lines not on this page · raw message

net/ipv4/Makefile +1 -1
index 0dc772d..f2dc69c 100644
@@ -11,7 +11,7 @@ obj-y     := route.o inetpeer.o protocol.o \
 	     datagram.o raw.o udp.o udplite.o \
 	     arp.o icmp.o devinet.o af_inet.o  igmp.o \
 	     fib_frontend.o fib_semantics.o fib_trie.o \
-	     inet_fragment.o
+	     inet_fragment.o ping.o
 
 obj-$(CONFIG_SYSCTL) += sysctl_net_ipv4.o
 obj-$(CONFIG_PROC_FS) += proc.o
net/ipv4/af_inet.c +22 -0
index 851aa05..cc14631 100644
@@ -105,6 +105,7 @@
 #include <net/tcp.h>
 #include <net/udp.h>
 #include <net/udplite.h>
+#include <net/ping.h>
 #include <linux/skbuff.h>
 #include <net/sock.h>
 #include <net/raw.h>

@@ -1008,6 +1009,14 @@ static struct inet_protosw inetsw_array[] =
 		.flags =      INET_PROTOSW_PERMANENT,
        },
 
+       {
+		.type =       SOCK_DGRAM,
+		.protocol =   IPPROTO_ICMP,
+		.prot =       &ping_prot,
+		.ops =        &inet_dgram_ops,
+		.no_check =   UDP_CSUM_DEFAULT,
+		.flags =      INET_PROTOSW_REUSE,
+       },
 
        {
 	       .type =       SOCK_RAW,

@@ -1527,6 +1536,7 @@ static const struct net_protocol udp_protocol = {
 
 static const struct net_protocol icmp_protocol = {
 	.handler =	icmp_rcv,
+	.err_handler =	ping_err,
 	.no_policy =	1,
 	.netns_ok =	1,
 };

@@ -1642,6 +1652,10 @@ static int __init inet_init(void)
 	if (rc)
 		goto out_unregister_udp_proto;
 
+	rc = proto_register(&ping_prot, 1);
+	if (rc)
+		goto out_unregister_raw_proto;
+
 	/*
 	 *	Tell SOCKET that we are alive...
 	 */

@@ -1697,6 +1711,8 @@ static int __init inet_init(void)
 	/* Add UDP-Lite (RFC 3828) */
 	udplite4_register();
 
+	ping_init();
+
 	/*
 	 *	Set the ICMP layer up
 	 */

@@ -1727,6 +1743,8 @@ static int __init inet_init(void)
 	rc = 0;
 out:
 	return rc;
+out_unregister_raw_proto:
+	proto_unregister(&raw_prot);
 out_unregister_udp_proto:
 	proto_unregister(&udp_prot);
 out_unregister_tcp_proto:

@@ -1751,11 +1769,15 @@ static int __init ipv4_proc_init(void)
 		goto out_tcp;
 	if (udp4_proc_init())
 		goto out_udp;
+	if (ping_proc_init())
+		goto out_ping;
 	if (ip_misc_proc_init())
 		goto out_misc;
 out:
 	return rc;
 out_misc:
+	ping_proc_exit();
+out_ping:
 	udp4_proc_exit();
 out_udp:
 	tcp4_proc_exit();
net/ipv4/icmp.c +11 -1
index 853a670..7c47eca 100644
@@ -83,6 +83,7 @@
 #include <net/tcp.h>
 #include <net/udp.h>
 #include <net/raw.h>
+#include <net/ping.h>
 #include <linux/skbuff.h>
 #include <net/sock.h>
 #include <linux/errno.h>

@@ -781,6 +782,15 @@ static void icmp_redirect(struct sk_buff *skb)
 			       iph->saddr, skb->dev);
 		break;
 	}
+
+	/* Ping wants to see redirects.
+         * Let's pretend they are errors of sorts... */
+	if (iph->protocol == IPPROTO_ICMP &&
+	    iph->ihl >= 5 &&
+	    pskb_may_pull(skb, (iph->ihl<<2)+8)) {
+		ping_err(skb, icmp_hdr(skb)->un.gateway);
+	}
+
 out:
 	return;
 out_err:

@@ -1041,7 +1051,7 @@ error:
  */
 static const struct icmp_control icmp_pointers[NR_ICMP_TYPES + 1] = {
 	[ICMP_ECHOREPLY] = {
-		.handler = icmp_discard,
+		.handler = ping_rcv,
 	},
 	[1] = {
 		.handler = icmp_discard,
net/ipv4/ping.c new file +937 -0

937 lines not on this page · raw message

net/ipv4/sysctl_net_ipv4.c +80 -0

118 lines not on this page · raw message

Signature · 1 line
1.7.0.4
maintainer Applied
#9 lore raw

From: Vasiliy Kulikov <[email protected]>
Date: Sat, 14 May 2011 00:01:00 +0400

Quoted 1 line

This patch adds IPPROTO_ICMP socket kind.

Applied, thanks for following through on all the review feedback.

#10 lore raw
In reply to Vasiliy Kulikov
Quoted 8 lines · Vasiliy Kulikov

This patch adds IPPROTO_ICMP socket kind. It makes it possible to send ICMP_ECHO messages and receive the corresponding ICMP_ECHOREPLY messages without any special privileges. In other words, the patch makes it possible to implement setuid-less and CAP_NET_RAW-less /bin/ping. In order not to increase the kernel's attack surface, the new functionality is disabled by default, but is enabled at bootup by supporting Linux distributions, optionally with restriction to a group or a group range (see below).

You'll need to do a manpage patch too. Otherwise noone will know how to use it.

-Andi

Signature · 1 line
[email protected] -- Speaking for myself only
#11 lore raw

PATCH 1/1 net: ipv4: add ping_group_range documentation

Quoted 11 lines · Andi Kleen
Quoted 8 lines · Vasiliy Kulikov

This patch adds IPPROTO_ICMP socket kind. It makes it possible to send ICMP_ECHO messages and receive the corresponding ICMP_ECHOREPLY messages without any special privileges. In other words, the patch makes it possible to implement setuid-less and CAP_NET_RAW-less /bin/ping. In order not to increase the kernel's attack surface, the new functionality is disabled by default, but is enabled at bootup by supporting Linux distributions, optionally with restriction to a group or a group range (see below).

Quoted 2 lines

You'll need to do a manpage patch too. Otherwise noone will know how to use it.

Yes probably...

It would be nice to copy part of changelog in
Documentation/networking/ip-sysctl.txt

socket(2) is restricted to the group range specified in "/proc/sys/net/ipv4/ping_group_range". It is "1 0" by default, meaning that nobody (not even root) may create ping sockets. Setting it to "100 100" would grant permissions to the single group (to either make /sbin/ping g+s and owned by this group or to grant permissions to the "netadmins" group), "0 4294967295" would enable it for the world, "100 4294967295" would enable it for the users, but not daemons.

Too late here, time for sleep...

--
To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#12 lore raw

PATCH 1/1 net: ping: dont call udp_ioctl()

In reply to Vasiliy Kulikov
Quoted 6 lines · Vasiliy Kulikov
(quoted diff) +6 -0
+/*
+ *	IOCTL requests applicable to the UDP^H^H^HICMP protocol
+ */
+
+int ping_ioctl(struct sock *sk, int cmd, unsigned long arg)
+{

Review

Quoted 10 lines
(quoted diff) +10 -0
+	pr_debug("ping_ioctl(sk=%p,sk->num=%u,cmd=%d,arg=%lu)\n",
+		inet_sk(sk), inet_sk(sk)->inet_num, cmd, arg);
+	switch (cmd) {
+	case SIOCOUTQ:
+	case SIOCINQ:
+		return udp_ioctl(sk, cmd, arg);
+	default:
+		return -ENOIOCTLCMD;
+	}
+}

Do we really need to support SIOCOUTQ and SIOCINQ ioctls for ping sockets ?

I ask this because udp_ioctl() assumes it handles UDP frames, and can change UDP_MIB_INERRORS in case first_packet_length() finds a frame with bad checksum.

[ UDP let the checksum be completed and checked when it performs the Kernel->User copy ]

I would just remove this legacy, please shout if you believe we really should support ioctl...

[ I actually tested that ping was still working correctly, of course ]

BTW, link
(ftp://mirrors.kernel.org/openwall/Owl/current/sources/Owl/packages/iputils/iputils-ss020927.tar.gz ) provided in http://openwall.info/wiki/people/segoon/ping is not working.

I had to manually patch iputils-s20101006.tar.bz2 instead.

Thanks

[PATCH net-next-2.6] net: ping: dont call udp_ioctl()

udp_ioctl() really handles UDP and UDPLite protocols.

1) It can increment UDP_MIB_INERRORS in case first_packet_length() finds a frame with bad checksum.

2) It has a dependency on sizeof(struct udphdr), not applicable to ICMP/PING

If ping sockets need to handle SIOCINQ/SIOCOUTQ ioctl, this should be done differently.

Signed-off-by Eric Dumazet <[email protected]>
CC Vasiliy Kulikov <[email protected]>
---
 net/ipv4/ping.c |   11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)
net/ipv4/ping.c +3 -8
index 7041d09..952505a 100644
@@ -610,20 +610,15 @@ do_confirm:
 }
 
 /*
- *	IOCTL requests applicable to the UDP^H^H^HICMP protocol
+ *	IOCTL requests applicable to PING sockets
  */
 
 int ping_ioctl(struct sock *sk, int cmd, unsigned long arg)
 {
 	pr_debug("ping_ioctl(sk=%p,sk->num=%u,cmd=%d,arg=%lu)\n",
 		inet_sk(sk), inet_sk(sk)->inet_num, cmd, arg);
-	switch (cmd) {
-	case SIOCOUTQ:
-	case SIOCINQ:
-		return udp_ioctl(sk, cmd, arg);
-	default:
-		return -ENOIOCTLCMD;
-	}
+
+	return -ENOIOCTLCMD;
 }
 
 int ping_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
Reviewed-by on v1 1/1
#13 lore raw

Re: [PATCH net-next-2.6] net: ping: dont call udp_ioctl()

Quoted 2 lines · Eric Dumazet

Do we really need to support SIOCOUTQ and SIOCINQ ioctls for ping sockets ?

Probably not.

Quoted 4 lines

BTW, link
(ftp://mirrors.kernel.org/openwall/Owl/current/sources/Owl/packages/iputils/iputils-ss020927.tar.gz ) provided in http://openwall.info/wiki/people/segoon/ping is not working.

I had to manually patch iputils-s20101006.tar.bz2 instead.

Oh, the link broke precisely because we updated to s20101006 since then, and the link was to our current branch. I've just updated the wiki page to include links both for iputils-ss020927 and for iputils-s20101006 (both original tarballs and patches).

Quoted 7 lines

[PATCH net-next-2.6] net: ping: dont call udp_ioctl()

udp_ioctl() really handles UDP and UDPLite protocols.

1) It can increment UDP_MIB_INERRORS in case first_packet_length() finds a frame with bad checksum.

Quoted 8 lines

2) It has a dependency on sizeof(struct udphdr), not applicable to ICMP/PING

If ping sockets need to handle SIOCINQ/SIOCOUTQ ioctl, this should be done differently.

Signed-off-by Eric Dumazet <[email protected]>
CC Vasiliy Kulikov <[email protected]>
Reviewed-by Solar Designer <[email protected]>

Thanks,

Alexander

maintainer
#14 lore raw

Re: [PATCH net-next-2.6] net: ping: dont call udp_ioctl()

From: Solar Designer <[email protected]>
Date: Mon, 16 May 2011 01:30:18 +0400

Quoted 26 lines · Eric Dumazet
Quoted 2 lines

Do we really need to support SIOCOUTQ and SIOCINQ ioctls for ping sockets ?

Probably not.

Quoted 4 lines

BTW, link
(ftp://mirrors.kernel.org/openwall/Owl/current/sources/Owl/packages/iputils/iputils-ss020927.tar.gz ) provided in http://openwall.info/wiki/people/segoon/ping is not working.

I had to manually patch iputils-s20101006.tar.bz2 instead.

Oh, the link broke precisely because we updated to s20101006 since then, and the link was to our current branch. I've just updated the wiki page to include links both for iputils-ss020927 and for iputils-s20101006 (both original tarballs and patches).

Quoted 10 lines

[PATCH net-next-2.6] net: ping: dont call udp_ioctl()

udp_ioctl() really handles UDP and UDPLite protocols.

1) It can increment UDP_MIB_INERRORS in case first_packet_length() finds a frame with bad checksum.

2) It has a dependency on sizeof(struct udphdr), not applicable to ICMP/PING

Quoted 7 lines
Quoted 5 lines

If ping sockets need to handle SIOCINQ/SIOCOUTQ ioctl, this should be done differently.

Signed-off-by Eric Dumazet <[email protected]>
CC Vasiliy Kulikov <[email protected]>
Reviewed-by Solar Designer <[email protected]>

Just get rid of ping_ioctl() entirely, as that is the effect of this change since inet_ioctl() returns -ENOIOCTLCMD when sk_prot->ioctl is NULL.

Also get rid of asm/ioctls.h since that will be no longer needed.

Thanks.

#15 lore raw

PATCH v2 1/1 net: ping: dont call udp_ioctl()

Quoted 5 lines · David Miller

Just get rid of ping_ioctl() entirely, as that is the effect of this change since inet_ioctl() returns -ENOIOCTLCMD when sk_prot->ioctl is NULL.

Also get rid of asm/ioctls.h since that will be no longer needed.

Sure, here is updated version, thanks.

[PATCH net-next-2.6 v2] net: ping: dont call udp_ioctl()

udp_ioctl() really handles UDP and UDPLite protocols.

1) It can increment UDP_MIB_INERRORS in case first_packet_length() finds a frame with bad checksum.

2) It has a dependency on sizeof(struct udphdr), not applicable to ICMP/PING

If ping sockets need to handle SIOCINQ/SIOCOUTQ ioctl, this should be done differently.

Signed-off-by Eric Dumazet <[email protected]>
CC Vasiliy Kulikov <[email protected]>
---
 net/ipv4/ping.c |   19 -------------------
 1 file changed, 19 deletions(-)
net/ipv4/ping.c +0 -19
index 7041d09..41836ab 100644
@@ -22,7 +22,6 @@
 
 #include <asm/system.h>
 #include <linux/uaccess.h>
-#include <asm/ioctls.h>
 #include <linux/types.h>
 #include <linux/fcntl.h>
 #include <linux/socket.h>

@@ -609,23 +608,6 @@ do_confirm:
 	goto out;
 }
 
-/*
- *	IOCTL requests applicable to the UDP^H^H^HICMP protocol
- */
-
-int ping_ioctl(struct sock *sk, int cmd, unsigned long arg)
-{
-	pr_debug("ping_ioctl(sk=%p,sk->num=%u,cmd=%d,arg=%lu)\n",
-		inet_sk(sk), inet_sk(sk)->inet_num, cmd, arg);
-	switch (cmd) {
-	case SIOCOUTQ:
-	case SIOCINQ:
-		return udp_ioctl(sk, cmd, arg);
-	default:
-		return -ENOIOCTLCMD;
-	}
-}
-
 int ping_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
 		 size_t len, int noblock, int flags, int *addr_len)
 {

@@ -735,7 +717,6 @@ struct proto ping_prot = {
 	.close =	ping_close,
 	.connect =	ip4_datagram_connect,
 	.disconnect =	udp_disconnect,
-	.ioctl =	ping_ioctl,
 	.setsockopt =	ip_setsockopt,
 	.getsockopt =	ip_getsockopt,
 	.sendmsg =	ping_sendmsg,
author Acked-by on v2 1/1
#16 lore raw

Re: [PATCH net-next-2.6 v2] net: ping: dont call udp_ioctl()

Quoted 17 lines · Eric Dumazet
Quoted 5 lines · David Miller

Just get rid of ping_ioctl() entirely, as that is the effect of this change since inet_ioctl() returns -ENOIOCTLCMD when sk_prot->ioctl is NULL.

Also get rid of asm/ioctls.h since that will be no longer needed.

Sure, here is updated version, thanks.

[PATCH net-next-2.6 v2] net: ping: dont call udp_ioctl()

udp_ioctl() really handles UDP and UDPLite protocols.

1) It can increment UDP_MIB_INERRORS in case first_packet_length() finds a frame with bad checksum.

Quoted 7 lines

2) It has a dependency on sizeof(struct udphdr), not applicable to ICMP/PING

If ping sockets need to handle SIOCINQ/SIOCOUTQ ioctl, this should be done differently.

Signed-off-by Eric Dumazet <[email protected]>
Acked-by Vasiliy Kulikov <[email protected]>

Thanks,

Signature · 2 lines
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
maintainer Applied
#17 lore raw

Re: [PATCH net-next-2.6 v2] net: ping: dont call udp_ioctl()

In reply to Eric Dumazet

From: Eric Dumazet <[email protected]>
Date: Mon, 16 May 2011 09:26:31 +0200

Quoted 17 lines · David Miller
Quoted 6 lines

Just get rid of ping_ioctl() entirely, as that is the effect of this change since inet_ioctl() returns -ENOIOCTLCMD when sk_prot->ioctl is NULL.

Also get rid of asm/ioctls.h since that will be no longer needed.

Sure, here is updated version, thanks.

[PATCH net-next-2.6 v2] net: ping: dont call udp_ioctl()

udp_ioctl() really handles UDP and UDPLite protocols.

1) It can increment UDP_MIB_INERRORS in case first_packet_length() finds a frame with bad checksum.

Quoted 8 lines

2) It has a dependency on sizeof(struct udphdr), not applicable to ICMP/PING

If ping sockets need to handle SIOCINQ/SIOCOUTQ ioctl, this should be done differently.

Signed-off-by Eric Dumazet <[email protected]>
CC Vasiliy Kulikov <[email protected]>

Applied, thanks.

#18 lore raw
In reply to Vasiliy Kulikov
Quoted 8 lines · Vasiliy Kulikov

This patch adds IPPROTO_ICMP socket kind.

+       seq_printf(f, "%5d: %08X:%04X %08X:%04X"
+               " %02X %08X:%08X %02X:%08lX %08X %5d %8d %lu %d %p %d%n",
+               bucket, src, srcp, dest, destp, sp->sk_state,
+               sk_wmem_alloc_get(sp),
+               sk_rmem_alloc_get(sp),
+               0, 0L, 0, sock_i_uid(sp), 0, sock_i_ino(sp),

These zeroes can be embedded into format string for slightly faster printing.

Quoted 2 lines

+static const struct file_operations ping_seq_fops = {
+       .owner          = THIS_MODULE,

Unnecessary line.
->owner is unused for proc files, this is not documented anywhere, but it's unused.

net/ipv4/ping.c line 879

Quoted 7 lines

+       .open           = ping_seq_open,
+       .read           = seq_read,
+       .llseek         = seq_lseek,
+       .release        = seq_release_net,

net/ipv4/ping.c new file +3 -0
+};
+
+static const char ping_proc_name[] = "icmp";

Ewww :-)
Does not compiler create only one string?

net/ipv4/ping.c line 887

Quoted 7 lines
net/ipv4/ping.c new file +7 -0
+static int ping_proc_register(struct net *net)
+{
+       struct proc_dir_entry *p;
+       int rc = 0;
+
+       p = proc_create_data(ping_proc_name, S_IRUGO, net->proc_net,
+                            &ping_seq_fops, NULL);

There is proc_net_fops_create().

Quoted 25 lines

+       if (!p)
+               rc = -ENOMEM;
+       return rc;

net/ipv4/sysctl_net_ipv4.c +1 -0
+}

@@ -680,6 +747,15 @@ static struct ctl_table ipv4_net_table[] = {
                .mode           = 0644,

               .proc_handler   = proc_dointvec
       },

net/ipv4/sysctl_net_ipv4.c +9 -0
+#ifdef CONFIG_IP_PING
+       {
+               .procname       = "ping_group_range",
+               .data           = &init_net.ipv4.sysctl_ping_group_range,
+               .maxlen         = sizeof(init_net.ipv4.sysctl_ping_group_range),
+               .mode           = 0644,
+               .proc_handler   = ipv4_ping_group_range,
+       },
+#endif

       { }
 };

net/ipv4/sysctl_net_ipv4.c +0 -0
@@ -714,8 +790,22 @@ static __net_init int ipv4_sysctl_init_net(struct net *net)
                        &net->ipv4.sysctl_icmp_ratemask;

               table[6].data = &net->ipv4.sysctl_rt_cache_rebuild_count;

net/ipv4/sysctl_net_ipv4.c static __net_init int ipv4_sysctl_init_net(struct net *net) · line 796

Quoted 4 lines
net/ipv4/sysctl_net_ipv4.c +4 -0
+#ifdef CONFIG_IP_PING
+               table[7].data =
+                       &net->ipv4.sysctl_ping_group_range;
+#endif

Now I understand it's not related, but next sysctl will have "table[8].data = ..." line which is off-by-one if CONFIG_IP_PING=n.

author
#19 lore raw
Quoted 11 lines · Alexey Dobriyan
Quoted 8 lines · Vasiliy Kulikov

This patch adds IPPROTO_ICMP socket kind.

+       seq_printf(f, "%5d: %08X:%04X %08X:%04X"
+               " %02X %08X:%08X %02X:%08lX %08X %5d %8d %lu %d %p %d%n",
+               bucket, src, srcp, dest, destp, sp->sk_state,
+               sk_wmem_alloc_get(sp),
+               sk_rmem_alloc_get(sp),
+               0, 0L, 0, sock_i_uid(sp), 0, sock_i_ino(sp),

These zeroes can be embedded into format string for slightly faster printing.

Is it really needed? I mean, it is not a fast path, so such a small overhead is not very bad. But embedding them into the string makes it a bit more difficult to read.

Quoted 6 lines
Quoted 2 lines

+static const struct file_operations ping_seq_fops = {
+       .owner          = THIS_MODULE,

Unnecessary line.
->owner is unused for proc files, this is not documented anywhere, but it's unused.

OK.

Quoted 4 lines
Quoted 1 line

+static const char ping_proc_name[] = "icmp";

Ewww :-)
Does not compiler create only one string?

I used it for better readability as it is used 2 times.

Quoted 4 lines
Quoted 2 lines

+       p = proc_create_data(ping_proc_name, S_IRUGO, net->proc_net,
+                            &ping_seq_fops, NULL);

There is proc_net_fops_create().

OK.

Quoted 7 lines

net/ipv4/sysctl_net_ipv4.c static __net_init int ipv4_sysctl_init_net(struct net *net) · line 796

Quoted 4 lines
net/ipv4/sysctl_net_ipv4.c +4 -0
+#ifdef CONFIG_IP_PING
+               table[7].data =
+                       &net->ipv4.sysctl_ping_group_range;
+#endif

Now I understand it's not related, but next sysctl will have "table[8].data = ..." line which is off-by-one if CONFIG_IP_PING=n.

Agreed that hardcoded indexes look a bit ugly, especially with configurable elements. But as Dave suggested to completely remove CONFIG_IP_PING, it doesn't make sense now.

Thank you,

Signature · 2 lines
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
#20 lore raw
Quoted 11 lines · Vasiliy Kulikov
Quoted 10 lines · Alexey Dobriyan
Quoted 8 lines · Vasiliy Kulikov

This patch adds IPPROTO_ICMP socket kind.

+       seq_printf(f, "%5d: %08X:%04X %08X:%04X"
+               " %02X %08X:%08X %02X:%08lX %08X %5d %8d %lu %d %p %d%n",
+               bucket, src, srcp, dest, destp, sp->sk_state,
+               sk_wmem_alloc_get(sp),
+               sk_rmem_alloc_get(sp),
+               0, 0L, 0, sock_i_uid(sp), 0, sock_i_ino(sp),

Quoted 5 lines
Quoted 1 line

These zeroes can be embedded into format string for slightly faster printing.

Is it really needed?  I mean, it is not a fast path, so such a small overhead is not very bad.  But embedding them into the string makes it a bit more difficult to read.

In fact, if field is always zero, it can be removed altogether. Also, there was big discussion re exposing kernel socket pointers, which this file continue to do.

Quoted 2 lines
net/ipv4/ping.c new file +2 -0
+               atomic_read(&sp->sk_refcnt), sp,
+               atomic_read(&sp->sk_drops), len);
#21 lore raw
Quoted 2 lines · Alexey Dobriyan

Also, there was big discussion re exposing kernel socket pointers, which this file continue to do.

One late comment:

The code was intented to immitate /proc/net/udp.

As far as I can tell UDP and TCP (having checked udp.c and tcp_ipv[46].c in the latest net-next-2.6) do not have any qualms about exposing kernel pointers so we are a little bit holier than the pope here. :)

Signature · 2 lines
Pavel Kankovsky aka Peak                          / Jeremiah 9:21        \
"For death is come up into our MS Windows(tm)..." \ 21st century edition /
author
#22 lore raw
Quoted 5 lines · Pavel Kankovsky
Quoted 2 lines · Alexey Dobriyan

Also, there was big discussion re exposing kernel socket pointers, which this file continue to do.

Quoted 7 lines

One late comment:

The code was intented to immitate /proc/net/udp.

As far as I can tell UDP and TCP (having checked udp.c and tcp_ipv[46].c in the latest net-next-2.6) do not have any qualms about exposing kernel pointers so we are a little bit holier than the pope here. :)

Anyway, ping uses %pK:

seq_printf(f, "%5d: %08X:%04X %08X:%04X"

           " %02X %08X:%08X %02X:%08lX %08X %5d %8d %lu %d %pK d%n",
           bucket, src, srcp, dest, destp, sp->sk_state,
           sk_wmem_alloc_get(sp),
           sk_rmem_alloc_get(sp),
           0, 0L, 0, sock_i_uid(sp), 0, sock_i_ino(sp),
           atomic_read(&sp->sk_refcnt), sp,
           atomic_read(&sp->sk_drops), len);

Thanks,

Signature · 2 lines
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
#23 lore raw
In reply to Alexey Dobriyan
Quoted 1 line · Alexey Dobriyan

On Sat, Apr 9, 2011 at 1:15 PM, Vasiliy Kulikov <[email protected]> wrote:

[snip]

Quoted 11 lines

net/ipv4/sysctl_net_ipv4.c static __net_init int ipv4_sysctl_init_net(struct net *net) · line 796

Quoted 8 lines
net/ipv4/sysctl_net_ipv4.c +0 -0
@@ -714,8 +790,22 @@ static __net_init int ipv4_sysctl_init_net(struct net *net)
                        &net->ipv4.sysctl_icmp_ratemask;

               table[6].data = &net->ipv4.sysctl_rt_cache_rebuild_count;

net/ipv4/sysctl_net_ipv4.c +4 -0
+#ifdef CONFIG_IP_PING
+               table[7].data =
+                       &net->ipv4.sysctl_ping_group_range;
+#endif

Now I understand it's not related, but next sysctl will have "table[8].data = ..." line which is off-by-one if CONFIG_IP_PING=n.

Another good reason for the code to be non-optoinal and not to have CONFIG_IP_PING.