IP_RECVERR can prevent graceful connection termination

[stormshield-fabs]

The recently added support of IP_RECVERR (#2646) has an unintended impact on the normal operation of the socket: when an error is received on the socket, the next syscall (recvmmsg or sendmsg for quinn) will ignore the data and return this error (once for each enqueued error).

This is described (although quite discreetly I think) in the recv(2) manpage:

After an error has been passed, the pending socket error is regenerated based on the next queued error and will be passed on the next socket operation.

In practice, this does not have a huge functional impact since reads and writes are retried in quinn.

However, we stumbled upon this when chasing a lost CONNECTION_CLOSE frame that should have been sent when closing the Connection. It turned out the packet containing this frame was absorbed by sendmsg, since an ICMP error had occured right before it was called. This error is only meant to be a notification that an error was enqueued and is not related to the packet we're trying to sent (the syscall returns an error even if the error was for a different remote address). Because close notifications are not retried, the remote peer never gets it.

I believe this was understood in a previous implementation, since this comment hints at a refactor from a design that pulled errors immediately to the external getter design : https://github.com/quinn-rs/quinn/pull/2421/changes#r2450585268

To move forward, we would like to suggest adding an option to let users choose if the IP_RECVERR flag is passed on the socket.

Additionally, we think both recvmmsg and sendmsg calls should be wrapped to capture such errors and emit a notification that the user could receive to know if they should call the new recv_transport_error method. Then, the syscall should be retried (and that would fix the silently dropped CONNECTION_CLOSE).

[djc]

@thomaseizinger @mxinden thoughts?

[syszery]

I was able to reproduce the sendmsg absorption locally with a minimal test: send to a closed port, then send to a valid receiver without draining the queue -> the second packet never arrives.

Regarding the opt-out suggestion: I'm not sure it really resolves the issue, since users who disable IP_RECVERR lose the transport error notifications, while users who keep it still hit the problem.

I've sketched the following approach, which seems to work:

• Add a bounded ring buffer VecDeque<TransportError> to UdpSocketState.
• When sendmsg returns an unexpected error on Linux, try to drain one entry from the error queue.
  • If successful, push it to the buffer and retry the send.
  • If not, the error is likely genuine and should get propagated normally.
• Update recv_transport_error to check this buffer before going to the kernel, so no errors are silently lost.

This makes the regression test pass. I haven't confirmed the recvmmsg path yet, but it likely needs similar treatment.

[stormshield-fabs]

Thank you for looking into this!

I agree the opt-out suggestion does not resolve the issue, but it would make this feature zero-cost for users who don't need it, since we'll likely have to do more bookkeeping than initially thought.

From my experiments, retrying the syscall without draining the queue works and the packets get sent as expected with the second call.

I'm not sure recv_transport_error is a sufficient abstraction for this feature. The user cannot know if or when they should call it, so they have to poll. This is why I proposed the alternative design with a notification primitive. I don't believe we need to drain the queue, so we could just count error notifications and let the end user drain them from the kernel when they want?

[syszery]

Thanks for the clarification. I did a bit more testing on my side.

First, I verified @stormshield-fabs's observation that adding a single retry at the end of send() after an unexpected sendmsg() error seems to be enough.

The change is just a single one-time retry:

_ => {
    // existing GSO handling...
    // existing EINVAL fallback...

    #[cfg(any(target_os = "linux", target_os = "android"))]
    { 
        if !retried {
            retried = true; 
            continue; 
        }
    } 
    return Err(e); 
}

I then extended the existing recv_transport_error test to check the behavior when multiple ICMP unreachable errors are queued before calling the modified send() to transmit a packet to a valid receiver. The packet was delivered successfully after the single retry. All three transport errors were still retrievable via recv_transport_error().

quinn/quinn-udp/tests/tests.rs

Line 488 in 953b466

fn recv_transport_error() {

Unless I'm missing something, this suggests that a single retry is sufficient even when multiple errors are queued, and that the queued transport errors are neither consumed nor lost by doing so. If that's the expected kernel behavior, the fix may be considerably simpler than previously suggested.