Changes from V3:
* Rebased onto 5.17-rc1.
V3: https://lore.kernel.org/lkml/[email protected]/
---
The recent x86 dynamic state support incorporates the arch_prctl option to
request permission before using a dynamic state.
It was designed to add the requested feature in the group leader's
permission bitmask so that every thread can reference this master bitmask.
The group leader is assumed to be unchanged here. The mainline is the case
as a group leader is identified at fork() or exec() time only.
This master bitmask should include non-dynamic features always, as they
are permitted by default. Users may check them via ARCH_GET_XCOMP_PERM.
But, in hindsight, the implementation does overwrite the bitmask with the
requested bit only, instead of adding the bit to the existing one. This
overwrite effectively revokes the permission that is granted already.
Fix the code and also update the selftest to disclose the issue if there
is.
Chang S. Bae (1):
selftests/x86/amx: Update the ARCH_REQ_XCOMP_PERM test
Yang Zhong (1):
x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation
arch/x86/kernel/fpu/xstate.c | 2 +-
tools/testing/selftests/x86/amx.c | 16 ++++++++++++++--
2 files changed, 15 insertions(+), 3 deletions(-)
base-commit: e783362eb54cd99b2cac8b3a9aeac942e6f6ac07
--
2.17.1
Update the arch_prctl test to check the permission bitmap whether the
requested feature is added as expected or not.
Every non-dynamic feature that is enabled is permitted already for use.
TILECFG is not dynamic feature. Ensure the bit is always on from
ARCH_GET_XCOMP_PERM.
Signed-off-by: Chang S. Bae <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
tools/testing/selftests/x86/amx.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/x86/amx.c b/tools/testing/selftests/x86/amx.c
index 3615ef4a48bb..e1e2c8f3356f 100644
--- a/tools/testing/selftests/x86/amx.c
+++ b/tools/testing/selftests/x86/amx.c
@@ -368,9 +368,16 @@ static void req_xtiledata_perm(void)
static void validate_req_xcomp_perm(enum expected_result exp)
{
- unsigned long bitmask;
+ unsigned long bitmask, expected_bitmask;
long rc;
+ rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_PERM, &bitmask);
+ if (rc) {
+ fatal_error("prctl(ARCH_GET_XCOMP_PERM) error: %ld", rc);
+ } else if (!(bitmask & XFEATURE_MASK_XTILECFG)) {
+ fatal_error("ARCH_GET_XCOMP_PERM returns XFEATURE_XTILECFG off.");
+ }
+
rc = syscall(SYS_arch_prctl, ARCH_REQ_XCOMP_PERM, XFEATURE_XTILEDATA);
if (exp == FAIL_EXPECTED) {
if (rc) {
@@ -383,10 +390,15 @@ static void validate_req_xcomp_perm(enum expected_result exp)
fatal_error("ARCH_REQ_XCOMP_PERM saw unexpected failure.\n");
}
+ expected_bitmask = bitmask | XFEATURE_MASK_XTILEDATA;
+
rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_PERM, &bitmask);
if (rc) {
fatal_error("prctl(ARCH_GET_XCOMP_PERM) error: %ld", rc);
- } else if (bitmask & XFEATURE_MASK_XTILE) {
+ } else if (bitmask != expected_bitmask) {
+ fatal_error("ARCH_REQ_XCOMP_PERM saw a wrong bitmask: %lx, expected: %lx.\n",
+ bitmask, expected_bitmask);
+ } else {
printf("\tARCH_REQ_XCOMP_PERM is successful.\n");
}
}
--
2.17.1
From: Yang Zhong <[email protected]>
ARCH_REQ_XCOMP_PERM is supposed to add the requested feature to the
permission bitmap of thread_group_leader()->fpu. But the code overwrites
the bitmap with the requested feature bit only rather than adding it.
Fix the code to add the request feature bit to the master bitmask.
Fixes: db8268df0983 ("x86/arch_prctl: Add controls for dynamic XSTATE components")
Signed-off-by: Yang Zhong <[email protected]>
Signed-off-by: Chang S. Bae <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
Changes from v3:
* Adjust the change to the mainline (v5.17-rc).
Changes from v2:
* Fix the authorship.
Changes from v1:
* Change the mask value only and trim the changelog.
---
arch/x86/kernel/fpu/xstate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 02b3ddaf4f75..2d4363e32517 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1636,7 +1636,7 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest)
perm = guest ? &fpu->guest_perm : &fpu->perm;
/* Pairs with the READ_ONCE() in xstate_get_group_perm() */
- WRITE_ONCE(perm->__state_perm, requested);
+ WRITE_ONCE(perm->__state_perm, mask);
/* Protected by sighand lock */
perm->__state_size = ksize;
perm->__user_state_size = usize;
--
2.17.1
x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation
If WRITE_ONCE(perm->__state_perm, requested) is modified to
WRITE_ONCE(perm->__state_perm, mask), When the qemu process does not
request the XFEATURE_MASK_XTILE_DATA xsave state permission, there may
be a gp error (kvm: kvm_set_xcr line 1091 inject gp fault with cpl 0)
because __kvm_set_xcr return 1.
static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr){
...
// xcr0 includes XFEATURE_MASK_XTILE_CFG by default.
if ((xcr0 & XFEATURE_MASK_XTILE) &&
((xcr0 & XFEATURE_MASK_XTILE) != XFEATURE_MASK_XTILE))
return 1;
...
}
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 02b3dda..2d4363e 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1636,7 +1636,7 @@ static int __xstate_request_perm(u64 permitted,
u64 requested, bool guest)
perm = guest ? &fpu->guest_perm : &fpu->perm;
/* Pairs with the READ_ONCE() in xstate_get_group_perm() */
- WRITE_ONCE(perm->__state_perm, requested);
+ WRITE_ONCE(perm->__state_perm, mask);
/* Protected by sighand lock */
perm->__state_size = ksize;
perm->__user_state_size = usize;
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 494d4d3..e8704568 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -908,6 +908,9 @@ static inline int __do_cpuid_func(struct
kvm_cpuid_array *array, u32 function)
break;
case 0xd: {
u64 permitted_xcr0 = supported_xcr0 &
xstate_get_guest_group_perm();
+ permitted_xcr0 = ((permitted_xcr0 &
XFEATURES_MASK_XTILE) != XFEATURES_MASK_XTILE)
+ ? permitted_xcr0
+ : permitted_xcr0 & ~XFEATURES_MASK_XTILE;
u64 permitted_xss = supported_xss;
entry->eax &= permitted_xcr0;
On 3/7/2022 4:20 AM, Hao Xiang wrote:
> x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation
>
> If WRITE_ONCE(perm->__state_perm, requested) is modified to
> WRITE_ONCE(perm->__state_perm, mask), When the qemu process does not
> request the XFEATURE_MASK_XTILE_DATA xsave state permission, there may
> be a gp error (kvm: kvm_set_xcr line 1091 inject gp fault with cpl 0)
> because __kvm_set_xcr return 1.
What you said here does not make sense to me. When the Qemu process does
not request XTILEDATA, then the __xstate_request_perm() function is
never called in this, no?
>
> static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr){
> ...
> // xcr0 includes XFEATURE_MASK_XTILE_CFG by default.
> if ((xcr0 & XFEATURE_MASK_XTILE) &&
> ((xcr0 & XFEATURE_MASK_XTILE) != XFEATURE_MASK_XTILE))
> return 1;
> ...
> }
>
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 02b3dda..2d4363e 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1636,7 +1636,7 @@ static int __xstate_request_perm(u64 permitted,
> u64 requested, bool guest)
>
> perm = guest ? &fpu->guest_perm : &fpu->perm;
> /* Pairs with the READ_ONCE() in xstate_get_group_perm() */
> - WRITE_ONCE(perm->__state_perm, requested);
> + WRITE_ONCE(perm->__state_perm, mask);
> /* Protected by sighand lock */
> perm->__state_size = ksize;
> perm->__user_state_size = usize;
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 494d4d3..e8704568 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -908,6 +908,9 @@ static inline int __do_cpuid_func(struct
> kvm_cpuid_array *array, u32 function)
> break;
> case 0xd: {
> u64 permitted_xcr0 = supported_xcr0 &
> xstate_get_guest_group_perm();
Yang, I think you should have included your fix [1] in your series [2]
in the first place, before using it widely like [3].
> + permitted_xcr0 = ((permitted_xcr0 &
> XFEATURES_MASK_XTILE) != XFEATURES_MASK_XTILE)
> + ? permitted_xcr0
> + : permitted_xcr0 & ~XFEATURES_MASK_XTILE;
> u64 permitted_xss = supported_xss;
>
> entry->eax &= permitted_xcr0;
>
Well, first of all, one patch should fix one issue, not two or more, no?
But this hunk looks duplicate with this [4].
Thanks,
Chang
[1] https://lore.kernel.org/lkml/[email protected]/
[2]
https://lore.kernel.org/lkml/[email protected]/
[3]
https://lore.kernel.org/lkml/[email protected]/
[4]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kvm/x86.c#n1033
Using the linux upsteam code with your patches，the problem is not
reproduced. Maybe my patches are incomplete.
Thanks,
Hao
On 2022/3/8 2:53, Chang S. Bae wrote:
> On 3/7/2022 4:20 AM, Hao Xiang wrote:
>> x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation
>>
>> If WRITE_ONCE(perm->__state_perm, requested) is modified to
>> WRITE_ONCE(perm->__state_perm, mask), When the qemu process does not
>> request the XFEATURE_MASK_XTILE_DATA xsave state permission, there may
>> be a gp error (kvm: kvm_set_xcr line 1091 inject gp fault with cpl 0)
>> because __kvm_set_xcr return 1.
>
> What you said here does not make sense to me. When the Qemu process does
> not request XTILEDATA, then the __xstate_request_perm() function is
> never called in this, no?
>
>>
>> static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr){
>> ...
>> // xcr0 includes XFEATURE_MASK_XTILE_CFG by default.
>> if ((xcr0 & XFEATURE_MASK_XTILE) &&
>> ((xcr0 & XFEATURE_MASK_XTILE) != XFEATURE_MASK_XTILE))
>> return 1;
>> ...
>> }
>>
>> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
>> index 02b3dda..2d4363e 100644
>> --- a/arch/x86/kernel/fpu/xstate.c
>> +++ b/arch/x86/kernel/fpu/xstate.c
>> @@ -1636,7 +1636,7 @@ static int __xstate_request_perm(u64 permitted,
>> u64 requested, bool guest)
>>
>> perm = guest ? &fpu->guest_perm : &fpu->perm;
>> /* Pairs with the READ_ONCE() in xstate_get_group_perm() */
>> - WRITE_ONCE(perm->__state_perm, requested);
>> + WRITE_ONCE(perm->__state_perm, mask);
>> /* Protected by sighand lock */
>> perm->__state_size = ksize;
>> perm->__user_state_size = usize;
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 494d4d3..e8704568 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -908,6 +908,9 @@ static inline int __do_cpuid_func(struct
>> kvm_cpuid_array *array, u32 function)
>> break;
>> case 0xd: {
>> u64 permitted_xcr0 = supported_xcr0 &
>> xstate_get_guest_group_perm();
>
> Yang, I think you should have included your fix [1] in your series [2]
> in the first place, before using it widely like [3].
>
>> + permitted_xcr0 = ((permitted_xcr0 &
>> XFEATURES_MASK_XTILE) != XFEATURES_MASK_XTILE)
>> + ? permitted_xcr0
>> + : permitted_xcr0 & ~XFEATURES_MASK_XTILE;
>> u64 permitted_xss = supported_xss;
>>
>> entry->eax &= permitted_xcr0;
>>
>
> Well, first of all, one patch should fix one issue, not two or more, no?
>
> But this hunk looks duplicate with this [4].
>
> Thanks,
> Chang
>
>
> [1]
> https://lore.kernel.org/lkml/[email protected]/
> [2]
> https://lore.kernel.org/lkml/[email protected]/
> [3]
> https://lore.kernel.org/lkml/[email protected]/
> [4]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kvm/x86.c#n1033
>
On Sat, Jan 29 2022 at 09:36, Chang S. Bae wrote:
> Update the arch_prctl test to check the permission bitmap whether the
> requested feature is added as expected or not.
>
> Every non-dynamic feature that is enabled is permitted already for use.
> TILECFG is not dynamic feature. Ensure the bit is always on from
> ARCH_GET_XCOMP_PERM.
Running it on a machine which does not have AMX results in:
amx_64: [FAIL] xstate cpuid: invalid tile data size/offset: 0/0: Success
It's not a failure, really. Selftests are supposed to run on all
machines and the proper thing to do if a hardware feature is not
available is to SKIP the test and return 0.
Thanks,
tglx
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: 063452fd94d153d4eb38ad58f210f3d37a09cca4
Gitweb: https://git.kernel.org/tip/063452fd94d153d4eb38ad58f210f3d37a09cca4
Author: Yang Zhong <[email protected]>
AuthorDate: Sat, 29 Jan 2022 09:36:46 -08:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Wed, 23 Mar 2022 21:28:34 +01:00
x86/fpu/xstate: Fix the ARCH_REQ_XCOMP_PERM implementation
ARCH_REQ_XCOMP_PERM is supposed to add the requested feature to the
permission bitmap of thread_group_leader()->fpu. But the code overwrites
the bitmap with the requested feature bit only rather than adding it.
Fix the code to add the requested feature bit to the master bitmask.
Fixes: db8268df0983 ("x86/arch_prctl: Add controls for dynamic XSTATE components")
Signed-off-by: Yang Zhong <[email protected]>
Signed-off-by: Chang S. Bae <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: [email protected]
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/kernel/fpu/xstate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 7c7824a..dc6d5e9 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1639,7 +1639,7 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest)
perm = guest ? &fpu->guest_perm : &fpu->perm;
/* Pairs with the READ_ONCE() in xstate_get_group_perm() */
- WRITE_ONCE(perm->__state_perm, requested);
+ WRITE_ONCE(perm->__state_perm, mask);
/* Protected by sighand lock */
perm->__state_size = ksize;
perm->__user_state_size = usize;
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: 20df737561484cb2d42e537663c03a7311d2b3c1
Gitweb: https://git.kernel.org/tip/20df737561484cb2d42e537663c03a7311d2b3c1
Author: Chang S. Bae <[email protected]>
AuthorDate: Sat, 29 Jan 2022 09:36:47 -08:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Wed, 23 Mar 2022 21:28:34 +01:00
selftests/x86/amx: Update the ARCH_REQ_XCOMP_PERM test
Update the arch_prctl test to check the permission bitmap whether the
requested feature is added as expected or not.
Every non-dynamic feature that is enabled is permitted already for use.
TILECFG is not dynamic feature. Ensure the bit is always on from
ARCH_GET_XCOMP_PERM.
Signed-off-by: Chang S. Bae <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
tools/testing/selftests/x86/amx.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/x86/amx.c b/tools/testing/selftests/x86/amx.c
index 3615ef4..2189f03 100644
--- a/tools/testing/selftests/x86/amx.c
+++ b/tools/testing/selftests/x86/amx.c
@@ -368,9 +368,16 @@ static void req_xtiledata_perm(void)
static void validate_req_xcomp_perm(enum expected_result exp)
{
- unsigned long bitmask;
+ unsigned long bitmask, expected_bitmask;
long rc;
+ rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_PERM, &bitmask);
+ if (rc) {
+ fatal_error("prctl(ARCH_GET_XCOMP_PERM) error: %ld", rc);
+ } else if (!(bitmask & XFEATURE_MASK_XTILECFG)) {
+ fatal_error("ARCH_GET_XCOMP_PERM returns XFEATURE_XTILECFG off.");
+ }
+
rc = syscall(SYS_arch_prctl, ARCH_REQ_XCOMP_PERM, XFEATURE_XTILEDATA);
if (exp == FAIL_EXPECTED) {
if (rc) {
@@ -383,10 +390,15 @@ static void validate_req_xcomp_perm(enum expected_result exp)
fatal_error("ARCH_REQ_XCOMP_PERM saw unexpected failure.\n");
}
+ expected_bitmask = bitmask | XFEATURE_MASK_XTILEDATA;
+
rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_PERM, &bitmask);
if (rc) {
fatal_error("prctl(ARCH_GET_XCOMP_PERM) error: %ld", rc);
- } else if (bitmask & XFEATURE_MASK_XTILE) {
+ } else if (bitmask != expected_bitmask) {
+ fatal_error("ARCH_REQ_XCOMP_PERM set a wrong bitmask: %lx, expected: %lx.\n",
+ bitmask, expected_bitmask);
+ } else {
printf("\tARCH_REQ_XCOMP_PERM is successful.\n");
}
}
On 3/23/2022 9:44 AM, Thomas Gleixner wrote:
>
> Running it on a machine which does not have AMX results in:
>
> amx_64: [FAIL] xstate cpuid: invalid tile data size/offset: 0/0: Success
>
> It's not a failure, really. Selftests are supposed to run on all
> machines and the proper thing to do if a hardware feature is not
> available is to SKIP the test and return 0.
Ah, right. The test should be just *skipped* on non-AMX or even
non-XSAVE systems.
Will follow up with a separate patch.
Thanks,
Chang
Thomas, Dave,
can this series be included in 5.18 and CCed to stable?
The bug makes the __state_perm field completely wrong. As a result,
arch_prctl(ARCH_GET_XCOMP_PERM) only returns the features that were
requested last, forgetting what was already in __state_perm (the
"permitted" argument to __xstate_request_perm).
In KVM, it is a bit worse. It affects
arch_prctl(ARCH_GET_XCOMP_GUEST_PERM) in the same way and also
ioctl(KVM_GET_SUPPORTED_CPUID), but the bug can also make KVM return the
wrong xsave state size to userspace. It's likely to go unnoticed by
userspace until Intel adds non-dynamic states above a dynamic one, but
potentially userspace could allocate a buffer that is too small.
Paolo
On 1/29/22 18:36, Chang S. Bae wrote:
> Changes from V3:
> * Rebased onto 5.17-rc1.
>
> V3: https://lore.kernel.org/lkml/[email protected]/
>
> ---
>
> The recent x86 dynamic state support incorporates the arch_prctl option to
> request permission before using a dynamic state.
>
> It was designed to add the requested feature in the group leader's
> permission bitmask so that every thread can reference this master bitmask.
> The group leader is assumed to be unchanged here. The mainline is the case
> as a group leader is identified at fork() or exec() time only.
>
> This master bitmask should include non-dynamic features always, as they
> are permitted by default. Users may check them via ARCH_GET_XCOMP_PERM.
>
> But, in hindsight, the implementation does overwrite the bitmask with the
> requested bit only, instead of adding the bit to the existing one. This
> overwrite effectively revokes the permission that is granted already.
>
> Fix the code and also update the selftest to disclose the issue if there
> is.
>
> Chang S. Bae (1):
> selftests/x86/amx: Update the ARCH_REQ_XCOMP_PERM test
>
> Yang Zhong (1):
> x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation
>
> arch/x86/kernel/fpu/xstate.c | 2 +-
> tools/testing/selftests/x86/amx.c | 16 ++++++++++++++--
> 2 files changed, 15 insertions(+), 3 deletions(-)
>
>
> base-commit: e783362eb54cd99b2cac8b3a9aeac942e6f6ac07