tcpip/stack: incorrect UDP UnknownPortErrors increment

[xjasonlyu]

Description

It started with a tool I wrote and I noticed the increasing UnknownPortErrors counts.

E.g., say we set a UDPTransportProtocolHandler like this:

udpForwarder := udp.NewForwarder(s, func(r *udp.ForwarderRequest) {
    var wq waiter.Queue
    ep, err := r.CreateEndpoint(&wq)
    if err != nil {
        return
    }
    // ...
})
s.SetTransportProtocolHandler(udp.ProtocolNumber, udpForwarder.HandlePacket)

And when a UDP packet arrives to the stack, it comes to nic and is handled by (*nic).stack.demux.deliverPacket in (*nic).DeliverTransportPacket:

https://github.com/google/gvisor/blob/fcd82c8ebf671e268fa16da3f172ea6679eb45ec/pkg/tcpip/stack/nic.go#L802-L838

Then, in the (*transportDemuxer).deliverPacket, it calls the eps.findEndpointLocked(id) but nil would be returned (because it's a new arrived packet, not registered yet).

https://github.com/google/gvisor/blob/fcd82c8ebf671e268fa16da3f172ea6679eb45ec/pkg/tcpip/stack/transport_demuxer.go#L544-L596

So in the next if statement, since it's UDP packet, an UnknownPortErrors would be increased.

if protocol == header.UDPProtocolNumber {
    d.stack.stats.UDP.UnknownPortErrors.Increment()
}

However, there should not be an error here, since we have the defaultHandler to handle it later. Therefore, I think the right behavior for the stack is check if defaultHandler is nil and then decides the UDP.UnknownPortErrors.Increment().

Steps to reproduce

mentioned above.

runsc version

nil

docker version (if using docker)

nil

uname

nil

kubectl (if using Kubernetes)

nil

repo state (if built from source)

nil

runsc debug logs (if available)

nil

[kevinGC]

Is the increased UnknownPortErrors breaking anything? I feel that this is working as intended, and that in this particular use case it's not a useful value to you.

UnknownPortErrors tracks the stack being unable to find an endpoint for a packet. If netstack is used as described above, then it's simply counting a value that is irrelevant to this use case. For other use cases, the value is more useful.

[xjasonlyu]

No, it didn’t break anything. I just previously thought this shouldn’t be an error for this particular use case :)