icmp: workaround for gvisor's fake ICMP echo

[Lanius-collaris]

Not working. I'm trying inserting code in other positions.

[ignoramous]

Thanks.

I wonder if this forceful default handling of ICMP is a bug on gvisor upstream despite having set a ICMP handler? It was working as I'd expect it to in previous (believe a year or so ago) gvisor version.

[Lanius-collaris]

It was working as I'd expect it to in previous (believe a year or so ago) gvisor version.

🤔Will current handler work if these lines are deleted? https://github.com/google/gvisor/blob/3a9ba17351571e343e16caec2e44215d8adc400f/pkg/tcpip/network/ipv6/icmp.go#L694 https://github.com/google/gvisor/blob/3a9ba17351571e343e16caec2e44215d8adc400f/pkg/tcpip/network/ipv4/icmp.go#L449

[Lanius-collaris]

I wonder if this forceful default handling of ICMP is a bug on gvisor upstream despite having set a ICMP handler? It was working as I'd expect it to in previous (believe a year or so ago) gvisor version.

I have a theory that these lines were written for inbound ICMP echo requests (packet sockets can receive them).

@kevinGC Could you answer ignoramous' question?

[kevinGC]

Sorry, what's the specific question? I'm missing some context and not sure what 'fake' means in the title here.

I don't remember any recent changes to ICMP handling. If you've set a custom handler for ICMP packets and the stack is still doing its own ICMP handling, I think that's because ICMP handling is tightly coupled to the ipv4 and ipv6 packages. Nobody has tried to split them AFAIK, so there's no easy way to turn off just ICMP.

Packet sockets should get ICMP messages regardless of the code in the ipv4 and ipv6 packages; it happens earlier in packaet processing (around nic.go:DeliverLinkPacket).

BTW @nybidari is the gVisor networking lead these days.

[ignoramous]

Sorry, what's the specific question? I'm missing some context and not sure what 'fake' means in the title here.

Netstack replies to ICMP messages on its own and does not wait on the Handler set via stack.SetTransportProtocolHandler (unlike for TCP and UDP, for which Netstack processes packets only if the Handler does not).

I should note that Netstack we use is setup with Spoofing and is Promiscuous (ref), in addition to being the default gateway (0.0.0.0 and :: routes); HandleLocal & NICForwarding opts are also set to false (ref).

Packet sockets should get ICMP messages regardless of the code in the ipv4 and ipv6 packages;

This happens even today. The Handler does get those packets, but Netstack replies to those packets before the Handler has had the chance.

Like @Lanius-collaris points out, ICMP is handled internally (here) before being delivered to the Handler (here?)? For us, it is enough if ICMP requests (echo, specifically) (which aren't tied to an existing UDP/TCP sesssion), like those usually coming from ping or equivalent progs, are sent to the Handler before Netstack looks at them.

[Lanius-collaris]

I have a theory that these lines were written for inbound ICMP echo requests (packet sockets can receive them).

Packet sockets should get ICMP messages regardless of the code in the ipv4 and ipv6 packages; it happens earlier in packaet processing (around nic.go:DeliverLinkPacket).

"packet sockets" here mean sockets created with socket(AF_PACKET, int socket_type, int protocol) on linux, not tcpip.Endpoint provided by gvisor.dev/gvisor/pkg/tcpip/transport/packet module.

If you've set a custom handler for ICMP packets and the stack is still doing its own ICMP handling, I think that's because ICMP handling is tightly coupled to the ipv4 and ipv6 packages. Nobody has tried to split them AFAIK, so there's no easy way to turn off just ICMP.

I have tried and succeeded. 😆 Stack.IPTables() is powerful. https://github.com/Lanius-collaris/gvisor-playground/commit/5403bf8553c8d77497e57b4f38bffd554597fa9a

[ignoramous]

Can we create a bool icmp6IPtablesHack (or if you can think of a better name), which when set to true uses these changes, but otherwise falls back to running existing code? If that's too much work, I can take it up, too.

[Lanius-collaris]

Can we create a bool icmp6IPtablesHack (or if you can think of a better name), which when set to true uses these changes, but otherwise falls back to running existing code? If that's too much work, I can take it up, too.

Even if gvisor changes the behavior ( in pkg/tcpip/network/ipv6/icmp.go and pkg/tcpip/network/ipv6/ipv6.go ), you still need to recompile firestack, and it's very easy to remove ICMPHackTarget, so I think a new option is not needed.
I will leave the maintenance to you.

[ignoramous]

Gotcha.

Even if gvisor changes the behavior ( in pkg/tcpip/network/ipv6/icmp.go and pkg/tcpip/network/ipv6/ipv6.go ), you still need to recompile firestack

Recompiling firestack is no big deal? Each commit batch has a compiled artifact pushed to GitHub Packages and Maven Central... and we've managed to upgrade the gvisor@go module from upstream without much fanfare (at least until now).

[Lanius-collaris]

Recompiling firestack is no big deal? Each commit batch has a compiled artifact pushed to GitHub Packages and Maven Central... and we've managed to upgrade the gvisor@go module from upstream without much fanfare (at least until now).

I means rethink app can't call settings.XXXX() to use stack.SetTransportProtocolHandler(icmp.ProtocolNumber6, …) for ICMPv6 without recompiling, do you want to keep two implementations?