Re: [PATCH v3 0/2] migration/tls: Graceful shutdowns for main and postcopy channels

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> v3:
> - Patch 1
>   - Update qcrypto_tls_session_read() doc to reflect the new retval [Dan]
>   - Update commit message to explain the qatomic_read() change [Dan]
> - Patch 2 (old)
>   - Dropped for now, more at the end
>
> This is v3 of the series.
>
> Fabiano fixed graceful shutdowns for multifd channels previously:
>
> https://lore.kernel.org/qemu-devel/20250206175824.22664-1-farosas@suse.de/
>
> However we can still see an warning when running preempt unit test on TLS,
> even though migration functionality will not be affected:
>
> QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test --full -r /x86_64/migration/postcopy/preempt/tls/psk
> ...
> qemu-kvm: Cannot read from TLS channel: The TLS connection was non-properly terminated.
> ...
>
> It turns out this is because the crypto code only passes the ->shutdown
> field into the read function, however that value can change concurrently in
> another thread by a concurrent qio_channel_shutdown().
>
> Patch 1 should fix this issue.
>
> Patch 2 is something I found when looking at this problem, there's no known
> issues I am aware of with them, however I still think they're logically
> flawed, so I post them together here.
>
> Please review, thanks.
>
> ============= ABOUT OLD PATCH 2 ===================
>
> I dropped it for now to unblock almost patch 1, because patch 1 will fix a
> real warning that can be triggered for not only qtest but also normal tls
> postcopy migration.
>
> While I was looking at temporary settings for multifd send iochannels to be
> blocking always, I found I cannot explain how migration_tls_channel_end()
> currently works, because it writes to the multifd iochannels while the
> channels should still be owned (and can be written at the same time?) by
> the sender threads.  It sounds like a thread-safety issue, or is it not?
>

IIUC, the multifd channels will be stuck at p->sem because this is the
success path so migration will have already finished when we reach
migration_cleanup(). The ram/device state migration will hold the main
thread until the multifd channels finish transferring.

> Peter Xu (2):
>   io/crypto: Move tls premature termination handling into QIO layer
>   migration: Make migration_has_failed() work even for CANCELLING
>
>  include/crypto/tlssession.h | 10 +++-------
>  crypto/tlssession.c         |  7 ++-----
>  io/channel-tls.c            | 21 +++++++++++++++++++--
>  migration/migration.c       |  3 ++-
>  4 files changed, 26 insertions(+), 15 deletions(-)