Re: [PATCH v8 07/15] s390/vfio-ap: File ops called to save the vfio device migration state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] vfs: stream position permanently advanced on copy_to_user failure
- [Low] vfio: missing memory cleanup in vfio_ap_release_mig_file contradicts documented design
--

commit e098ee9f1268fb534553f678f8c768d5b444ac95
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: File ops called to save the vfio device migration state

This commit implements the read callback function for the file created to
save the state of the vfio-ap device when transitioning to the STOP_COPY
state. It copies the guest's AP configuration information to userspace.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index bbd007c06a776..0c43f25e2d9ba 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
[ ... ]
> @@ -150,6 +144,276 @@ static int vfio_ap_release_mig_file(struct inode *file_inode, struct file *filp)
[ ... ]
> +	/*
> +	 * Lazy initialization: the migration config is generated on the first
> +	 * read() call. vfio_ap_open_file_stream() initializes ap_config to NULL
> +	 * and config_sz to 0. On first read, we generate the config by
> +	 * snapshotting the guest's APQNs from shadow_apcb and retrieving hardware
> +	 * info via TAPQ for each queue. The result is cached in mig_file->ap_config
> +	 * for subsequent read() calls. The config is freed when the migration FD
> +	 * is released (vfio_ap_release_mig_file -> vfio_ap_release_stop_copy_file).
> +	 */

[Severity: Low]
The documentation here states the configuration is freed when the migration
file descriptor is released by calling vfio_ap_release_stop_copy_file() from
vfio_ap_release_mig_file().

However, looking at the existing vfio_ap_release_mig_file(), it doesn't
appear to call vfio_ap_release_stop_copy_file() during cleanup:

static int vfio_ap_release_mig_file(struct inode *file_inode, struct file *filp)
{
	struct ap_matrix_mdev *matrix_mdev = filp->private_data;

	/* ... */
	vfio_device_put_registration(&matrix_mdev->vdev);
	return 0;
}

Could this lead to delayed resource reclamation and a dangling pointer since
the ap_config buffer isn't actually freed when the file descriptor is closed?

[ ... ]
> +	read_pos = *pos;
> +	*pos += len;
> +
> +	/*
> +	 * Keep mdevs_lock held across copy_to_user() to prevent a concurrent
> +	 * vfio_ap_reset_migration_state() from freeing ap_config while we are
> +	 * reading it.  copy_to_user() may fault on a non-resident user page,
> +	 * but that is legal for a sleeping mutex.  The data transferred is at
> +	 * most a few KB for any realistic AP configuration, so holding the lock
> +	 * here is acceptable.
> +	 */
> +	if (copy_to_user(buf, (char *)mig_file->ap_config + read_pos, len)) {
> +		mutex_unlock(&matrix_dev->mdevs_lock);
> +		return -EFAULT;
> +	}
> +
> +	mutex_unlock(&matrix_dev->mdevs_lock);
> +
> +	return len;
> +}

[Severity: Medium]
If copy_to_user() fails and returns -EFAULT, pos has already been permanently
advanced by len.

Will a subsequent read() on this stream start from the advanced offset,
skipping over those bytes and breaking the migration data stream?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009114244.1213173-1-akrowiak@linux.ibm.com?part=7