Re: [PATCH v4 08/33] migration/multifd: Allow premature EOF on TLS incoming channels

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Feb 07, 2025 at 10:17:19AM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Thu, Feb 06, 2025 at 02:32:12PM -0300, Fabiano Rosas wrote:
>> >> > In any case we'd still need some kind of a compatibility behavior for
>> >> > the TLS bit stream emitted by older QEMU versions (which is always
>> >> > improperly terminated).
>> >> >
>> >> 
>> >> There is no compat issue. For <= 9.2, QEMU is still doing an extra
>> >> multifd_send_sync_main(), which results in an extra MULTIFD_FLAG_SYNC on
>> >> the destination and it gets stuck waiting for the
>> >> RAM_SAVE_FLAG_MULTIFD_FLUSH that never comes. Therefore the src always
>> >> closes the connection before dst reaches the extra recv().
>> >> 
>> >> I test migration both ways with 2 previous QEMU versions and the
>> >> gnutls_bye() series passes all tests. I also put an assert at
>> >> tlssession.c and never triggers for GNUTLS_E_PREMATURE_TERMINATION. The
>> >> MULTIFD_FLAG_EOS should behave the same.
>> >
>> > Which are the versions you tried?  As only 9.1 and 9.2 has 637280aeb2, so I
>> > wonder if the same issue would hit too with 9.0 or older.
>> 
>> Good point. 9.0 indeed breaks.
>> 
>> >
>> > I'd confess I feel unreliable relying on the side effect of 637280aeb2,
>> > because fundamentally it works based on the fact that multifd threads need
>> > to be kicked out by the main load thread SYNC event on dest QEMU to avoid
>> > the readv() from going wrong.
>> >
>> 
>> We're relying on the opposite: mutlifd_recv NOT getting kicked. Which is
>> a bug that 1d457daf86 fixed.
>> 
>> > What I'm not sure here is, is it sheer luck that the main channel SYNC will
>> > always arrive _before_ pre-mature terminations of the multifd channels?  It
>> > sounds like it could also happen when the multifd channels got its
>> > pre-mature termination early, before the main thread got the SYNC.
>> 
>> You lost me here, what main channel sync? Its the MULTIFD_FLAG_SYNC that
>> puts the recv thread in the "won't see the termination" state and that
>> is serialized:
>> 
>>    SEND                        RECV
>>    -------------------------+----------------------------
>> 1  multifd_send_sync_main()
>> 2  pending_sync==true,
>> 3  send thread sends SYNC      recv thread gets SYNC
>> 4  <some work>                 recv gets stuck.
>> 5  multifd_send_shutdown()     <time passes>
>> 6  shutdown()                  multifd_recv_shutdown()
>>                                recv_terminate_threads()
>>                                recv exits without recv()
>> 
>> In other words, RECV would need to see the shutdown (6) before the SYNC
>> (3), which I don't think it possible.
>
> Ah yeah, I somehow remembered we sent a SYNC in the main channel but forgot
> to push the per-channel SYNC.  I got it the other way round.  Yeah if data
> is always ordered with shutdown() effect on recv then it seems in order.
>
>> 
>> >
>> > Maybe we still need a compat property at the end..
>> 
>> This is actually similar to preempt_pre_7_2, what about:
>> 
>>     /*
>>      * This variable only makes sense when set on the machine that is
>>      * the destination of a multifd migration with TLS enabled. It
>>      * affects the behavior of the last send->recv iteration with
>>      * regards to termination of the TLS session. Defaults to true.
>>      *
>>      * When set:
>>      *
>>      * - the destination QEMU instance can expect to never get a
>>      *   GNUTLS_E_PREMATURE_TERMINATION error. Manifested as the error
>>      *   message: "The TLS connection was non-properly terminated".
>>      *
>>      * When clear:
>>      *
>>      * - the destination QEMU instance can expect to see a
>>      *   GNUTLS_E_PREMATURE_TERMINATION error in any multifd channel
>>      *   whenever the last recv() call of that channel happens after
>>      *   the source QEMU instance has already issued shutdown() on the
>>      *   channel. This is affected by (at least) commits 637280aeb2
>>      *   and 1d457daf86.
>
> If we want to reference them after all, we could use another sentence to
> describe the effects:
>
>        *   Commit 637280aeb2 (since 9.1) introduced a side effect to cause
>        *   pre-mature termination not happen, while commit 1d457daf86
>        *   (since 10.0) can unexpectedly re-expose the pre-mature
>        *   termination issue.
>

I'll add this.

>>      *
>>      * NOTE: Regardless of the state of this option, a premature
>>      * termination of the TLS connection might happen due to error at
>>      * any moment prior to the last send->recv iteration.
>>      */
>>     bool multifd_clean_tls_termination;
>> 
>> And I think the more straight-forward implementation is to incorporate
>> Maciej's premature_ok patches (in some form), otherwise that option will
>> have to take effect on the QIOChannel which is a layering violation.
>
> If we take Dan's comment into account:
>
> https://lore.kernel.org/r/Z6I86e-hzJAlxk0r@redhat.com
>
> It means whenever multifd recv thread invokes the iochannel API it will use
> multifd_clean_tls_termination to decide QIO_CHANNEL_READ_RELAXED_EOF flag
> to pass in.  I hope this is not layer violation, or I could miss something..

Yes, we need that.

>
> So if we're on the same page we need that knob, to make this series easier
> we could make it two steps:
>
>   - Step 1: introduce the parameter and QIO_CHANNEL_READ_RELAXED_EOF, set
>     it default to false.
>
>   - Step 2: Your other RFC series to implement gnutls_bye(), at last make
>     it a compat property and switch default true.
>
> Then Maciej only needs step 1, it looks to me.

I'm sending everything as a v2 in a moment. We can cherry-pick from
there.