On Tue, Oct 26, 2021 at 05:16:44PM -0600, Alex Williamson wrote: > > @@ -471,6 +474,47 @@ mlx5vf_pci_migration_data_rw(struct mlx5vf_pci_core_device *mvdev, > > return count; > > } > > > > +/* This function is called in all state_mutex unlock cases to > > + * handle a 'defered_reset' if exists. > > + */ > > I refrained from noting it elsewhere, but we're not in net/ or > drivers/net/ here, but we're using their multi-line comment style. Are > we using the strong relation to a driver that does belong there as > justification for the style here? I think it is an oversight, tell Yishai you prefer the other format in drivers/vfio and it can be fixed > > @@ -539,7 +583,7 @@ static ssize_t mlx5vf_pci_mig_rw(struct vfio_pci_core_device *vdev, > > } > > > > end: > > - mutex_unlock(&mvdev->state_mutex); > > + mlx5vf_state_mutex_unlock(mvdev); > > I'm a little lost here, if the operation was to read the device_state > and mvdev->vmig.vfio_dev_state was error, that's already been copied to > the user buffer, so the user continues to see the error state for the > first read of device_state after reset if they encounter this race? Yes. If the userspace races ioctls they get a deserved mess. This race exists no matter what we do, as soon as the unlock happens a racing reset ioctl could run in during the system call exit path. The purpose of the locking is to protect the kernel from hostile userspace, not to allow userspace to execute concurrent ioctl's in a sensible way. Jason
