Message ID | 2142872.6QHf5ZAfZz@msg-id |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
Hi, Salvatore Mesoraca wrote: > From: Vasiliy Kulikov <segooon@gmail.com> > > This patch adds non-raw IPPROTO_ICMP socket kind support that was added > to the Linux 3.0. The patch is backward-compatible: if ICMP socket kind > is not enabled in the kernel (either in case of an old kernel or > explicitly disabled via /proc/sys/net/ipv4/ping_group_range), ping uses > old privileged raw sockets as a fallback. : > This patch is going to be included in Ubuntu 15.10 and it is already > included in Gentoo stable tree (at the moment of the writing ping has > CAP_NET_RAW still enabled by default) it is also included in OpenWall > since 2011. > This patch also tries to sneak in a fix for a missing colon in a printf. > I've tested it on Linux 3.17.7 and it worked without issues. Please do not mix changes in a single commit. Thanks.
YOSHIFUJI Hideaki wrote: > Hi, Hi, thank you for thaking to the time to answer > Salvatore Mesoraca wrote: > > From: Vasiliy Kulikov <segooon@gmail.com> > > > > This patch adds non-raw IPPROTO_ICMP socket kind support that was added > > to the Linux 3.0. The patch is backward-compatible: if ICMP socket kind > > is not enabled in the kernel (either in case of an old kernel or > > explicitly disabled via /proc/sys/net/ipv4/ping_group_range), ping uses > > old privileged raw sockets as a fallback. > > > > This patch is going to be included in Ubuntu 15.10 and it is already > > included in Gentoo stable tree (at the moment of the writing ping has > > CAP_NET_RAW still enabled by default) it is also included in OpenWall > > since 2011. > > This patch also tries to sneak in a fix for a missing colon in a printf. > > I've tested it on Linux 3.17.7 and it worked without issues. > > Please do not mix changes in a single commit. > Thanks. I had some doubt about that additional change (it is so small that I don't think that anyone will ever make a separate patch for it), but it was present in the original patch and I decided to add it anyway and wait for feedback. I'll delete it. Thank you again for you comment. -- 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
Salvatore Mesoraca wrote: > YOSHIFUJI Hideaki wrote: >> Hi, > Hi, > thank you for thaking to the time to answer > >> Salvatore Mesoraca wrote: >>> From: Vasiliy Kulikov <segooon@gmail.com> >>> >>> This patch adds non-raw IPPROTO_ICMP socket kind support that was added >>> to the Linux 3.0. The patch is backward-compatible: if ICMP socket kind >>> is not enabled in the kernel (either in case of an old kernel or >>> explicitly disabled via /proc/sys/net/ipv4/ping_group_range), ping uses >>> old privileged raw sockets as a fallback. >>> >>> This patch is going to be included in Ubuntu 15.10 and it is already >>> included in Gentoo stable tree (at the moment of the writing ping has >>> CAP_NET_RAW still enabled by default) it is also included in OpenWall >>> since 2011. >>> This patch also tries to sneak in a fix for a missing colon in a printf. >>> I've tested it on Linux 3.17.7 and it worked without issues. >> >> Please do not mix changes in a single commit. >> Thanks. > > I had some doubt about that additional change (it is so small that I don't > think that anyone will ever make a separate patch for it), but it was present > in the original patch and I decided to add it anyway and wait for feedback. > I'll delete it. > Thank you again for you comment. What I meant was changes not for supporting non-raw icmp socket should be formed separately. It seems that the patch try to change other things as well, in receive_error_msg() for example. --yoshfuji
In data mercoledì 8 aprile 2015 18:20:43, YOSHIFUJI Hideaki ha scritto: > Salvatore Mesoraca wrote: > > YOSHIFUJI Hideaki wrote: > >> Hi, > > > > Hi, > > thank you for thaking to the time to answer > > > >> Salvatore Mesoraca wrote: > >>> From: Vasiliy Kulikov <segooon@gmail.com> > >>> > >>> This patch adds non-raw IPPROTO_ICMP socket kind support that was added > >>> to the Linux 3.0. The patch is backward-compatible: if ICMP socket kind > >>> is not enabled in the kernel (either in case of an old kernel or > >>> explicitly disabled via /proc/sys/net/ipv4/ping_group_range), ping uses > >>> old privileged raw sockets as a fallback. > >>> > >>> This patch is going to be included in Ubuntu 15.10 and it is already > >>> included in Gentoo stable tree (at the moment of the writing ping has > >>> CAP_NET_RAW still enabled by default) it is also included in OpenWall > >>> since 2011. > >>> This patch also tries to sneak in a fix for a missing colon in a printf. > >>> I've tested it on Linux 3.17.7 and it worked without issues. > >> > >> Please do not mix changes in a single commit. > >> Thanks. > > > > I had some doubt about that additional change (it is so small that I don't > > think that anyone will ever make a separate patch for it), but it was > > present in the original patch and I decided to add it anyway and wait for > > feedback. I'll delete it. > > Thank you again for you comment. > > What I meant was changes not for supporting non-raw icmp socket > should be formed separately. It seems that the patch try to > change other things as well, in receive_error_msg() for example. > > --yoshfuji Thank you for the clarification. If I understood correctly you were referring to these lines: > @@ -652,9 +687,18 @@ int receive_error_msg() > goto out; > } > > - acknowledge(ntohs(icmph.un.echo.sequence)); > + error_pkt = (e->ee_type != ICMP_REDIRECT && > + e->ee_type != ICMP_SOURCE_QUENCH); > + if (error_pkt) { > + acknowledge(ntohs(icmph.un.echo.sequence)); > + net_errors++; > + nerrors++; > + } > + else { > + saved_errno = 0; > + } In my understanding these changes are required to support non-raw ICMP sockets because they cannot use setsockopt(icmp_sock, SOL_RAW, ICMP_FILTER...) and they need to get rid of ICMP_REDIRECT and ICMP_SOURCE_QUENCH in a different way. IMHO this change alone does not make much sense. Anyway if you think that this and other changes should be in different commits I'll post a split PATCHv2. Thank you again for your time. Salvatore Mesoraca -- 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
Salvatore Mesoraca wrote: > In data mercoledì 8 aprile 2015 18:20:43, YOSHIFUJI Hideaki ha scritto: >> Salvatore Mesoraca wrote: >>> YOSHIFUJI Hideaki wrote: >>>> Hi, >>> >>> Hi, >>> thank you for thaking to the time to answer >>> >>>> Salvatore Mesoraca wrote: >>>>> From: Vasiliy Kulikov <segooon@gmail.com> >>>>> >>>>> This patch adds non-raw IPPROTO_ICMP socket kind support that was added >>>>> to the Linux 3.0. The patch is backward-compatible: if ICMP socket kind >>>>> is not enabled in the kernel (either in case of an old kernel or >>>>> explicitly disabled via /proc/sys/net/ipv4/ping_group_range), ping uses >>>>> old privileged raw sockets as a fallback. >>>>> >>>>> This patch is going to be included in Ubuntu 15.10 and it is already >>>>> included in Gentoo stable tree (at the moment of the writing ping has >>>>> CAP_NET_RAW still enabled by default) it is also included in OpenWall >>>>> since 2011. >>>>> This patch also tries to sneak in a fix for a missing colon in a printf. >>>>> I've tested it on Linux 3.17.7 and it worked without issues. >>>> >>>> Please do not mix changes in a single commit. >>>> Thanks. >>> >>> I had some doubt about that additional change (it is so small that I don't >>> think that anyone will ever make a separate patch for it), but it was >>> present in the original patch and I decided to add it anyway and wait for >>> feedback. I'll delete it. >>> Thank you again for you comment. >> >> What I meant was changes not for supporting non-raw icmp socket >> should be formed separately. It seems that the patch try to >> change other things as well, in receive_error_msg() for example. >> >> --yoshfuji > > Thank you for the clarification. > If I understood correctly you were referring to these lines: >> @@ -652,9 +687,18 @@ int receive_error_msg() >> goto out; >> } >> >> - acknowledge(ntohs(icmph.un.echo.sequence)); >> + error_pkt = (e->ee_type != ICMP_REDIRECT && >> + e->ee_type != ICMP_SOURCE_QUENCH); >> + if (error_pkt) { >> + acknowledge(ntohs(icmph.un.echo.sequence)); >> + net_errors++; >> + nerrors++; >> + } >> + else { >> + saved_errno = 0; >> + } > > > In my understanding these changes are required to support non-raw ICMP sockets > because they cannot use setsockopt(icmp_sock, SOL_RAW, ICMP_FILTER...) and > they need to get rid of ICMP_REDIRECT and ICMP_SOURCE_QUENCH in a different > way. IMHO this change alone does not make much sense. OK, I understand. > Anyway if you think that this and other changes should be in different commits > I'll post a split PATCHv2. No, please just exclude this from first patch: - printf("From %s icmp_seq=%u ", pr_addr(sin->sin_addr.s_addr), ntohs(icmph.un.echo.sequence)); + printf("From %s: icmp_seq=%u ", pr_addr(sin->sin_addr.s_addr), ntohs(icmph.un.echo.sequence)); And, would you provide patch for ping6 as well? Thank you.
YOSHIFUJI Hideaki wrote: > Salvatore Mesoraca wrote: > > In data mercoledì 8 aprile 2015 18:20:43, YOSHIFUJI Hideaki ha scritto: > >> Salvatore Mesoraca wrote: > >>> YOSHIFUJI Hideaki wrote: > >>>> Hi, > >>> > >>> Hi, > >>> thank you for thaking to the time to answer > >>> > >>>> Salvatore Mesoraca wrote: > >>>>> From: Vasiliy Kulikov <segooon@gmail.com> > >>>>> > >>>>> This patch adds non-raw IPPROTO_ICMP socket kind support that was > >>>>> added > >>>>> to the Linux 3.0. The patch is backward-compatible: if ICMP socket > >>>>> kind > >>>>> is not enabled in the kernel (either in case of an old kernel or > >>>>> explicitly disabled via /proc/sys/net/ipv4/ping_group_range), ping > >>>>> uses > >>>>> old privileged raw sockets as a fallback. > >>>>> > >>>>> This patch is going to be included in Ubuntu 15.10 and it is already > >>>>> included in Gentoo stable tree (at the moment of the writing ping has > >>>>> CAP_NET_RAW still enabled by default) it is also included in OpenWall > >>>>> since 2011. > >>>>> This patch also tries to sneak in a fix for a missing colon in a > >>>>> printf. > >>>>> I've tested it on Linux 3.17.7 and it worked without issues. > >>>> > >>>> Please do not mix changes in a single commit. > >>>> Thanks. > >>> > >>> I had some doubt about that additional change (it is so small that I > >>> don't > >>> think that anyone will ever make a separate patch for it), but it was > >>> present in the original patch and I decided to add it anyway and wait > >>> for > >>> feedback. I'll delete it. > >>> Thank you again for you comment. > >> > >> What I meant was changes not for supporting non-raw icmp socket > >> should be formed separately. It seems that the patch try to > >> change other things as well, in receive_error_msg() for example. > >> > >> --yoshfuji > > > > Thank you for the clarification. > > > > If I understood correctly you were referring to these lines: > >> @@ -652,9 +687,18 @@ int receive_error_msg() > >> > >> goto out; > >> > >> } > >> > >> - acknowledge(ntohs(icmph.un.echo.sequence)); > >> + error_pkt = (e->ee_type != ICMP_REDIRECT && > >> + e->ee_type != ICMP_SOURCE_QUENCH); > >> + if (error_pkt) { > >> + acknowledge(ntohs(icmph.un.echo.sequence)); > >> + net_errors++; > >> + nerrors++; > >> + } > >> + else { > >> + saved_errno = 0; > >> + } > > > > In my understanding these changes are required to support non-raw ICMP > > sockets because they cannot use setsockopt(icmp_sock, SOL_RAW, > > ICMP_FILTER...) and they need to get rid of ICMP_REDIRECT and > > ICMP_SOURCE_QUENCH in a different way. IMHO this change alone does not > > make much sense. > > OK, I understand. > > > Anyway if you think that this and other changes should be in different > > commits I'll post a split PATCHv2. > > No, please just exclude this from first patch: > - printf("From %s icmp_seq=%u ", pr_addr(sin->sin_addr.s_addr), > ntohs(icmph.un.echo.sequence)); + printf("From %s: icmp_seq=%u ", > pr_addr(sin->sin_addr.s_addr), ntohs(icmph.un.echo.sequence)); Great, I'll submit PATCHv2 as soon as possible. > And, would you provide patch for ping6 as well? I was planning to work on ping6 as well If this patch get accepted. > Thank you. Thank you too for your time. Salvatore Mesoraca -- 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
On Wed, Apr 8, 2015 at 10:05 PM, Salvatore Mesoraca <s.mesoraca16@gmail.com> wrote: >> And, would you provide patch for ping6 as well? > > I was planning to work on ping6 as well If this patch get accepted. Yoshifuji-san, I sent you a patch that works for both IPv4 and IPv6 a couple of years ago: http://marc.info/?l=linux-netdev&m=137026384215378 http://marc.info/?l=linux-netdev&m=137026384815379 This code has been running on Android since then: https://android-review.googlesource.com/#/c/61948/ Perhaps we can start from that patchset? Cheers, Lorenzo -- 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
Lorenzo Colitti wrote: > On Wed, Apr 8, 2015 at 10:05 PM, Salvatore Mesoraca <s.mesoraca16@gmail.com> > wrote: > > > And, would you provide patch for ping6 as well? > > > > I was planning to work on ping6 as well If this patch get accepted. > > Yoshifuji-san, > > I sent you a patch that works for both IPv4 and IPv6 a couple of years ago: > > http://marc.info/?l=linux-netdev&m=137026384215378 > http://marc.info/?l=linux-netdev&m=137026384815379 > > This code has been running on Android since then: > > https://android-review.googlesource.com/#/c/61948/ > > Perhaps we can start from that patchset? > > Cheers, > Lorenzo Hi Lorenzo, Thank you for bringing this up, it seems that you have a better understanding than mine on the original patch. Anyway I'm wondering why you didn't include this change: > - if (!working_recverr) { > + if (!using_ping_socket && !working_recverr) { inside receive_error_msg() The code in the if block try to execute a setsockopt(icmp_sock, SOL_RAW, ICMP_FILTER, ...) which is only possible on a raw socket. What am I missing? Thank you for your patience, I hope this time your patch get accepted, Salvatore Mesoraca -- 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
diff --git a/ping.c b/ping.c index c0366cd..1cb9bfa 100644 --- a/ping.c +++ b/ping.c @@ -55,7 +55,8 @@ char copyright[] = * Public Domain. Distribution Unlimited. * Bugs - * More statistics could always be gathered. - * This program has to run SUID to ROOT to access the ICMP socket. + * This program has to run SUID to ROOT to access the ICMP socket only + * if the kernel does not support non-raw ICMP socket. */ #include "ping_common.h" @@ -91,6 +92,7 @@ struct sockaddr_in whereto; /* who to ping */ int optlen = 0; int settos = 0; /* Set TOS, Precendence or other QOS options */ int icmp_sock; /* socket file descriptor */ +int using_ping_socket = 0; u_char outpack[0x10000]; int maxpacket = sizeof(outpack); @@ -139,7 +141,11 @@ main(int argc, char **argv) enable_capability_raw(); - icmp_sock = socket(AF_INET, SOCK_RAW, IPPROTO_ICMP); + icmp_sock = socket(AF_INET, SOCK_DGRAM, IPPROTO_ICMP); + if (icmp_sock != -1) + using_ping_socket = 1; + else + icmp_sock = socket(AF_INET, SOCK_RAW, IPPROTO_ICMP); socket_errno = errno; disable_capability_raw(); @@ -453,13 +459,35 @@ main(int argc, char **argv) } } - if ((options&F_STRICTSOURCE) && - bind(icmp_sock, (struct sockaddr*)&source, sizeof(source)) == -1) { - perror("bind"); - exit(2); + if (!using_ping_socket) { + if ((options&F_STRICTSOURCE) && + bind(icmp_sock, (struct sockaddr*)&source, sizeof(source)) == -1) { + perror("bind"); + exit(2); + } + } else { + struct sockaddr_in sa; + socklen_t sl; + + sa.sin_family = AF_INET; + sa.sin_port = 0; + sa.sin_addr.s_addr = (options&F_STRICTSOURCE) ? + source.sin_addr.s_addr : 0; + sl = sizeof(sa); + + if (bind(icmp_sock, (struct sockaddr *) &sa, sl) == -1) { + perror("bind"); + exit(2); + } + + if (getsockname(icmp_sock, (struct sockaddr *) &sa, &sl) == -1) { + perror("getsockname"); + exit(2); + } + ident = sa.sin_port; } - if (1) { + if (!using_ping_socket) { struct icmp_filter filt; filt.data = ~((1<<ICMP_SOURCE_QUENCH)| (1<<ICMP_DEST_UNREACH)| @@ -474,6 +502,12 @@ main(int argc, char **argv) hold = 1; if (setsockopt(icmp_sock, SOL_IP, IP_RECVERR, (char *)&hold, sizeof(hold))) fprintf(stderr, "WARNING: your kernel is veeery old. No problems.\n"); + if (using_ping_socket) { + if (setsockopt(icmp_sock, SOL_IP, IP_RECVTTL, (char *)&hold, sizeof(hold))) + perror("WARNING: setsockopt(IP_RECVTTL)"); + if (setsockopt(icmp_sock, SOL_IP, IP_RETOPTS, (char *)&hold, sizeof(hold))) + perror("WARNING: setsockopt(IP_RETOPTS)"); + } /* record route option */ if (options & F_RROUTE) { @@ -642,6 +676,7 @@ int receive_error_msg() nerrors++; } else if (e->ee_origin == SO_EE_ORIGIN_ICMP) { struct sockaddr_in *sin = (struct sockaddr_in*)(e+1); + int error_pkt; if (res < sizeof(icmph) || target.sin_addr.s_addr != whereto.sin_addr.s_addr || @@ -652,9 +687,18 @@ int receive_error_msg() goto out; } - acknowledge(ntohs(icmph.un.echo.sequence)); + error_pkt = (e->ee_type != ICMP_REDIRECT && + e->ee_type != ICMP_SOURCE_QUENCH); + if (error_pkt) { + acknowledge(ntohs(icmph.un.echo.sequence)); + net_errors++; + nerrors++; + } + else { + saved_errno = 0; + } - if (!working_recverr) { + if (!using_ping_socket && !working_recverr) { struct icmp_filter filt; working_recverr = 1; /* OK, it works. Add stronger filter. */ @@ -665,15 +709,14 @@ int receive_error_msg() perror("\rWARNING: setsockopt(ICMP_FILTER)"); } - net_errors++; - nerrors++; if (options & F_QUIET) goto out; if (options & F_FLOOD) { - write_stdout("\bE", 2); + if (error_pkt) + write_stdout("\bE", 2); } else { print_timestamp(); - printf("From %s icmp_seq=%u ", pr_addr(sin->sin_addr.s_addr), ntohs(icmph.un.echo.sequence)); + printf("From %s: icmp_seq=%u ", pr_addr(sin->sin_addr.s_addr), ntohs(icmph.un.echo.sequence)); pr_icmph(e->ee_type, e->ee_code, e->ee_info, NULL); fflush(stdout); } @@ -765,15 +808,41 @@ parse_reply(struct msghdr *msg, int cc, void *addr, struct timeval *tv) struct iphdr *ip; int hlen; int csfailed; + struct cmsghdr *cmsg; + int ttl; + __u8 *opts; + int optlen; /* Check the IP header */ ip = (struct iphdr *)buf; - hlen = ip->ihl*4; - if (cc < hlen + 8 || ip->ihl < 5) { - if (options & F_VERBOSE) - fprintf(stderr, "ping: packet too short (%d bytes) from %s\n", cc, - pr_addr(from->sin_addr.s_addr)); - return 1; + if (!using_ping_socket) { + hlen = ip->ihl*4; + if (cc < hlen + 8 || ip->ihl < 5) { + if (options & F_VERBOSE) + fprintf(stderr, "ping: packet too short (%d bytes) from %s\n", cc, + pr_addr(from->sin_addr.s_addr)); + return 1; + } + ttl = ip->ttl; + opts = buf + sizeof(struct iphdr); + optlen = hlen - sizeof(struct iphdr); + } else { + hlen = 0; + ttl = 0; + opts = buf; + optlen = 0; + for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) { + if (cmsg->cmsg_level != SOL_IP) + continue; + if (cmsg->cmsg_type == IP_TTL) { + if (cmsg->cmsg_len < sizeof(int)) + continue; + ttl = *(int *) CMSG_DATA(cmsg); + } else if (cmsg->cmsg_type == IP_RETOPTS) { + opts = (__u8 *) CMSG_DATA(cmsg); + optlen = cmsg->cmsg_len; + } + } } /* Now the ICMP part */ @@ -786,7 +855,7 @@ parse_reply(struct msghdr *msg, int cc, void *addr, struct timeval *tv) return 1; /* 'Twas not our ECHO */ if (gather_statistics((__u8*)icp, sizeof(*icp), cc, ntohs(icp->un.echo.sequence), - ip->ttl, 0, tv, pr_addr(from->sin_addr.s_addr), + ttl, 0, tv, pr_addr(from->sin_addr.s_addr), pr_echo_reply)) return 0; } else { @@ -877,7 +946,7 @@ parse_reply(struct msghdr *msg, int cc, void *addr, struct timeval *tv) } if (!(options & F_FLOOD)) { - pr_options(buf + sizeof(struct iphdr), hlen); + pr_options(opts, optlen + sizeof(struct iphdr)); if (options & F_AUDIBLE) putchar('\a'); @@ -1022,8 +1091,7 @@ void pr_icmph(__u8 type, __u8 code, __u32 info, struct icmphdr *icp) printf("Redirect, Bad Code: %d", code); break; } - if (icp) - printf("(New nexthop: %s)\n", pr_addr(icp->un.gateway)); + printf("(New nexthop: %s)\n", pr_addr(icp ? icp->un.gateway : info)); if (icp && (options & F_VERBOSE)) pr_iph((struct iphdr*)(icp + 1)); break; @@ -1339,7 +1407,7 @@ void install_filter(void) insns }; - if (once) + if (once || using_ping_socket) return; once = 1; diff --git a/ping_common.c b/ping_common.c index 8d6b145..0342e1a 100644 --- a/ping_common.c +++ b/ping_common.c @@ -677,7 +677,8 @@ void setup(int icmp_sock) *p++ = i; } - ident = htons(getpid() & 0xFFFF); + if (!ident) + ident = htons(getpid() & 0xFFFF); set_signal(SIGINT, sigexit); set_signal(SIGALRM, sigexit);