On Fri, Feb 18, 2022 at 04:03:28PM +0100, Florian Obser wrote:
> On 2022-02-18 12:17 +10, Jonathan Matthew <[email protected]> wrote:
> > The only thing ping uses to determine whether a received icmp echo reply
> > packet is a
> > response to one of its requests is the 16 bit icmp ident field. If you
> > ping enough
> > stuff at the same time, eventually you'll have two concurrent pings using
> > the same ident,
> > and they will both see each other's replies. Since we do tricky MAC stuff
> > on the ping
> > payload, this results in signature mismatches that look like this:
> >
> > PING 172.23.94.210 (172.23.94.210): 56 data bytes
> > 64 bytes from 172.23.94.210: icmp_seq=0 ttl=253 time=0.820 ms
> > 64 bytes from 172.23.94.210: icmp_seq=1 ttl=253 time=0.419 ms
> > 64 bytes from 172.23.94.210: icmp_seq=2 ttl=253 time=0.369 ms
> > signature mismatch!
> > 64 bytes from 172.23.94.210: icmp_seq=3 ttl=253 time=0.273 ms
> >
> > --- 172.23.94.210 ping statistics ---
> > 4 packets transmitted, 5 packets received, -- somebody's duplicating
> > packets!
> > round-trip min/avg/max/std-dev = 0.273/0.376/0.820/0.265 ms
> >
> > ping is counting the packet with the signature mismatch as a reply it
> > received, and it
> > prints a misleading message about duplicated packets because it got more
> > replies than
> > the number of requests it sent.
> >
> > I think it would be more helpful not to count signature mismatch packets as
> > replies.
> > If you're actually getting corrupted replies, I'd say that's more like
> > packet loss
> > than normal operation. If you're getting extra replies due to ident
> > collisions, this
> > will result in ping sending and receiving the expected number of packets.
> >
> > Printing the source address and sequence number on signature mismatches
> > would also help.
> > I would have figured this out much quicker had ping told me the mismatch
> > packets were
> > from a completely different source. For example:
> >
> > PING 172.23.94.210 (172.23.94.210): 56 data bytes
> > 64 bytes from 172.23.94.210: icmp_seq=0 ttl=253 time=2.645 ms
> > 64 bytes from 172.23.94.210: icmp_seq=1 ttl=253 time=1.360 ms
> > 64 bytes from 172.23.94.210: icmp_seq=2 ttl=253 time=0.506 ms
> > 64 bytes from 172.23.94.210: icmp_seq=3 ttl=253 time=0.615 ms
> > signature mismatch from 10.138.79.45: icmp_seq=0
> > 64 bytes from 172.23.94.210: icmp_seq=4 ttl=253 time=0.431 ms
> >
> > --- 172.23.94.210 ping statistics ---
> > 5 packets transmitted, 5 packets received, 0.0% packet loss
> > round-trip min/avg/max/std-dev = 0.431/1.111/2.645/0.835 ms
> >
> > ok?
>
> OK florian
>
> I think we can go further and also check the from address in the echo
> reply case, like this.
>
> If something on the path is so confused as to answer to our pings with
> the wrong source address I think it's tcpdump time...
>
> Feel free to put this in at the same time if you agree.
I considered doing this, but I think I'd rather have ping print out
anything it sees with the same ident, as long as it doesn't get confused
and mess up its statistics.
>
> diff --git sbin/ping/ping.c sbin/ping/ping.c
> index 6fa634bca3e..e47baa8912c 100644
> --- sbin/ping/ping.c
> +++ sbin/ping/ping.c
> @@ -181,6 +181,9 @@ char *hostname;
> int ident; /* random number to identify our packets */
> int v6flag; /* are we ping6? */
>
> +struct sockaddr_in dst4;
> +struct sockaddr_in6 dst6;
> +
> /* counters */
> int64_t npackets; /* max packets to transmit */
> int64_t nreceived; /* # of packets we got back */
> @@ -243,8 +246,8 @@ main(int argc, char *argv[])
> struct addrinfo hints, *res;
> struct itimerval itimer;
> struct sockaddr *from, *dst;
> - struct sockaddr_in from4, dst4;
> - struct sockaddr_in6 from6, dst6;
> + struct sockaddr_in from4;
> + struct sockaddr_in6 from6;
> struct cmsghdr *scmsg = NULL;
> struct in6_pktinfo *pktinfo = NULL;
> struct icmp6_filter filt;
> @@ -1285,6 +1288,13 @@ pr_pack(u_char *buf, int cc, struct msghdr *mhdr)
> }
>
> if (echo_reply) {
> + if (v6flag) {
> + if (memcmp(&dst6, from, sizeof(dst6)) != 0)
> + return; /* 'Twas not our ECHO */
> + } else {
> + if (memcmp(&dst4, from, sizeof(dst4)) != 0)
> + return; /* 'Twas not our ECHO */
> + }
> ++nreceived;
> if (cc >= ECHOLEN + ECHOTMLEN) {
> SIPHASH_CTX ctx;
> @@ -1302,7 +1312,10 @@ pr_pack(u_char *buf, int cc, struct msghdr *mhdr)
>
> if (timingsafe_memcmp(mac, &payload.mac,
> sizeof(mac)) != 0) {
> - printf("signature mismatch!\n");
> + printf("signature mismatch from %s: "
> + "icmp_seq=%u\n", pr_addr(from, fromlen),
> + ntohs(seq));
> + --nreceived;
> return;
> }
> timinginfo=1;
>
>
> >
> > Index: ping.c
> > ===================================================================
> > RCS file: /cvs/src/sbin/ping/ping.c,v
> > retrieving revision 1.245
> > diff -u -p -r1.245 ping.c
> > --- ping.c 12 Jul 2021 15:09:19 -0000 1.245
> > +++ ping.c 18 Feb 2022 01:52:22 -0000
> > @@ -1302,7 +1302,10 @@ pr_pack(u_char *buf, int cc, struct msgh
> >
> > if (timingsafe_memcmp(mac, &payload.mac,
> > sizeof(mac)) != 0) {
> > - printf("signature mismatch!\n");
> > + printf("signature mismatch from %s: "
> > + "icmp_seq=%u\n", pr_addr(from, fromlen),
> > + ntohs(seq));
> > + --nreceived;
> > return;
> > }
> > timinginfo=1;
> >
>
> --
> I'm not entirely sure you are real.
