Re: [PATCH 1/1] migration/cpr: lock the cpr state

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Sep 16, 2026 at 03:24:28PM +0200, Denis V. Lunev wrote:
>> From: Denis V. Lunev <den@openvz.org>
>> 
>> The cpr state is a process wide registry of the resources that have to
>> survive an exec. Entries are added and dropped from realize and teardown
>> paths all over the tree, and a caller has no way to tell which thread the
>> next caller will run in.
>> 
>> The state carries no serialization of its own. It is safe today only
>> because its callers happen to hold the BQL, and that is a property of
>> where those callers live rather than anything the interface promises or
>> checks. A caller that is correct within its own subsystem can therefore
>> break this one from a distance: two that overlap corrupt the registry,
>> and the next lookup walks an entry that has already been freed.
>> 
>> Give the state a lock of its own instead of relying on that. Entries
>> move when a device is realized, when a resource is handed over, and when
>> a migration starts, so none of this is on a hot path and an uncontended
>> mutex costs nothing.
>> 
>> Take the lock in every accessor and across the save and load walks.
>> Holding it over the walk callbacks is safe because none of them comes
>> back through this interface. State that is not reached through these
>> accessors, such as the VFIO device list that hw/vfio manipulates
>> directly, stays outside it.
>> 
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Cc: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>> Cc: Peter Xu <peterx@redhat.com>
>> Cc: Fabiano Rosas <farosas@suse.de>
>> Cc: Mark Kanda <mark.kanda@oracle.com>
>> Cc: Ben Chaney <bchaney@akamai.com>
>> Cc: Steve Sistare <steven.sistare@oracle.com>
>
> Is there a real problem anywhere?
>
> My gut feeling is we shouldn't be randomly adding fds into cpr unless we're
> creating a new device or so, which should normally need bql already.
>
> To ask in another way, shall we simply assert bql_locked()?
>
> Thanks,

Fair questions, let's see what Denis says.

From my point of view, if the work is already done to use a lock, I'd
take it; one less place for us to bother when dealing with BQL issues.

(on the other hand, I'm not doing any active use or testing of cpr aside
from migration-test, so my opinion is the least important here)