Re: [PATCH] iputils ping: add (non-raw) ICMP socket support

Salvatore Mesoraca
2015-04-08T13:05:16Z

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