Re: [PATCH v3 14/15] s390/vfio-ap: Add 'migratable' feature to sysfs 'features' attribute

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Dangling pointer in migration data after `krealloc`.
- [High] Unlocking an arbitrary thread's mutex using `mutex_is_locked()`.
- [High] Failure to persist `ap_config` pointer during chunked resuming writes causes memory leaks and breaks subsequent writes.
- [High] Uninitialized `config_sz` sent over the wire breaks buffer reallocation on the destination host.
--

commit 0de28964ca6b520852841fe66380c0b0caacaec6
Author: Anthony Krowiak <akrowiak@linux.ibm.com>
Date:   Tue Jun 30 10:41:24 2026 +0000

    s390/vfio-ap: Add 'migratable' feature to sysfs 'features' attribute
    
    Add the 'migratable' feature to the features provided by the sysfs
    /sys/devices/vfio_ap/matrix/feature attribute to indicate that migration
    of vfio devices is supported.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> new file mode 100644
> index 0000000000000..83e1ce2c40c0c
> --- /dev/null
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> @@ -0,0 +1,1373 @@

[ ... ]

> +static int
> +vfio_ap_get_config(struct ap_matrix_mdev *matrix_mdev,
> +		   struct vfio_ap_config **ap_config, size_t *ap_config_size)
> +{
> +	struct vfio_ap_config *ap_configuration;
> +	int num_queues, ret;
> +
> +	*ap_config_size = vfio_ap_config_size(matrix_mdev, &num_queues);
> +
> +	ap_configuration = kzalloc(*ap_config_size, GFP_KERNEL_ACCOUNT);
> +	if (!ap_configuration)
> +		return -ENOMEM;
> +
> +	ap_configuration->num_queues = num_queues;

[Severity: High]
Does this code fail to initialize ap_configuration->config_sz before sending
it over the wire during vfio_ap_stop_copy_read()?

The config_sz field is left as 0, which is then sent to the destination host.

> +
> +	ret = vfio_ap_store_queue_info(matrix_mdev, ap_configuration);

[ ... ]

> +static ssize_t reallocate_ap_config(struct vfio_ap_config **mig_ap_config,
> +				    size_t len)
> +{
> +	struct vfio_ap_config *ap_config = *mig_ap_config;
> +	struct vfio_ap_config *new_ap_config;
> +	size_t new_cfg_sz, cur_cfg_sz;
> +	unsigned int num_queues;
> +
> +	cur_cfg_sz = ap_config->config_sz;
> +	num_queues = ap_config->num_queues;
> +
> +	/*
> +	 * If the current configuration size is greater than the
> +	 * size of a vfio_ap_config structure (i.e., contains the num_queues
> +	 * field), then there should already be enough storage allocated
> +	 * to store the source guest's AP configuration. Let's verify that the
> +	 * amount of storage allocated is what we expect based on the number of
> +	 * vfio_ap_queue_info objects that must be stored.
> +	 */
> +	if (cur_cfg_sz >= sizeof(struct vfio_ap_config)) {

[Severity: High]
Can this condition fail unexpectedly due to the uninitialized config_sz sent
from the source host?

During restoration, allocate_ap_config() receives the first chunk, storing 0
in config_sz. On the next chunk, reallocate_ap_config() reads cur_cfg_sz as 0.

The else branch is taken, where new_cfg_sz = cur_cfg_sz + len calculates len,
truncating the buffer to just the size of the incoming chunk, causing immediate
-EIO failures.

> +		new_cfg_sz = calculate_ap_config_size(num_queues);

[ ... ]

> +static ssize_t vfio_ap_resuming_write(struct file *filp, const char __user *buf,
> +				      size_t len, loff_t *pos)
> +{
> +	struct vfio_ap_migration_data *mig_data;
> +	struct ap_matrix_mdev *matrix_mdev;
> +	struct vfio_ap_config *ap_config;
> +	struct ap_matrix guest_matrix;
> +	ssize_t ret = 0, cfg_sz;
> +	const char *mdev_name;

[ ... ]

> +	/*
> +	 * If this is the first write operation, then allocate storage for the
> +	 * AP configuration information; otherwise, reallocate the
> +	 * struct vfio_ap_config object used to store the AP configuration data
> +	 * sent from userspace.
> +	 */
> +	if (*pos == 0) {
> +		ret = allocate_ap_config(&ap_config, buf, len);
> +
> +		/* If the allocation failed, we'll return the error */
> +		if (ret < 0)
> +			goto done;
> +
> +		cfg_sz = ret;

[Severity: High]
Is it possible for a chunked write to fail to persist the ap_config pointer?

If allocate_ap_config() succeeds but the write does not contain the complete
configuration (*pos + len != cfg_sz), ap_config is not assigned to
mig_data->resuming_mig_state.ap_config before the function returns.

The allocated ap_config memory leaks, and on the next chunk, *pos > 0 but the
state pointer remains NULL. This will cause validate_resuming_write_parms() to
falsely detect an illegal state and return -EFAULT.

> +	} else {
> +		ap_config = mig_data->resuming_mig_state.ap_config;
> +
> +		ret = reallocate_ap_config(&ap_config, len);

[Severity: Critical]
Does this code create a dangling pointer in the migration data after krealloc()?

The reallocate_ap_config() call uses krealloc(), which may free the old buffer
and return a new one. The new pointer is stored in the local ap_config variable,
but it isn't updated in mig_data->resuming_mig_state.ap_config unless the
entire configuration is received successfully.

If copy_from_user() fails below, the new buffer is freed via kfree(ap_config),
but mig_data retains the old, already-freed pointer. When the file is closed,
vfio_ap_release_resuming_file() calls kfree() on the old pointer again,
leading to a double-free.

> +		if (ret < 0)
> +			goto cleanup;
> +
> +		cfg_sz = ret;
> +	}

[ ... ]

> +cleanup:
> +	kfree(ap_config);
> +done:
> +	if (mutex_is_locked(&matrix_dev->mdevs_lock))
> +		mutex_unlock(&matrix_dev->mdevs_lock);

[Severity: High]
Does this error path illegally unlock matrix_dev->mdevs_lock if it is held by
another thread?

Because mutex_is_locked() just checks if the lock is held by any thread, and
vfio_ap_resuming_write() dropped mdevs_lock earlier to call copy_from_user(),
if a concurrent thread is currently holding mdevs_lock, this will unlock it.

This leads to race conditions and potential data corruption.

> +
> +	return ret;
> +}

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