From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202502 header.b=WnGet0lE; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 4399B5A0622 for ; Thu, 27 Feb 2025 05:10:51 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202502; t=1740629434; bh=V4muvwXwau3tp2eJsjpFFbTgBuKOjeefuXRkW8jJTp8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=WnGet0lEEC6D5H8esUT9VQ7AtgMOTmpgmb+qWIPHRpNSTkYVJiMqKCngPMJDPXfdn FIxdljy2vQQW8Hmc3rwY6pwbUm1nLUs4D7N6QEqzkco9WEuUb63bjsoHV11fWOfKo3 9IhTvWRWE0Ob+n1VoDLueOUXCpPhWTkv6g6PWc5sMsZR9qXjJCX9XBdWZ8ckqr6Co2 tx+bsmcYbmMFi36Ul4lFKrIzrXPgnvbkOk5rHvz5MGDJWyGpBwgqflqLUj2Ro65C0d em4hOJK+XRNTJ6mBRsRD9xsN+hGmpusvom5WohVJDZ8otkYarylnhJx9svyhAP18oz WeEQJJ2kdO1NQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Z3Hty3V3Jz4wyh; Thu, 27 Feb 2025 15:10:34 +1100 (AEDT) Date: Thu, 27 Feb 2025 15:10:32 +1100 From: David Gibson To: Stefano Brivio Subject: Re: tcp_rst() complications Message-ID: References: <20250226111507.166ed938@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="GSFMetayPfWO+mny" Content-Disposition: inline In-Reply-To: Message-ID-Hash: EP4MTIWCXAWIOU4LBWDLKA4Z5PE4PO2L X-Message-ID-Hash: EP4MTIWCXAWIOU4LBWDLKA4Z5PE4PO2L X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --GSFMetayPfWO+mny Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Feb 27, 2025 at 12:57:15PM +1100, David Gibson wrote: > On Wed, Feb 26, 2025 at 11:15:07AM +0100, Stefano Brivio wrote: > > On Wed, 26 Feb 2025 20:05:25 +1100 > > David Gibson wrote: > >=20 > > > 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. > > >=20 > > > 1) Sending RST to guest during migration > > >=20 > > > 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). > > >=20 > > > 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. > >=20 > > 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. >=20 > Ah, yes. I'd seen those "Got packet, but RX virtqueue is not usable > yet" messages, but didn't make the connection. Significantly, of course, that case also still returns 0 from tcp_send_flag() so we carry on with changing the state to CLOSED. > I had been concerned that might only happen when we tried to kick the > queue, after we'd touched guest memory, but that doesn't appear to be > the case. Ok, now I'm reasonably convinced this is ifne. >=20 > > 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. >=20 > Well that, plus with vhost-user we don't really have anywhere we can > queue them other than in guest memory. >=20 > > 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. > >=20 > > 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? > >=20 > > > 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. > > >=20 > > > 2) tcp_rst() failures > > >=20 > > > 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. > > >=20 > > > But.. I was worried there might be some subtle reason for not changing > > > the event state in that case. > >=20 > > 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. >=20 > Ok. Makes sense, but I don't think it's necessary. We can treat a > failure here as if the RST was lost on the wire: we just forget the > connection, and if the guest doesn't drop it we'll send it another RST > if it tries to send a packet to the connection we no longer know > about. I was wrong about this, and we hit this case in practice for a migration where the target has a different address (which I've now managed to test properly). With my draft fix, after the bind() failure we correctly We tcp_rst() the imported flow, but because the queue is disabled the RST isn't actually delivered to the guest. So, the (target) guest will now send mid-stream packets which no longer match any flow. Turns out flow-less packets which aren't SYN we simply ignore. That seems wrong, I think we should RST and/or send an ICMP before. I'm slightly worried it might be one of those cases where that was how it was originally supposed to work, but the realities of a hostile internet mean it turns out to be unwise. Then again, this is guest facing, so it won't usually be exposed to the whole internet. > > In practice I've never observed a tcp_send_flag() failure so I'm not > > sure if that mechanism even works. >=20 > I don't think the mechanism even exists. I don't think there's any > timer that does this, and more to the point, if we don't change events > we basically don't have any record that we already reset the connection. >=20 > > Moving the socket to CLOSED sounds > > totally okay to me, surely simpler, and probably more robust. >=20 > Ok. I'll go ahead with that. I still think this change is correct, and I'm going ahead with it, but I think we probably want another change on that to RST (or something) on flowless !SYN packets. --=20 David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson --GSFMetayPfWO+mny Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAme/5akACgkQzQJF27ox 2Gc44hAAjI6/O4MT5arAgjmrM8ZZOQ/BPGawfwJIMtKQJwe3F37WDYcU7cy7AI2q o5ZZK1xWpnxISnrS8NmJji/tSZ8JVr0kfBkpqIrLmi8fEpxRYfv2R0C84PrBrz5q Q9rOJ85xSc97I71WeThLjssf+m913XonzQkSGW+o5ofbwSOPS7jvsin8apO3TA9w JjhAARXkCdAQqJUcszpkvcceuuZKAHT/R0jzNvoD5AFFuSjQrqyb2ygN47LMeCNp 9bpHG11s0RRNN2duxqDPUYrVODjlYt4MrcZKpMSFaW7VEXt1SIERLRVC/tmWW/Ss 4zJ3+XgKxytIbC/F9vG740vWjwTDN0zLIkuhSCKFq+9LDQHS4dHDwGcqkEmxfjRe MnTOSa+DU7FtTNd5zlDZubZM0MmAayMSaC0UISXo0VT35Kv2fqNcxSf4OZTJ0oUc Mq4nsNi1anjwWZBj3rz/tj9vJlrWw6arIXgW+TdU3CKLjoQ9dHg1Qaqijxg7ACI9 sTpVww/EjmjhqGvBF0W6NxAmdwCmaKmXcVSlvNb4f2Lv0axOM50jr4LkGx8jzc81 ZWU2ysnJa0+p9KQ2B4V4TC8XxuV7FHhG/ykdZ2bnZ3UA0zfsy7XX+wBpf04o64r1 BsX/DDjdxYZ51iJMP78U1RZkU2GYdVm4b7cesI4RT9SZCzk8utM= =D9Hu -----END PGP SIGNATURE----- --GSFMetayPfWO+mny--