[PATCH] KVM: arm64: vgic-its: clear dte when mapd unmaps a device

[PATCH] KVM: arm64: vgic-its: clear dte when mapd unmaps a device

[Kunkun Jiang]

vgic_its_save_device_tables will traverse its->device_list to
save dte for each device. vgic_its_restore_device_tables will
traverse each entry of device table and check if it is valid.
Restore if valid.

But when mapd unmaps a devices, we do not invalidate the
corresponding dte. In the scenario of continuous save
and restore, there may be a situation where a device's dte
is not saved but is restored. This is unreasonable and may
cause restore to fail. This patch clears the corresponding
dte when mapd unmaps a device.

Singed-off-by: Shusen Li <lishusen2@huawei.com>
Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
---
 arch/arm64/kvm/vgic/vgic-its.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 40bb43f20bf3..dd5fba1e8ba3 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -1140,8 +1140,12 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
 	u8 num_eventid_bits = its_cmd_get_size(its_cmd);
 	gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd);
 	struct its_device *device;
+	gpa_t gpa;
+	const struct vgic_its_abi *abi;
+	int dte_esz;
+	u64 val;
 
-	if (!vgic_its_check_id(its, its->baser_device_table, device_id, NULL))
+	if (!vgic_its_check_id(its, its->baser_device_table, device_id, &gpa))
 		return E_ITS_MAPD_DEVICE_OOR;
 
 	if (valid && num_eventid_bits > VITS_TYPER_IDBITS)
@@ -1161,8 +1165,13 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
 	 * The spec does not say whether unmapping a not-mapped device
 	 * is an error, so we are done in any case.
 	 */
-	if (!valid)
+	if (!valid) {
+		abi = vgic_its_get_abi(its);
+		dte_esz = abi->dte_esz;
+		val = cpu_to_le64(0);
+		vgic_write_guest_lock(kvm, gpa, &val, dte_esz);
 		return 0;
+	}
 
 	device = vgic_its_alloc_device(its, device_id, itt_addr,
 				       num_eventid_bits);
-- 
2.33.0

Re: [PATCH] KVM: arm64: vgic-its: clear dte when mapd unmaps a device

[Marc Zyngier]

On Thu, 13 Jun 2024 10:38:11 +0100,
Kunkun Jiang <jiangkunkun@huawei.com> wrote:
> 
> vgic_its_save_device_tables will traverse its->device_list to
> save dte for each device. vgic_its_restore_device_tables will
> traverse each entry of device table and check if it is valid.
> Restore if valid.
> 
> But when mapd unmaps a devices, we do not invalidate the
> corresponding dte. In the scenario of continuous save
> and restore, there may be a situation where a device's dte
> is not saved but is restored. This is unreasonable and may
> cause restore to fail. This patch clears the corresponding
> dte when mapd unmaps a device.
> 
> Singed-off-by: Shusen Li <lishusen2@huawei.com>

  ^^^^^^^^^^^^^ This isn't a valid tag!

> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>

Something is wrong. Either Shusen Li is the author and you are merely
posting it, in which case there should be a 'From' line in the commit
message, or you are co-authors and there should be a 'Co-developed-by'
right after Shusen Li's SoB.

Please read Documentation/process/submitting-patches.rst for the
details.

> ---
>  arch/arm64/kvm/vgic/vgic-its.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 40bb43f20bf3..dd5fba1e8ba3 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -1140,8 +1140,12 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
>  	u8 num_eventid_bits = its_cmd_get_size(its_cmd);
>  	gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd);
>  	struct its_device *device;
> +	gpa_t gpa;
> +	const struct vgic_its_abi *abi;
> +	int dte_esz;
> +	u64 val;

Aside from 'gpa', all the extra variables should be moved into the
!valid block. And it would be nicer if this variable was called
something more descriptive.

>  
> -	if (!vgic_its_check_id(its, its->baser_device_table, device_id, NULL))
> +	if (!vgic_its_check_id(its, its->baser_device_table, device_id, &gpa))
>  		return E_ITS_MAPD_DEVICE_OOR;
>  
>  	if (valid && num_eventid_bits > VITS_TYPER_IDBITS)
> @@ -1161,8 +1165,13 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
>  	 * The spec does not say whether unmapping a not-mapped device
>  	 * is an error, so we are done in any case.
>  	 */
> -	if (!valid)
> +	if (!valid) {
> +		abi = vgic_its_get_abi(its);
> +		dte_esz = abi->dte_esz;
> +		val = cpu_to_le64(0);
> +		vgic_write_guest_lock(kvm, gpa, &val, dte_esz);

This is just a bit wrong:

- you store 0 in a u64 after having (pointlessly) converted it to le64

- you write 'dte_esz' bytes into guest memory, but you don't have any
  check whether this is *larger* than the u64 you have passed

Similar issue exists in all the vgic_its_save_*() functions, calling
for a generic fix, as we're just lucky to never have needed a new ABI
so far. At the very least a check that we deal with data that is
exactly 8 bytes.

Also, is the device table the only one that is subject to this
'reload' problem? How about the Collection table?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] KVM: arm64: vgic-its: clear dte when mapd unmaps a device

