linux-kernel - RE: [PATCH vfio] vfio/pds: Rework and simplify reset flows

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 5 Feb 2024 06:58:51 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Brett Creeley <brett.creeley@....com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"alex.williamson@...hat.com" <alex.williamson@...hat.com>,
	"shameerali.kolothum.thodi@...wei.com"
	<shameerali.kolothum.thodi@...wei.com>, "yishaih@...dia.com"
	<yishaih@...dia.com>, "jgg@...pe.ca" <jgg@...pe.ca>
CC: "shannon.nelson@....com" <shannon.nelson@....com>
Subject: RE: [PATCH vfio] vfio/pds: Rework and simplify reset flows

> From: Brett Creeley <brett.creeley@....com>
> Sent: Saturday, January 27, 2024 2:32 AM
> 
> The current logic for handling resets based on
> whether they were initiated from the DSC or
> host/VMM is slightly confusing and incorrect.
> The incorrect behavior can cause the VF device
> to be unusable on the destination on failed
> migrations due to incompatible configurations.
> Fix this by setting the state back to
> VFIO_DEVICE_STATE_RUNNING when an FLR is
> triggered, so the VF device is put back in
> an "initial" pre-configured state after failures.

any reason for putting short lines (<50 chars) in commit msg?

> 
> Also, while here clean-up the reset logic to
> make the source of the reset more obvious.

as a fix the a 'Fixed' tag is preferred and CC stable

also separate the real fix from the cleanup so stable kernel doesn't need
to backport unnecessary code.

btw the commit msg is not clear to me. It says fixing the problem
by setting the state to _ERROR for the DSC path and to _RUNNING for
the FLR path.

But looks it's already such case with old code:

pds_vfio_recovery()
	pds_vfio->deferred_reset = true;
	pds_vfio->deferred_reset_state = VFIO_DEVICE_STATE_ERROR;

pds_vfio_reset()
	pds_vfio->deferred_reset = true;
	pds_vfio->deferred_reset_state = VFIO_DEVICE_STATE_RUNNING;

pds_vfio_state_mutex_unlock()
	if (pds_vfio->deferred_reset) {
		...
		pds_vfio->state = pds_vfio->deferred_reset_state;
		...
	}

it's same as what this patch does:

pds_vfio_recovery()
	pds_vfio->deferred_reset_type = PDS_VFIO_DEVICE_RESET;

pds_vfio_reset()
	pds_vfio->deferred_reset_state = PDS_VFIO_HOST_RESET;

pds_vfio_state_mutex_unlock()
	if (pds_vfio->deferred_reset) {
		...
		if (pds_vfio->deferred_reset_type == PDS_VFIO_HOST_RESET)
			pds_vfio->state = VFIO_DEVICE_STATE_RUNNING;
		else
			pds_vfio->state = VFIO_DEVICE_STATE_ERROR;
		...
	}

looks the actual functional difference is from below change:

> @@ -32,13 +32,14 @@ void pds_vfio_state_mutex_unlock(struct
> pds_vfio_pci_device *pds_vfio)
>  	mutex_lock(&pds_vfio->reset_mutex);
>  	if (pds_vfio->deferred_reset) {
>  		pds_vfio->deferred_reset = false;
> -		if (pds_vfio->state == VFIO_DEVICE_STATE_ERROR) {
> -			pds_vfio_put_restore_file(pds_vfio);
> -			pds_vfio_put_save_file(pds_vfio);
> +		pds_vfio_put_restore_file(pds_vfio);
> +		pds_vfio_put_save_file(pds_vfio);

above two are changed from conditional to always.

> +		if (pds_vfio->deferred_reset_type == PDS_VFIO_HOST_RESET)
> {
> +			pds_vfio->state = VFIO_DEVICE_STATE_RUNNING;
> +		} else {
>  			pds_vfio_dirty_disable(pds_vfio, false);

and this is now only for the DSC path.

need a better explanation here.