* [PATCH v4 1/2] x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation
2022-01-29 17:36 [PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test Chang S. Bae
@ 2022-01-29 17:36 ` Chang S. Bae
2022-03-07 12:20 ` Hao Xiang
2022-03-23 23:31 ` [tip: x86/urgent] x86/fpu/xstate: " tip-bot2 for Yang Zhong
2022-01-29 17:36 ` [PATCH v4 2/2] selftests/x86/amx: Update the ARCH_REQ_XCOMP_PERM test Chang S. Bae
2022-03-23 11:04 ` ping Re: [PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test Paolo Bonzini
2 siblings, 2 replies; 16+ messages in thread
From: Chang S. Bae @ 2022-01-29 17:36 UTC (permalink / raw)
To: linux-kernel, x86, tglx, bp, dave.hansen, mingo
Cc: yang.zhong, ravi.v.shankar, chang.seok.bae
From: Yang Zhong <yang.zhong@intel.com>
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 <yang.zhong@intel.com>
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/2] x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation
2022-01-29 17:36 ` [PATCH v4 1/2] x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation Chang S. Bae
@ 2022-03-07 12:20 ` Hao Xiang
2022-03-07 18:53 ` Chang S. Bae
2022-03-23 23:31 ` [tip: x86/urgent] x86/fpu/xstate: " tip-bot2 for Yang Zhong
1 sibling, 1 reply; 16+ messages in thread
From: Hao Xiang @ 2022-03-07 12:20 UTC (permalink / raw)
To: chang.seok.bae
Cc: bp, dave.hansen, linux-kernel, mingo, ravi.v.shankar, tglx, x86,
yang.zhong
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;
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/2] x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation
2022-03-07 12:20 ` Hao Xiang
@ 2022-03-07 18:53 ` Chang S. Bae
2022-03-08 8:36 ` Hao Xiang
0 siblings, 1 reply; 16+ messages in thread
From: Chang S. Bae @ 2022-03-07 18:53 UTC (permalink / raw)
To: Hao Xiang, yang.zhong
Cc: bp, dave.hansen, linux-kernel, mingo, ravi.v.shankar, tglx, x86
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/20211108222815.4078-1-yang.zhong@intel.com/
[2]
https://lore.kernel.org/lkml/20220105123532.12586-1-yang.zhong@intel.com/
[3]
https://lore.kernel.org/lkml/20220105123532.12586-2-yang.zhong@intel.com/
[4]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kvm/x86.c#n1033
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/2] x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation
2022-03-07 18:53 ` Chang S. Bae
@ 2022-03-08 8:36 ` Hao Xiang
0 siblings, 0 replies; 16+ messages in thread
From: Hao Xiang @ 2022-03-08 8:36 UTC (permalink / raw)
To: Chang S. Bae, yang.zhong
Cc: bp, dave.hansen, linux-kernel, mingo, ravi.v.shankar, tglx, x86
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/20211108222815.4078-1-yang.zhong@intel.com/
> [2]
> https://lore.kernel.org/lkml/20220105123532.12586-1-yang.zhong@intel.com/
> [3]
> https://lore.kernel.org/lkml/20220105123532.12586-2-yang.zhong@intel.com/
> [4]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kvm/x86.c#n1033
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [tip: x86/urgent] x86/fpu/xstate: Fix the ARCH_REQ_XCOMP_PERM implementation
2022-01-29 17:36 ` [PATCH v4 1/2] x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation Chang S. Bae
2022-03-07 12:20 ` Hao Xiang
@ 2022-03-23 23:31 ` tip-bot2 for Yang Zhong
1 sibling, 0 replies; 16+ messages in thread
From: tip-bot2 for Yang Zhong @ 2022-03-23 23:31 UTC (permalink / raw)
To: linux-tip-commits
Cc: Yang Zhong, Chang S. Bae, Thomas Gleixner, Paolo Bonzini, stable,
x86, linux-kernel
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 <yang.zhong@intel.com>
AuthorDate: Sat, 29 Jan 2022 09:36:46 -08:00
Committer: Thomas Gleixner <tglx@linutronix.de>
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 <yang.zhong@intel.com>
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Paolo Bonzini <bonzini@gnu.org>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/20220129173647.27981-2-chang.seok.bae@intel.com
---
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;
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 2/2] selftests/x86/amx: Update the ARCH_REQ_XCOMP_PERM test
2022-01-29 17:36 [PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test Chang S. Bae
2022-01-29 17:36 ` [PATCH v4 1/2] x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation Chang S. Bae
@ 2022-01-29 17:36 ` Chang S. Bae
2022-03-23 16:44 ` Thomas Gleixner
2022-03-23 23:31 ` [tip: x86/urgent] " tip-bot2 for Chang S. Bae
2022-03-23 11:04 ` ping Re: [PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test Paolo Bonzini
2 siblings, 2 replies; 16+ messages in thread
From: Chang S. Bae @ 2022-01-29 17:36 UTC (permalink / raw)
To: linux-kernel, x86, tglx, bp, dave.hansen, mingo
Cc: yang.zhong, ravi.v.shankar, chang.seok.bae, linux-kselftest
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 <chang.seok.bae@intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: linux-kselftest@vger.kernel.org
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 2/2] selftests/x86/amx: Update the ARCH_REQ_XCOMP_PERM test
2022-01-29 17:36 ` [PATCH v4 2/2] selftests/x86/amx: Update the ARCH_REQ_XCOMP_PERM test Chang S. Bae
@ 2022-03-23 16:44 ` Thomas Gleixner
2022-03-23 21:27 ` Chang S. Bae
2022-03-23 23:31 ` [tip: x86/urgent] " tip-bot2 for Chang S. Bae
1 sibling, 1 reply; 16+ messages in thread
From: Thomas Gleixner @ 2022-03-23 16:44 UTC (permalink / raw)
To: Chang S. Bae, linux-kernel, x86, bp, dave.hansen, mingo
Cc: yang.zhong, ravi.v.shankar, chang.seok.bae, linux-kselftest
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 2/2] selftests/x86/amx: Update the ARCH_REQ_XCOMP_PERM test
2022-03-23 16:44 ` Thomas Gleixner
@ 2022-03-23 21:27 ` Chang S. Bae
0 siblings, 0 replies; 16+ messages in thread
From: Chang S. Bae @ 2022-03-23 21:27 UTC (permalink / raw)
To: Thomas Gleixner, linux-kernel, x86, bp, dave.hansen, mingo
Cc: yang.zhong, ravi.v.shankar, linux-kselftest
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [tip: x86/urgent] selftests/x86/amx: Update the ARCH_REQ_XCOMP_PERM test
2022-01-29 17:36 ` [PATCH v4 2/2] selftests/x86/amx: Update the ARCH_REQ_XCOMP_PERM test Chang S. Bae
2022-03-23 16:44 ` Thomas Gleixner
@ 2022-03-23 23:31 ` tip-bot2 for Chang S. Bae
1 sibling, 0 replies; 16+ messages in thread
From: tip-bot2 for Chang S. Bae @ 2022-03-23 23:31 UTC (permalink / raw)
To: linux-tip-commits; +Cc: Chang S. Bae, Thomas Gleixner, x86, linux-kernel
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 <chang.seok.bae@intel.com>
AuthorDate: Sat, 29 Jan 2022 09:36:47 -08:00
Committer: Thomas Gleixner <tglx@linutronix.de>
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 <chang.seok.bae@intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/r/20220129173647.27981-3-chang.seok.bae@intel.com
---
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");
}
}
^ permalink raw reply related [flat|nested] 16+ messages in thread
* ping Re: [PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test
2022-01-29 17:36 [PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test Chang S. Bae
2022-01-29 17:36 ` [PATCH v4 1/2] x86/arch_prctl: Fix the ARCH_REQ_XCOMP_PERM implementation Chang S. Bae
2022-01-29 17:36 ` [PATCH v4 2/2] selftests/x86/amx: Update the ARCH_REQ_XCOMP_PERM test Chang S. Bae
@ 2022-03-23 11:04 ` Paolo Bonzini
2022-03-23 12:27 ` Thomas Gleixner
2 siblings, 1 reply; 16+ messages in thread
From: Paolo Bonzini @ 2022-03-23 11:04 UTC (permalink / raw)
To: tglx, dave.hansen
Cc: yang.zhong, ravi.v.shankar, mingo, Chang S. Bae, bp, x86,
linux-kernel, KVM list
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/20211110003209.21666-1-chang.seok.bae@intel.com/
>
> ---
>
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: ping Re: [PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test
2022-03-23 11:04 ` ping Re: [PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test Paolo Bonzini
@ 2022-03-23 12:27 ` Thomas Gleixner
2022-03-23 12:55 ` Thomas Gleixner
0 siblings, 1 reply; 16+ messages in thread
From: Thomas Gleixner @ 2022-03-23 12:27 UTC (permalink / raw)
To: Paolo Bonzini, dave.hansen
Cc: yang.zhong, ravi.v.shankar, mingo, Chang S. Bae, bp, x86,
linux-kernel, KVM list
Paolo,
On Wed, Mar 23 2022 at 12:04, Paolo Bonzini wrote:
> can this series be included in 5.18 and CCed to stable?
working on it. There is another issue with that which I'm currently
looking into.
Thanks,
tglx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: ping Re: [PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test
2022-03-23 12:27 ` Thomas Gleixner
@ 2022-03-23 12:55 ` Thomas Gleixner
2022-03-23 14:24 ` Paolo Bonzini
0 siblings, 1 reply; 16+ messages in thread
From: Thomas Gleixner @ 2022-03-23 12:55 UTC (permalink / raw)
To: Paolo Bonzini, dave.hansen
Cc: yang.zhong, ravi.v.shankar, mingo, Chang S. Bae, bp, x86,
linux-kernel, KVM list
On Wed, Mar 23 2022 at 13:27, Thomas Gleixner wrote:
> On Wed, Mar 23 2022 at 12:04, Paolo Bonzini wrote:
>> can this series be included in 5.18 and CCed to stable?
>
> working on it. There is another issue with that which I'm currently
> looking into.
The size calculation for the kernel state fails to take supervisor
states into account. Up to 5.18 that did not matter because ENQCMD/PASID
was disabled. But now it matters...
Thanks,
tglx
---
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1625,6 +1625,8 @@ static int __xstate_request_perm(u64 per
/* Calculate the resulting kernel state size */
mask = permitted | requested;
+ /* Take supervisor states into account */
+ mask |= xfeatures_mask_supervisor();
ksize = xstate_calculate_size(mask, compacted);
/* Calculate the resulting user state size */
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: ping Re: [PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test
2022-03-23 12:55 ` Thomas Gleixner
@ 2022-03-23 14:24 ` Paolo Bonzini
2022-03-23 17:07 ` Thomas Gleixner
0 siblings, 1 reply; 16+ messages in thread
From: Paolo Bonzini @ 2022-03-23 14:24 UTC (permalink / raw)
To: Thomas Gleixner, dave.hansen
Cc: yang.zhong, ravi.v.shankar, mingo, Chang S. Bae, bp, x86,
linux-kernel, KVM list
On 3/23/22 13:55, Thomas Gleixner wrote:
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1625,6 +1625,8 @@ static int __xstate_request_perm(u64 per
>
> /* Calculate the resulting kernel state size */
> mask = permitted | requested;
> + /* Take supervisor states into account */
> + mask |= xfeatures_mask_supervisor();
> ksize = xstate_calculate_size(mask, compacted);
>
This should be only added in for the !guest case.
Paolo
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: ping Re: [PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test
2022-03-23 14:24 ` Paolo Bonzini
@ 2022-03-23 17:07 ` Thomas Gleixner
2022-03-23 17:20 ` Thomas Gleixner
0 siblings, 1 reply; 16+ messages in thread
From: Thomas Gleixner @ 2022-03-23 17:07 UTC (permalink / raw)
To: Paolo Bonzini, dave.hansen
Cc: yang.zhong, ravi.v.shankar, mingo, Chang S. Bae, bp, x86,
linux-kernel, KVM list
On Wed, Mar 23 2022 at 15:24, Paolo Bonzini wrote:
> On 3/23/22 13:55, Thomas Gleixner wrote:
>> --- a/arch/x86/kernel/fpu/xstate.c
>> +++ b/arch/x86/kernel/fpu/xstate.c
>> @@ -1625,6 +1625,8 @@ static int __xstate_request_perm(u64 per
>>
>> /* Calculate the resulting kernel state size */
>> mask = permitted | requested;
>> + /* Take supervisor states into account */
>> + mask |= xfeatures_mask_supervisor();
>> ksize = xstate_calculate_size(mask, compacted);
>>
>
> This should be only added in for the !guest case.
Yes, I figured that out already :)
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: ping Re: [PATCH v4 0/2] x86: Fix ARCH_REQ_XCOMP_PERM and update the test
2022-03-23 17:07 ` Thomas Gleixner
@ 2022-03-23 17:20 ` Thomas Gleixner
0 siblings, 0 replies; 16+ messages in thread
From: Thomas Gleixner @ 2022-03-23 17:20 UTC (permalink / raw)
To: Paolo Bonzini, dave.hansen
Cc: yang.zhong, ravi.v.shankar, mingo, Chang S. Bae, bp, x86,
linux-kernel, KVM list
On Wed, Mar 23 2022 at 18:07, Thomas Gleixner wrote:
> On Wed, Mar 23 2022 at 15:24, Paolo Bonzini wrote:
>> On 3/23/22 13:55, Thomas Gleixner wrote:
>>> --- a/arch/x86/kernel/fpu/xstate.c
>>> +++ b/arch/x86/kernel/fpu/xstate.c
>>> @@ -1625,6 +1625,8 @@ static int __xstate_request_perm(u64 per
>>>
>>> /* Calculate the resulting kernel state size */
>>> mask = permitted | requested;
>>> + /* Take supervisor states into account */
>>> + mask |= xfeatures_mask_supervisor();
>>> ksize = xstate_calculate_size(mask, compacted);
>>>
>>
>> This should be only added in for the !guest case.
>
> Yes, I figured that out already :)
Hrm, that has more consequences vs. the buffer conversion functions. Let
me stare some more.
Thanks,
tglx
^ permalink raw reply [flat|nested] 16+ messages in thread