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

YOSHIFUJI Hideaki
2015-04-08T12:05:40Z

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.

--
Hideaki Yoshifuji <hideaki.yoshifuji@miraclelinux.com>
Technical Division, MIRACLE LINUX CORPORATION