[Eric Auger]

Hi,

On 6/13/24 16:44, Marc Zyngier wrote:
> On Thu, 13 Jun 2024 10:38:11 +0100,
> Kunkun Jiang <jiangkunkun@huawei.com> wrote:
>>
>> vgic_its_save_device_tables will traverse its->device_list to
>> save dte for each device. vgic_its_restore_device_tables will
>> traverse each entry of device table and check if it is valid.
>> Restore if valid.
>>
>> But when mapd unmaps a devices, we do not invalidate the
>> corresponding dte. In the scenario of continuous save
>> and restore, there may be a situation where a device's dte
>> is not saved but is restored. This is unreasonable and may
>> cause restore to fail. This patch clears the corresponding
>> dte when mapd unmaps a device.
>>
>> Singed-off-by: Shusen Li <lishusen2@huawei.com>
> 
>   ^^^^^^^^^^^^^ This isn't a valid tag!
> 
>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> 
> Something is wrong. Either Shusen Li is the author and you are merely
> posting it, in which case there should be a 'From' line in the commit
> message, or you are co-authors and there should be a 'Co-developed-by'
> right after Shusen Li's SoB.
> 
> Please read Documentation/process/submitting-patches.rst for the
> details.
> 
>> ---
>>  arch/arm64/kvm/vgic/vgic-its.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
>> index 40bb43f20bf3..dd5fba1e8ba3 100644
>> --- a/arch/arm64/kvm/vgic/vgic-its.c
>> +++ b/arch/arm64/kvm/vgic/vgic-its.c
>> @@ -1140,8 +1140,12 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
>>  	u8 num_eventid_bits = its_cmd_get_size(its_cmd);
>>  	gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd);
>>  	struct its_device *device;
>> +	gpa_t gpa;
>> +	const struct vgic_its_abi *abi;
>> +	int dte_esz;
>> +	u64 val;
> 
> Aside from 'gpa', all the extra variables should be moved into the
> !valid block. And it would be nicer if this variable was called
> something more descriptive.
> 
>>  
>> -	if (!vgic_its_check_id(its, its->baser_device_table, device_id, NULL))
>> +	if (!vgic_its_check_id(its, its->baser_device_table, device_id, &gpa))
>>  		return E_ITS_MAPD_DEVICE_OOR;
>>  
>>  	if (valid && num_eventid_bits > VITS_TYPER_IDBITS)
>> @@ -1161,8 +1165,13 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
>>  	 * The spec does not say whether unmapping a not-mapped device
>>  	 * is an error, so we are done in any case.
>>  	 */
>> -	if (!valid)
>> +	if (!valid) {
>> +		abi = vgic_its_get_abi(its);
>> +		dte_esz = abi->dte_esz;
>> +		val = cpu_to_le64(0);
>> +		vgic_write_guest_lock(kvm, gpa, &val, dte_esz);
> 
> This is just a bit wrong:
> 
> - you store 0 in a u64 after having (pointlessly) converted it to le64
> 
> - you write 'dte_esz' bytes into guest memory, but you don't have any
>   check whether this is *larger* than the u64 you have passed
> 
> Similar issue exists in all the vgic_its_save_*() functions, calling
> for a generic fix, as we're just lucky to never have needed a new ABI
> so far. At the very least a check that we deal with data that is
> exactly 8 bytes.
> 
> Also, is the device table the only one that is subject to this
> 'reload' problem? How about the Collection table?
The collection table is not indexed by the collection ID and when saving
we end up writing an invalid dummy entry to close the recording. So I
don't think we have the same problem there.

For ITT I think we have the same issue on restore because the restore
scans the table until it gets a first valid entry and then chain on next
item. If we have stale entries this looks like the same issue.

Thanks

Eric
> 
> Thanks,
> 
> 	M.
> 

Re: [PATCH] KVM: arm64: vgic-its: clear dte when mapd unmaps a device

[Kunkun Jiang]

Hi Marc，

Thank you very much for reviewing.

On 2024/6/13 22:44, Marc Zyngier wrote:
> On Thu, 13 Jun 2024 10:38:11 +0100,
> Kunkun Jiang <jiangkunkun@huawei.com> wrote:
>> vgic_its_save_device_tables will traverse its->device_list to
>> save dte for each device. vgic_its_restore_device_tables will
>> traverse each entry of device table and check if it is valid.
>> Restore if valid.
>>
>> But when mapd unmaps a devices, we do not invalidate the
>> corresponding dte. In the scenario of continuous save
>> and restore, there may be a situation where a device's dte
>> is not saved but is restored. This is unreasonable and may
>> cause restore to fail. This patch clears the corresponding
>> dte when mapd unmaps a device.
>>
>> Singed-off-by: Shusen Li <lishusen2@huawei.com>
>    ^^^^^^^^^^^^^ This isn't a valid tag!
>
>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> Something is wrong. Either Shusen Li is the author and you are merely
> posting it, in which case there should be a 'From' line in the commit
> message, or you are co-authors and there should be a 'Co-developed-by'
> right after Shusen Li's SoB.
>
> Please read Documentation/process/submitting-patches.rst for the
> details.
The latest patch has been modified. See link below.
>> ---
>>   arch/arm64/kvm/vgic/vgic-its.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
>> index 40bb43f20bf3..dd5fba1e8ba3 100644
>> --- a/arch/arm64/kvm/vgic/vgic-its.c
>> +++ b/arch/arm64/kvm/vgic/vgic-its.c
>> @@ -1140,8 +1140,12 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
>>   	u8 num_eventid_bits = its_cmd_get_size(its_cmd);
>>   	gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd);
>>   	struct its_device *device;
>> +	gpa_t gpa;
>> +	const struct vgic_its_abi *abi;
>> +	int dte_esz;
>> +	u64 val;
> Aside from 'gpa', all the extra variables should be moved into the
> !valid block. And it would be nicer if this variable was called
> something more descriptive.
>
>>   
>> -	if (!vgic_its_check_id(its, its->baser_device_table, device_id, NULL))
>> +	if (!vgic_its_check_id(its, its->baser_device_table, device_id, &gpa))
>>   		return E_ITS_MAPD_DEVICE_OOR;
>>   
>>   	if (valid && num_eventid_bits > VITS_TYPER_IDBITS)
>> @@ -1161,8 +1165,13 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
>>   	 * The spec does not say whether unmapping a not-mapped device
>>   	 * is an error, so we are done in any case.
>>   	 */
>> -	if (!valid)
>> +	if (!valid) {
>> +		abi = vgic_its_get_abi(its);
>> +		dte_esz = abi->dte_esz;
>> +		val = cpu_to_le64(0);
>> +		vgic_write_guest_lock(kvm, gpa, &val, dte_esz);
> This is just a bit wrong:
>
> - you store 0 in a u64 after having (pointlessly) converted it to le64
>
> - you write 'dte_esz' bytes into guest memory, but you don't have any
>    check whether this is *larger* than the u64 you have passed
The latest patch has been modified. See link below.
> Similar issue exists in all the vgic_its_save_*() functions, calling
> for a generic fix, as we're just lucky to never have needed a new ABI
> so far. At the very least a check that we deal with data that is
> exactly 8 bytes.
A patch has been added to check it.
Link: 
https://lore.kernel.org/kvmarm/20240620130650.1279-1-jiangkunkun@huawei.com/

Thanks,
Kunkun Jiang

Re: [PATCH] KVM: arm64: vgic-its: clear dte when mapd unmaps a device

[Kunkun Jiang]

Hi Eric,

On 2024/6/19 1:18, Eric Auger wrote:
>> Also, is the device table the only one that is subject to this
>> 'reload' problem? How about the Collection table?
> The collection table is not indexed by the collection ID and when saving
> we end up writing an invalid dummy entry to close the recording. So I
> don't think we have the same problem there.
I'm a little confused here. In the current Linux, there is no implementation
of mapc unmaps a cte. But should this scenario be considered in vgic-its?
> For ITT I think we have the same issue on restore because the restore
> scans the table until it gets a first valid entry and then chain on next
> item. If we have stale entries this looks like the same issue.
Thanks for your reply. I added a patch to fix it.
https://lore.kernel.org/kvmarm/20240620130650.1279-4-jiangkunkun@huawei.com/

Thanks,
Kunkun Jiang

Re: [PATCH] KVM: arm64: vgic-its: clear dte when mapd unmaps a device

[Marc Zyngier]

On Thu, 20 Jun 2024 14:32:20 +0100,
Kunkun Jiang <jiangkunkun@huawei.com> wrote:
> 
> Hi Eric,
> 
> On 2024/6/19 1:18, Eric Auger wrote:
> >> Also, is the device table the only one that is subject to this
> >> 'reload' problem? How about the Collection table?
> > The collection table is not indexed by the collection ID and when saving
> > we end up writing an invalid dummy entry to close the recording. So I
> > don't think we have the same problem there.
> I'm a little confused here. In the current Linux, there is no implementation
> of mapc unmaps a cte. But should this scenario be considered in
> vgic-its?

We implement the "HW", not what Linux does.

	M.

-- 
Without deviation from the norm, progress is not possible.