Re: [PATCH 0/1] tidbits: net-ping: add ping tidbit using raw socket

Am 06.07.26 um 00:17 schrieb Hannes Diethelm:
> Am 05.07.26 um 18:49 schrieb Philippe Gerum:
>> Hannes Diethelm <[email protected]> writes:
>>
>>> Am 05.07.26 um 17:01 schrieb Philippe Gerum:
>>>> Hannes Diethelm <[email protected]> writes:
>>>>
>>>>> This patch is the result from experimenting with evl networking. I 
>>>>> use it to do basic
>>>>> connection tests from the out of band network and check the timing.
>>>>>
>>>>> It is a bit complex due to evl doesn't support IPPROTO_ICMP sockets.
>>>>>
>>>>> Using the in-band network stack, you can use: sockfd = 
>>>>> socket(AF_INET, SOCK_DGRAM, IPPROTO_ICMP);
>>>>> and then only send the icmp part of the message. However, such a 
>>>>> support is not really needed,
>>>>> it works fine without.
>>>>>
>>>> This said, we have an ICMP protocol module in the evl stack since
>>>> recently, which only responds to ICMP_ECHO requests ATM, directly from
>>>> kernel space. Extending IPPROTO_ICMP by implementing the oob_sendmsg()
>>>> handler for it there has become an option.
>>>>
>>>
>>> That would make the ping tool way simpler. I can also wait until this 
>>> is available and
>>> create a new patch. It's mostly done just to figure out that 
>>> IPPROTO_ICMP is not available.
>>
>> Ok, I believe having IPPROTO_ICMP would be the best approach. I'll
>> let you know when this is available.
>>
> 
> Meanwhile, I changed to ms and corrected the "%zu". Attached 4 variants:
> oob-net-ping.c - The variant from the patch, improved
> oob-net-ping-nonraw.c - OOB with IPPROTO_ICMP (Not tested due to obvious 
> reasons)
> ping-raw.c - POSIX raw
> ping-nonraw.c - POSIX with IPPROTO_ICMP
> 
> This might help you debug the IPPROTO_ICMP implementation.
> 
> While sometimes having issues with EVL, I created also POSIX variants
> so see if it is my issue.
> 
> I can create also a patch if you prefer for one of the two OOB variants.
> 
> There where issues with -O3 in ip_checksum(). With -O0, it works.
> 
> The only difference I see between working and faulty is that with 16bit- 
> aligned (raw)
> it fails and with 32bit-aligned (nonraw) it works. -O3 debugging doesn't 
> really work, in assembly
> its nearly unreadable with -O3 and as soon as I add a printf() in the 
> while loop,
> the checksum is good. I have no clue might be UB or a compiler bug. Took 
> me some time to debug.
> I have gcc (Debian 14.2.0-19) 14.2.0
> 
> Of course, as soon as I create a test program with only ip_checksum() 
> and exactly the
> same data, it works also, just combined with the code, it breaks down.
> 
> This version creates faulty check-sums:
> static uint16_t ip_checksum(void *buf, int len)
> {
>      uint16_t *p = buf;    /* buf is assumed to be 16bit-aligned. */
>      uint32_t sum = 0;
>      int count = len;
> 
>      while (count > 1) {
>          sum += *p++;
>          count -= sizeof(*p);
>      }
> 
>      if (count > 0)
>          sum += *(uint8_t *)p;
> 
>      while (sum >> 16)
>          sum = (sum & 0xffff) + (sum >> 16);
> 
>      return ~sum & 0xffff;
> }
> 
> While this works:
> static uint16_t ip_checksum(void *buf, int len)
> {
>      void *p = buf;
>      uint32_t sum = 0;
>      int count = len;
> 
>      while (count > 1) {
>          sum += (uint16_t)(*(uint8_t *)p) + ((uint16_t)(*(uint8_t *)(p + 
> 1)) << 8);
>          p += 2;
>          count -= 2;
>      }
> 
>      if (count > 0)
>          sum += *(uint8_t *)p;
> 
>      while (sum >> 16)
>          sum = (sum & 0xffff) + (sum >> 16);
> 
>      return ~sum & 0xffff;
> }

Disregard the issue with ip_checksum(). -fsanitize=undefined gave the clue. It was indeed UB, just not where I expected it. Fixing it right now:

ping-raw.c:206:16: runtime error: member access within misaligned address 0x7f4bbbd00a3e for type 'struct ip', which requires 4 byte alignment
0x7f4bbbd00a3e: note: pointer points here
  6f 6a 08 00 45 00  00 54 c3 84 40 00 40 01  00 00 c0 a8 7a f6 c0 a8  7a 01 00 00 00 00 00 00  00 00
              ^