Re: tcp_rst() complications

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 26 Feb 2025 20:05:25 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Amongst other things, I spotted some additional complications using
> tcp_rst() in the migration path (some of which might also have
> implications in other contexts).  These might be things we can safely
> ignore, at least for now, but I haven't thought through them enough to
> be sure.
> 
> 1) Sending RST to guest during migration
> 
> The first issue is that tcp_rst() will send an actual RST to the guest
> on the tap interface.  During migration, that means we're sending to
> the guest while it's suspended.  At the very least that means we
> probably have a much higher that usual chance of getting a queue full
> failure writing to the tap interface, which could hit problem (2).
> 
> But, beyond that, with vhost-user that means we're writing to guest
> memory while the guest is suspended.  Kind of the whole point of the
> suspended time is that the guest memory doesn't change during it, so
> I'm not sure what the consequences will be.

If I recall correctly I checked this and something in the vhost-user
code will tell us that the queue is not ready yet, done.

Ideally we want to make sure we queue those, but queue sizes are finite
and I don't think we can guarantee we can pile up 128k RST segments.

Right now I would check that the functionality is not spectacularly
broken (I looked into that briefly, QEMU didn't crash, guest kept
running, but I didn't check further than that). If we miss a RST too
bad, things will time out eventually.

As a second step we could perhaps introduce a post-migration stage and
move calling tcp_rst() to there if the connection is in a given state?

> Now, at the moment I
> think all our tcp_rst() calls are either on the source during rollback
> (i.e. we're committed to resuming only on the source) or on the target
> past the point of no return (i.e. we're committed to resuming only on
> the target).  I suspect that means we can get away with it, but I do
> worry this could break something in qeme by violating that assumption.
> 
> 2) tcp_rst() failures
> 
> tcp_rst() can fail if tcp_send_flag() fails.  In this case we *don't*
> change the events to CLOSED.  I _think_ that's a bug: even if we
> weren't able to send the RST to the guest, we've already closed the
> socket so the flow is dead.  Moving to CLOSED state (and then removing
> the flow entirely) should mean that we'll resend an RST if the guest
> attempts to use the flow again later.
> 
> But.. I was worried there might be some subtle reason for not changing
> the event state in that case.

Not very subtle: my original idea was that if we fail to send the RST,
we should note that (by not moving to CLOSED) and try again from a
timer.

In practice I've never observed a tcp_send_flag() failure so I'm not
sure if that mechanism even works. Moving the socket to CLOSED sounds
totally okay to me, surely simpler, and probably more robust.

-- 
Stefano