Re: [PATCH v1] migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply

[Fabiano Rosas <farosas@suse.de>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 15 Apr 2026 at 15:39, Fabiano Rosas <farosas@suse.de> wrote:
>>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>> > Use QAPI_CLONE_MEMBERS instead of making an assignment. The QAPI
>> > method makes the handling of the TLS strings more intuitive because it
>> > clones them as well.
>> >
>> > This also fixes a segfault when a NULL TLS option is accessed as part
>> > of a validation check for another option (e.g. in the zero-copy +
>> > multifd compression case). Details follow:
>> >
>> > Currently, after copying s->parameters to the temporary
>> > MigrationParameters object before migrate_params_check(), the
>> > references in temporary object to the TLS options are dropped, either
>> > because:
>> >
>> > a) the user set a new option, in which case that's fine as
>> >    s->parameters still holds the reference to the old memory or,
>> >
>> > b) the user did not set a new option, in which case keeping the
>> >    references in the temporary object would later cause them to be
>> >    freed along with it, leading to double-free when s->parameters is
>> >    also freed later on.
>> >
>> > In this second case, it was overlooked that the TLS options can be
>> > accessed already during migrate_params_check() as part of validation
>> > of another option. Those pointers should not have been cleared.
>> >
>> > Using QAPI_CLONE_MEMBERS fixes the issue because the temporary object
>> > is not stealing a reference from s->parameters anymore.
>> >
>> > Fixes: aed97f0563 ("migration: Normalize tls arguments")
>> > Reported-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
>> > Link: https://lore.kernel.org/r/a65a1049-9f19-460a-8e27-a62bb30d2727@maciej.szmigiero.name
>> > Reviewed-by: Peter Xu <peterx@redhat.com>
>> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> > ---
>> > NOTE#1: For the release we could have a simpler fix just checking for
>> > NULL, but that would allow two unsupported configurations to be
>> > accepted: zero-copy with either multifd compression or TLS.
>> >
>>
>> Peter Xu just pointed out on IRC that it's actually only the zero-copy +
>> TLS configuration that will be allowed with the simpler fix. I think
>> it's best we go that route instead and hold this patch until after the
>> release.
>
> rc4 has already been tagged and I really don't want to put anything
> more into 11.0 unless it is a super high priority release-blocker
> kind of a bug. The fact that this is a fix for a commit that's
> been in git for pretty much the whole of the 11.0 cycle without anybody
> noticing earlier suggests it probably isn't that important ?
>
> If we want to put it in then the commit message needs to clearly
> state the severity (e.g. "everybody trying to use migration
> hits a segfault") that makes it release-critical.
>
> Otherwise we should put this into 11.1 and with the usual cc:stable
> so it is backported to the 11.0 stable series.
>

Makes sense.

@Peter Xu, do you agree with leaving this to stable? Given that
it needs multifd and zero-copy-send to be both set, I'd say it doesn't
have that large of a user base.

@Maciej, what's the impact for you?