From: WANG Xuerui <[email protected]>
Hi,
While trying to add loongarch to the Rust kvm-bindings crate, I
accidentally discovered faulty logic in the handling of CPUCFG IDs
("leaves" for those more familiar with x86), that could result in
incorrectly accepting every possible int for the ID; fortunately it is
6.8 material that hasn't seen a release yet, so a fix is possible.
The first patch contains the fix, while the rest are general
drive-by refactoring and comment cleanups.
Although it is currently the Chinese holiday season, Huacai told me
over IM that he's able to test the series and handle the upstreaming, so
going through the loongarch tree seems to be the way forward for the
series.
v3 changes:
- Fixed the validation by accepting every CPUCFG IDs from 0 to 20
inclusive, instead of only 2; this was a misunderstanding of mine
regarding the userland. (currently the only known user, the QEMU
target/loongarch KVM code, expects to be able to set all these 21
CPUCFG leaves, even though 7~15 are undefined according to the
LoongArch reference manual.) This also had the effect of squashing the
first 2 patches.
- Made the _kvm_get_cpucfg_mask return a mask in all valid cases,
allowing the mask check to be lifted out of the CPUCFG2 case.
- Swapped the "LoongArch:" and "KVM:" tags because right now the patches
are likely to reach mainline through the loongarch tree, and having
the "LoongArch:" prefix first is the convention here.
v2 changes:
- Squashed the v1 patches 4 and 5 according to Huacai's review
- Reworded comments according to Huacai's suggestion
- Use WARN_ON_ONCE (instead of BUG) to replace unreachable() for not
crashing the kernel (per checkpatch.pl suggestion)
WANG Xuerui (3):
LoongArch: KVM: Fix input validation of _kvm_get_cpucfg and
kvm_check_cpucfg
LoongArch: KVM: Rename _kvm_get_cpucfg to _kvm_get_cpucfg_mask
LoongArch: KVM: Streamline kvm_check_cpucfg and improve comments
arch/loongarch/kvm/vcpu.c | 81 ++++++++++++++++++---------------------
1 file changed, 38 insertions(+), 43 deletions(-)
--
2.43.0
From: WANG Xuerui <[email protected]>
The function is not actually a getter of guest CPUCFG, but rather
validation of the input CPUCFG ID plus information about the supported
bit flags of that CPUCFG leaf. So rename it to avoid confusion.
Signed-off-by: WANG Xuerui <[email protected]>
---
arch/loongarch/kvm/vcpu.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
index 56da0881fc94..d86da3811bea 100644
--- a/arch/loongarch/kvm/vcpu.c
+++ b/arch/loongarch/kvm/vcpu.c
@@ -298,7 +298,7 @@ static int _kvm_setcsr(struct kvm_vcpu *vcpu, unsigned int id, u64 val)
return ret;
}
-static int _kvm_get_cpucfg(int id, u64 *v)
+static int _kvm_get_cpucfg_mask(int id, u64 *v)
{
switch (id) {
case 2:
@@ -335,7 +335,7 @@ static int kvm_check_cpucfg(int id, u64 val)
u64 mask = 0;
int ret;
- ret = _kvm_get_cpucfg(id, &mask);
+ ret = _kvm_get_cpucfg_mask(id, &mask);
if (ret)
return ret;
@@ -563,7 +563,7 @@ static int kvm_loongarch_get_cpucfg_attr(struct kvm_vcpu *vcpu,
uint64_t val;
uint64_t __user *uaddr = (uint64_t __user *)attr->addr;
- ret = _kvm_get_cpucfg(attr->attr, &val);
+ ret = _kvm_get_cpucfg_mask(attr->attr, &val);
if (ret)
return ret;
--
2.43.0
From: WANG Xuerui <[email protected]>
All the checks currently done in kvm_check_cpucfg can be realized with
early returns, so just do that to avoid extra cognitive burden related
to the return value handling.
While at it, clean up comments of _kvm_get_cpucfg_mask and
kvm_check_cpucfg, by removing comments that are merely restatement of
the code nearby, and paraphrasing the rest so they read more natural
for English speakers (that likely are not familiar with the actual
Chinese-influenced grammar).
No functional changes intended.
Signed-off-by: WANG Xuerui <[email protected]>
---
arch/loongarch/kvm/vcpu.c | 44 +++++++++++++++++++--------------------
1 file changed, 21 insertions(+), 23 deletions(-)
diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
index d86da3811bea..a241db6d77a6 100644
--- a/arch/loongarch/kvm/vcpu.c
+++ b/arch/loongarch/kvm/vcpu.c
@@ -302,20 +302,16 @@ static int _kvm_get_cpucfg_mask(int id, u64 *v)
{
switch (id) {
case 2:
- /* Return CPUCFG2 features which have been supported by KVM */
+ /* CPUCFG2 features unconditionally supported by KVM */
*v = CPUCFG2_FP | CPUCFG2_FPSP | CPUCFG2_FPDP |
CPUCFG2_FPVERS | CPUCFG2_LLFTP | CPUCFG2_LLFTPREV |
CPUCFG2_LAM;
/*
- * If LSX is supported by CPU, it is also supported by KVM,
- * as we implement it.
+ * For the ISA extensions listed below, if one is supported
+ * by the host, then it is also supported by KVM.
*/
if (cpu_has_lsx)
*v |= CPUCFG2_LSX;
- /*
- * if LASX is supported by CPU, it is also supported by KVM,
- * as we implement it.
- */
if (cpu_has_lasx)
*v |= CPUCFG2_LASX;
@@ -345,24 +341,26 @@ static int kvm_check_cpucfg(int id, u64 val)
switch (id) {
case 2:
- /* CPUCFG2 features checking */
- else if (!(val & CPUCFG2_LLFTP))
- /* The LLFTP must be set, as guest must has a constant timer */
- ret = -EINVAL;
- else if ((val & CPUCFG2_FP) && (!(val & CPUCFG2_FPSP) || !(val & CPUCFG2_FPDP)))
- /* Single and double float point must both be set when enable FP */
- ret = -EINVAL;
- else if ((val & CPUCFG2_LSX) && !(val & CPUCFG2_FP))
- /* FP should be set when enable LSX */
- ret = -EINVAL;
- else if ((val & CPUCFG2_LASX) && !(val & CPUCFG2_LSX))
- /* LSX, FP should be set when enable LASX, and FP has been checked before. */
- ret = -EINVAL;
- break;
+ if (!(val & CPUCFG2_LLFTP))
+ /* Guests must have a constant timer */
+ return -EINVAL;
+ if ((val & CPUCFG2_FP) && (!(val & CPUCFG2_FPSP) || !(val & CPUCFG2_FPDP)))
+ /* Single and double float point must both be set when FP is enabled */
+ return -EINVAL;
+ if ((val & CPUCFG2_LSX) && !(val & CPUCFG2_FP))
+ /* LSX architecturally implies FP but val does not satisfy that */
+ return -EINVAL;
+ if ((val & CPUCFG2_LASX) && !(val & CPUCFG2_LSX))
+ /* LASX architecturally implies LSX and FP but val does not satisfy that */
+ return -EINVAL;
+ return 0;
default:
- break;
+ /*
+ * Values for the other CPUCFG IDs are not being further validated
+ * besides the mask check above.
+ */
+ return 0;
}
- return ret;
}
static int kvm_get_one_reg(struct kvm_vcpu *vcpu,
--
2.43.0
From: WANG Xuerui <[email protected]>
The range check for the CPUCFG ID is wrong (should have been a ||
instead of &&); it is conceptually simpler to just express the check
as another case of the switch statement on the ID. As it turns out to be
the case, the userland (currently only the QEMU/KVM target code) expects
to set CPUCFG IDs 0 to 20 inclusive, but only CPUCFG2 values are being
validated.
Furthermore, the juggling of the temp return value is unnecessary,
because it is semantically equivalent and more readable to just
return at every switch case's end. This is done too to avoid potential
bugs in the future related to the unwanted complexity.
Also, the return value of _kvm_get_cpucfg is meant to be checked, but
this was not done, so bad CPUCFG IDs wrongly fall back to the default
case and 0 is incorrectly returned; check the return value to fix the
UAPI behavior.
While at it, also remove the redundant range check in kvm_check_cpucfg,
because out-of-range CPUCFG IDs are already rejected by the -EINVAL
as returned by _kvm_get_cpucfg.
Fixes: db1ecca22edf ("LoongArch: KVM: Add LSX (128bit SIMD) support")
Signed-off-by: WANG Xuerui <[email protected]>
---
arch/loongarch/kvm/vcpu.c | 33 +++++++++++++++------------------
1 file changed, 15 insertions(+), 18 deletions(-)
diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
index 27701991886d..56da0881fc94 100644
--- a/arch/loongarch/kvm/vcpu.c
+++ b/arch/loongarch/kvm/vcpu.c
@@ -300,11 +300,6 @@ static int _kvm_setcsr(struct kvm_vcpu *vcpu, unsigned int id, u64 val)
static int _kvm_get_cpucfg(int id, u64 *v)
{
- int ret = 0;
-
- if (id < 0 && id >= KVM_MAX_CPUCFG_REGS)
- return -EINVAL;
-
switch (id) {
case 2:
/* Return CPUCFG2 features which have been supported by KVM */
@@ -324,31 +319,33 @@ static int _kvm_get_cpucfg(int id, u64 *v)
if (cpu_has_lasx)
*v |= CPUCFG2_LASX;
- break;
+ return 0;
+ case 0 ... 1:
+ case 3 ... KVM_MAX_CPUCFG_REGS - 1:
+ /* no restrictions on other CPUCFG IDs' values */
+ *v = U64_MAX;
+ return 0;
default:
- ret = -EINVAL;
- break;
+ return -EINVAL;
}
- return ret;
}
static int kvm_check_cpucfg(int id, u64 val)
{
- u64 mask;
- int ret = 0;
-
- if (id < 0 && id >= KVM_MAX_CPUCFG_REGS)
- return -EINVAL;
+ u64 mask = 0;
+ int ret;
- if (_kvm_get_cpucfg(id, &mask))
+ ret = _kvm_get_cpucfg(id, &mask);
+ if (ret)
return ret;
+ if (val & ~mask)
+ /* Unsupported features should not be set */
+ return -EINVAL;
+
switch (id) {
case 2:
/* CPUCFG2 features checking */
- if (val & ~mask)
- /* The unsupported features should not be set */
- ret = -EINVAL;
else if (!(val & CPUCFG2_LLFTP))
/* The LLFTP must be set, as guest must has a constant timer */
ret = -EINVAL;
--
2.43.0
Hi Xuerui,
Good catch, and thank for your patch.
On 2024/2/16 下午4:58, WANG Xuerui wrote:
> From: WANG Xuerui <[email protected]>
>
> The range check for the CPUCFG ID is wrong (should have been a ||
> instead of &&); it is conceptually simpler to just express the check
> as another case of the switch statement on the ID. As it turns out to be
> the case, the userland (currently only the QEMU/KVM target code) expects
> to set CPUCFG IDs 0 to 20 inclusive, but only CPUCFG2 values are being
> validated.
>
> Furthermore, the juggling of the temp return value is unnecessary,
> because it is semantically equivalent and more readable to just
> return at every switch case's end. This is done too to avoid potential
> bugs in the future related to the unwanted complexity.
>
> Also, the return value of _kvm_get_cpucfg is meant to be checked, but
> this was not done, so bad CPUCFG IDs wrongly fall back to the default
> case and 0 is incorrectly returned; check the return value to fix the
> UAPI behavior.
>
> While at it, also remove the redundant range check in kvm_check_cpucfg,
> because out-of-range CPUCFG IDs are already rejected by the -EINVAL
> as returned by _kvm_get_cpucfg.
>
> Fixes: db1ecca22edf ("LoongArch: KVM: Add LSX (128bit SIMD) support")
> Signed-off-by: WANG Xuerui <[email protected]>
> ---
> arch/loongarch/kvm/vcpu.c | 33 +++++++++++++++------------------
> 1 file changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index 27701991886d..56da0881fc94 100644
> --- a/arch/loongarch/kvm/vcpu.c
> +++ b/arch/loongarch/kvm/vcpu.c
> @@ -300,11 +300,6 @@ static int _kvm_setcsr(struct kvm_vcpu *vcpu, unsigned int id, u64 val)
>
> static int _kvm_get_cpucfg(int id, u64 *v)
> {
> - int ret = 0;
> -
> - if (id < 0 && id >= KVM_MAX_CPUCFG_REGS)
> - return -EINVAL;
> -
yes, it had better be removed since the same thing is done in function
kvm_check_cpucfg().
> switch (id) {
> case 2:
> /* Return CPUCFG2 features which have been supported by KVM */
> @@ -324,31 +319,33 @@ static int _kvm_get_cpucfg(int id, u64 *v)
> if (cpu_has_lasx)
> *v |= CPUCFG2_LASX;
>
> - break;
> + return 0;
> + case 0 ... 1:
> + case 3 ... KVM_MAX_CPUCFG_REGS - 1:
> + /* no restrictions on other CPUCFG IDs' values */
> + *v = U64_MAX;
> + return 0;
how about something like this?
default:
/* no restrictions on other CPUCFG IDs' values */
*v = U64_MAX;
return 0;
Regards
Bibo Mao
> default:
> - ret = -EINVAL;
> - break;
> + return -EINVAL;
> }
> - return ret;
> }
>
> static int kvm_check_cpucfg(int id, u64 val)
> {
> - u64 mask;
> - int ret = 0;
> -
> - if (id < 0 && id >= KVM_MAX_CPUCFG_REGS)
> - return -EINVAL;
> + u64 mask = 0;
> + int ret;
>
> - if (_kvm_get_cpucfg(id, &mask))
> + ret = _kvm_get_cpucfg(id, &mask);
> + if (ret)
> return ret;
>
> + if (val & ~mask)
> + /* Unsupported features should not be set */
> + return -EINVAL;
> +
> switch (id) {
> case 2:
> /* CPUCFG2 features checking */
> - if (val & ~mask)
> - /* The unsupported features should not be set */
> - ret = -EINVAL;
> else if (!(val & CPUCFG2_LLFTP))
> /* The LLFTP must be set, as guest must has a constant timer */
> ret = -EINVAL;
>
Hi,
On 2/17/24 11:03, maobibo wrote:
> Hi Xuerui,
>
> Good catch, and thank for your patch.
>
> On 2024/2/16 下午4:58, WANG Xuerui wrote:
>> [snip]
>> @@ -324,31 +319,33 @@ static int _kvm_get_cpucfg(int id, u64 *v)
>> if (cpu_has_lasx)
>> *v |= CPUCFG2_LASX;
>> - break;
>> + return 0;
>> + case 0 ... 1:
>> + case 3 ... KVM_MAX_CPUCFG_REGS - 1:
>> + /* no restrictions on other CPUCFG IDs' values */
>> + *v = U64_MAX;
>> + return 0;
> how about something like this?
> default:
> /* no restrictions on other CPUCFG IDs' values */
> *v = U64_MAX;
> return 0;
I don't think this version correctly expresses the intent. Note that the
CPUCFG ID range check is squashed into the switch as well, so one switch
conveniently expresses the three intended cases at once:
* the special treatment of CPUCFG2,
* all-allow rules for other in-range CPUCFG IDs, and
* rejection for out-of-range IDs.
Yet the suggestion here is conflating the latter two cases, with the
effect of allowing every ID that's not 2 to take any value (as expressed
by the U64_MAX mask), and *removing the range check* (because no return
path returns -EINVAL with this change).
So I'd like to stick to the current version, but thanks anyway for your
kind review and suggestion.
--
WANG "xen0n" Xuerui
Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/
On 2024/2/22 下午5:45, WANG Xuerui wrote:
> Hi,
>
> On 2/17/24 11:03, maobibo wrote:
>> Hi Xuerui,
>>
>> Good catch, and thank for your patch.
>>
>> On 2024/2/16 下午4:58, WANG Xuerui wrote:
>>> [snip]
>>> @@ -324,31 +319,33 @@ static int _kvm_get_cpucfg(int id, u64 *v)
>>> if (cpu_has_lasx)
>>> *v |= CPUCFG2_LASX;
>>> - break;
>>> + return 0;
>>> + case 0 ... 1:
>>> + case 3 ... KVM_MAX_CPUCFG_REGS - 1:
>>> + /* no restrictions on other CPUCFG IDs' values */
>>> + *v = U64_MAX;
>>> + return 0;
>> how about something like this?
>> default:
>> /* no restrictions on other CPUCFG IDs' values */
>> *v = U64_MAX;
>> return 0;
>
> I don't think this version correctly expresses the intent. Note that the
> CPUCFG ID range check is squashed into the switch as well, so one switch
> conveniently expresses the three intended cases at once:
>
> * the special treatment of CPUCFG2,
+ case 0 ... 1:
+ case 3 ... KVM_MAX_CPUCFG_REGS - 1:
+ /* no restrictions on other CPUCFG IDs' values */
+ *v = U64_MAX;
+ return 0;
cpucfg6 checking will be added for PMU support soon. So it will be
case 6:
do something check for cpucfg6
return mask;
case 0 ... 1:
case 3 ... 5:
case 7 ... KVM_MAX_CPUCFG_REGS - 1:
*v = U64_MAX;
return 0;
If you think it is reasonable to add these separate "case" sentences, I
have no objection.
> * all-allow rules for other in-range CPUCFG IDs, and
> * rejection for out-of-range IDs.
static int kvm_check_cpucfg(int id, u64 val)
{
- u64 mask;
- int ret = 0;
-
- if (id < 0 && id >= KVM_MAX_CPUCFG_REGS)
- return -EINVAL;
you can modify && with ||, like this:
if (id < 0 || id >= KVM_MAX_CPUCFG_REGS)
return -EINVAL;
+ u64 mask = 0;
+ int ret;
Regards
Bibo Mao
>
> Yet the suggestion here is conflating the latter two cases, with the
> effect of allowing every ID that's not 2 to take any value (as expressed
> by the U64_MAX mask), and *removing the range check* (because no return
> path returns -EINVAL with this change).
>
> So I'd like to stick to the current version, but thanks anyway for your
> kind review and suggestion.
>
On 2/22/24 18:22, maobibo wrote:
>
>
> On 2024/2/22 下午5:45, WANG Xuerui wrote:
>> Hi,
>>
>> On 2/17/24 11:03, maobibo wrote:
>>> Hi Xuerui,
>>>
>>> Good catch, and thank for your patch.
>>>
>>> On 2024/2/16 下午4:58, WANG Xuerui wrote:
>>>> [snip]
>>>> @@ -324,31 +319,33 @@ static int _kvm_get_cpucfg(int id, u64 *v)
>>>> if (cpu_has_lasx)
>>>> *v |= CPUCFG2_LASX;
>>>> - break;
>>>> + return 0;
>>>> + case 0 ... 1:
>>>> + case 3 ... KVM_MAX_CPUCFG_REGS - 1:
>>>> + /* no restrictions on other CPUCFG IDs' values */
>>>> + *v = U64_MAX;
>>>> + return 0;
>>> how about something like this?
>>> default:
>>> /* no restrictions on other CPUCFG IDs' values */
>>> *v = U64_MAX;
>>> return 0;
>>
>> I don't think this version correctly expresses the intent. Note that
>> the CPUCFG ID range check is squashed into the switch as well, so one
>> switch conveniently expresses the three intended cases at once:
>>
>> * the special treatment of CPUCFG2,
> + case 0 ... 1:
> + case 3 ... KVM_MAX_CPUCFG_REGS - 1:
> + /* no restrictions on other CPUCFG IDs' values */
> + *v = U64_MAX;
> + return 0;
> cpucfg6 checking will be added for PMU support soon. So it will be
> case 6:
> do something check for cpucfg6
> return mask;
> case 0 ... 1:
> case 3 ... 5:
> case 7 ... KVM_MAX_CPUCFG_REGS - 1:
> *v = U64_MAX;
> return 0;
>
> If you think it is reasonable to add these separate "case" sentences, I
> have no objection.
>> * all-allow rules for other in-range CPUCFG IDs, and
>> * rejection for out-of-range IDs.
> static int kvm_check_cpucfg(int id, u64 val)
> {
> - u64 mask;
> - int ret = 0;
> -
> - if (id < 0 && id >= KVM_MAX_CPUCFG_REGS)
> - return -EINVAL;
> you can modify && with ||, like this:
> if (id < 0 || id >= KVM_MAX_CPUCFG_REGS)
> return -EINVAL;
Yes I know. Personally I don't find the "three cases" that annoying, but
I agree with you that it can be mildly frustrating if one case addition
would split the ranges even more.
Given I'd like to also change the U64_MAX into U32_MAX (CPUCFG data is
specified to be 32-bit wide), I'll send a v4 that keeps the "if" branch
to make more people happy (Huacai privately also expressed preference
for minimizing changes to the overall code shape). Thanks for your
suggestion.
--
WANG "xen0n" Xuerui
Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/