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

[YOSHIFUJI Hideaki <hideaki.yoshifuji@miraclelinux.com>]

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