On Tue, 1 Feb 2022 14:53:21 -0400
Jason Gunthorpe [off-list ref] wrote:

On Tue, Feb 01, 2022 at 11:31:44AM -0700, Alex Williamson wrote:

quoted

quoted

+	bool have_p2p = device->migration_flags & VFIO_MIGRATION_P2P;
+
 	if (cur_fsm >= ARRAY_SIZE(vfio_from_fsm_table) ||
 	    new_fsm >= ARRAY_SIZE(vfio_from_fsm_table))
 		return VFIO_DEVICE_STATE_ERROR;
 
-	return vfio_from_fsm_table[cur_fsm][new_fsm];
+	if (!have_p2p && (new_fsm == VFIO_DEVICE_STATE_RUNNING_P2P ||
+			  cur_fsm == VFIO_DEVICE_STATE_RUNNING_P2P))
+		return VFIO_DEVICE_STATE_ERROR;  

new_fsm is provided by the user, we pass set_state.device_state
directly to .migration_set_state.  We should do bounds checking and
compatibility testing on the end state in the core so that we can  

This is the core :)

But this is the wrong place, we need to do it earlier rather than when
we're already iterating next states.  I only mention core to avoid that
I'm suggesting a per driver responsibility.

quoted

return an appropriate -EINVAL and -ENOSUPP respectively, otherwise
we're giving userspace a path to put the device into ERROR state, which
we claim is not allowed.  

Userspace can never put the device into error. As the function comment
says VFIO_DEVICE_STATE_ERROR is returned to indicate the arc is not
permitted. The driver is required to reflect that back as an errno
like mlx5 shows:

+		next_state = vfio_mig_get_next_state(vdev, mvdev->mig_state,
+						     new_state);
+		if (next_state == VFIO_DEVICE_STATE_ERROR) {
+			res = ERR_PTR(-EINVAL);
+			break;
+		}

We never get the driver into error, userspaces gets an EINVAL and no
change to the device state.

Hmm, subtle.  I'd argue that if we do a bounds and support check of the
end state in vfio_ioctl_mig_set_state() before calling
.migration_set_state() then we could remove ERROR from
vfio_from_fsm_table[] altogether and simply begin
vfio_mig_get_next_state() with:

	if (cur_fsm = ERROR)
		return ERROR;

Then we only get to ERROR by the driver placing us in ERROR and things
feel a bit more sane to me.

It is organized this way because the driver controls the locking for
its current state and thus the core code caller along the ioctl path
cannot validate the arc before passing it to the driver. The code is
shared by having the driver callback to the core to validate the
entire fsm arc under its lock.

P2P is defined in a way that if the endpoint is valid then the arc is
valid.  We skip intermediate unsupported states.  We need to do that
for compatibility.  So why do we care about driver locking to do that?
Thanks,

Alex