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

[Salvatore Mesoraca <s.mesoraca16@gmail.com>]

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