[PATCH v2] x86: dovetail: reinstate I/O bitmap on user entry

[PATCH v2] x86: dovetail: reinstate I/O bitmap on user entry

[Philippe Gerum]

From: Philippe Gerum <rpm@xenomai.org>

We have to fix up the TSS with the proper I/O bitmap settings in
arch_dovetail_switch_finish() when the incoming Dovetail-enabled task
is about to re-enter user mode.

This fixes an application crash observed when such a task would rely
on iopl() to raise its I/O permissions then block, relinquishing the
CPU to another task invalidating them, before the former eventually
resumes in user mode.

See https://xenomai.org/pipermail/xenomai/2022-March/047451.html.

Reported-by: Richard Weinberger <richard.weinberger@gmail.com>
Tested-by: Richard Weinberger <richard.weinberger@gmail.com>
Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 arch/x86/include/asm/dovetail.h | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/dovetail.h b/arch/x86/include/asm/dovetail.h
index 9cf811fe98ba..940726f16f2e 100644
--- a/arch/x86/include/asm/dovetail.h
+++ b/arch/x86/include/asm/dovetail.h
@@ -9,6 +9,7 @@
 #if !defined(__ASSEMBLY__) && defined(CONFIG_DOVETAIL)
 
 #include <asm/fpu/api.h>
+#include <asm/io_bitmap.h>
 
 static inline void arch_dovetail_exec_prepare(void)
 {
@@ -25,11 +26,18 @@ void arch_dovetail_switch_prepare(bool leave_inband)
 static inline
 void arch_dovetail_switch_finish(bool enter_inband)
 {
-	if (enter_inband)
+	unsigned int ti_work = READ_ONCE(current_thread_info()->flags);
+
+	if (unlikely(ti_work & _TIF_IO_BITMAP))
+		tss_update_io_bitmap();
+
+	if (enter_inband) {
 		fpu__resume_inband();
-	else if (!(current->flags & PF_KTHREAD) &&
-		test_thread_flag(TIF_NEED_FPU_LOAD))
-		switch_fpu_return();
+	} else {
+		  if (unlikely(ti_work & _TIF_NEED_FPU_LOAD &&
+				  !(current->flags & PF_KTHREAD)))
+			  switch_fpu_return();
+	}
 }
 
 #endif
-- 
2.34.1

Re: [PATCH v2] x86: dovetail: reinstate I/O bitmap on user entry

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Philippe Gerum" <rpm@xenomai.org>
> An: "xenomai" <xenomai@xenomai.org>
> CC: "Philippe Gerum" <rpm@xenomai.org>, "Richard Weinberger" <richard.weinberger@gmail.com>
> Gesendet: Sonntag, 3. April 2022 16:43:48
> Betreff: [PATCH v2] x86: dovetail: reinstate I/O bitmap on user entry

> From: Philippe Gerum <rpm@xenomai.org>
> 
> We have to fix up the TSS with the proper I/O bitmap settings in
> arch_dovetail_switch_finish() when the incoming Dovetail-enabled task
> is about to re-enter user mode.
> 
> This fixes an application crash observed when such a task would rely
> on iopl() to raise its I/O permissions then block, relinquishing the
> CPU to another task invalidating them, before the former eventually
> resumes in user mode.
> 
> See https://xenomai.org/pipermail/xenomai/2022-March/047451.html.
> 
> Reported-by: Richard Weinberger <richard.weinberger@gmail.com>
> Tested-by: Richard Weinberger <richard.weinberger@gmail.com>
> Signed-off-by: Philippe Gerum <rpm@xenomai.org>
> ---
> arch/x86/include/asm/dovetail.h | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/dovetail.h b/arch/x86/include/asm/dovetail.h
> index 9cf811fe98ba..940726f16f2e 100644
> --- a/arch/x86/include/asm/dovetail.h
> +++ b/arch/x86/include/asm/dovetail.h
> @@ -9,6 +9,7 @@
> #if !defined(__ASSEMBLY__) && defined(CONFIG_DOVETAIL)
> 
> #include <asm/fpu/api.h>
> +#include <asm/io_bitmap.h>
> 
> static inline void arch_dovetail_exec_prepare(void)
> {
> @@ -25,11 +26,18 @@ void arch_dovetail_switch_prepare(bool leave_inband)
> static inline
> void arch_dovetail_switch_finish(bool enter_inband)
> {
> -	if (enter_inband)
> +	unsigned int ti_work = READ_ONCE(current_thread_info()->flags);
> +
> +	if (unlikely(ti_work & _TIF_IO_BITMAP))
> +		tss_update_io_bitmap();
> +
> +	if (enter_inband) {
> 		fpu__resume_inband();
> -	else if (!(current->flags & PF_KTHREAD) &&
> -		test_thread_flag(TIF_NEED_FPU_LOAD))
> -		switch_fpu_return();
> +	} else {
> +		  if (unlikely(ti_work & _TIF_NEED_FPU_LOAD &&
> +				  !(current->flags & PF_KTHREAD)))
> +			  switch_fpu_return();
> +	}
> }

For the whole series:
Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks for addressing the issue so quickly. :-)

Thanks,
//richard

Re: [PATCH v2] x86: dovetail: reinstate I/O bitmap on user entry

[Philippe Gerum]

Richard Weinberger <richard@nod.at> writes:

> ----- Ursprüngliche Mail -----
>> Von: "Philippe Gerum" <rpm@xenomai.org>
>> An: "xenomai" <xenomai@xenomai.org>
>> CC: "Philippe Gerum" <rpm@xenomai.org>, "Richard Weinberger" <richard.weinberger@gmail.com>
>> Gesendet: Sonntag, 3. April 2022 16:43:48
>> Betreff: [PATCH v2] x86: dovetail: reinstate I/O bitmap on user entry
>
>> From: Philippe Gerum <rpm@xenomai.org>
>> 
>> We have to fix up the TSS with the proper I/O bitmap settings in
>> arch_dovetail_switch_finish() when the incoming Dovetail-enabled task
>> is about to re-enter user mode.
>> 
>> This fixes an application crash observed when such a task would rely
>> on iopl() to raise its I/O permissions then block, relinquishing the
>> CPU to another task invalidating them, before the former eventually
>> resumes in user mode.
>> 
>> See https://xenomai.org/pipermail/xenomai/2022-March/047451.html.
>> 
>> Reported-by: Richard Weinberger <richard.weinberger@gmail.com>
>> Tested-by: Richard Weinberger <richard.weinberger@gmail.com>
>> Signed-off-by: Philippe Gerum <rpm@xenomai.org>
>> ---
>> arch/x86/include/asm/dovetail.h | 16 ++++++++++++----
>> 1 file changed, 12 insertions(+), 4 deletions(-)
>> 
>> diff --git a/arch/x86/include/asm/dovetail.h b/arch/x86/include/asm/dovetail.h
>> index 9cf811fe98ba..940726f16f2e 100644
>> --- a/arch/x86/include/asm/dovetail.h
>> +++ b/arch/x86/include/asm/dovetail.h
>> @@ -9,6 +9,7 @@
>> #if !defined(__ASSEMBLY__) && defined(CONFIG_DOVETAIL)
>> 
>> #include <asm/fpu/api.h>
>> +#include <asm/io_bitmap.h>
>> 
>> static inline void arch_dovetail_exec_prepare(void)
>> {
>> @@ -25,11 +26,18 @@ void arch_dovetail_switch_prepare(bool leave_inband)
>> static inline
>> void arch_dovetail_switch_finish(bool enter_inband)
>> {
>> -	if (enter_inband)
>> +	unsigned int ti_work = READ_ONCE(current_thread_info()->flags);
>> +
>> +	if (unlikely(ti_work & _TIF_IO_BITMAP))
>> +		tss_update_io_bitmap();
>> +
>> +	if (enter_inband) {
>> 		fpu__resume_inband();
>> -	else if (!(current->flags & PF_KTHREAD) &&
>> -		test_thread_flag(TIF_NEED_FPU_LOAD))
>> -		switch_fpu_return();
>> +	} else {
>> +		  if (unlikely(ti_work & _TIF_NEED_FPU_LOAD &&
>> +				  !(current->flags & PF_KTHREAD)))
>> +			  switch_fpu_return();
>> +	}
>> }
>
> For the whole series:
> Reviewed-by: Richard Weinberger <richard@nod.at>
>

These are the two remaining patches in the final series, ok for both?

tick: irq_pipeline: fix proxying of a broadcast device
x86: dovetail: reinstate I/O bitmap on user entry

> Thanks for addressing the issue so quickly. :-)
>

You are welcome, your test case made it easy.

-- 
Philippe.

Re: [PATCH v2] x86: dovetail: reinstate I/O bitmap on user entry

[Florian Bezdeka]

Hi all,

On 03.04.22 19:49, Philippe Gerum via Xenomai wrote:
> 
> Richard Weinberger <richard@nod.at> writes:
> 
>> ----- Ursprüngliche Mail -----
>>> Von: "Philippe Gerum" <rpm@xenomai.org>
>>> An: "xenomai" <xenomai@xenomai.org>
>>> CC: "Philippe Gerum" <rpm@xenomai.org>, "Richard Weinberger" <richard.weinberger@gmail.com>
>>> Gesendet: Sonntag, 3. April 2022 16:43:48
>>> Betreff: [PATCH v2] x86: dovetail: reinstate I/O bitmap on user entry
>>
>>> From: Philippe Gerum <rpm@xenomai.org>
>>>
>>> We have to fix up the TSS with the proper I/O bitmap settings in
>>> arch_dovetail_switch_finish() when the incoming Dovetail-enabled task
>>> is about to re-enter user mode.
>>>
>>> This fixes an application crash observed when such a task would rely
>>> on iopl() to raise its I/O permissions then block, relinquishing the
>>> CPU to another task invalidating them, before the former eventually
>>> resumes in user mode.
>>>
>>> See https://xenomai.org/pipermail/xenomai/2022-March/047451.html.
>>>
>>> Reported-by: Richard Weinberger <richard.weinberger@gmail.com>
>>> Tested-by: Richard Weinberger <richard.weinberger@gmail.com>
>>> Signed-off-by: Philippe Gerum <rpm@xenomai.org>
>>> ---
>>> arch/x86/include/asm/dovetail.h | 16 ++++++++++++----
>>> 1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/x86/include/asm/dovetail.h b/arch/x86/include/asm/dovetail.h
>>> index 9cf811fe98ba..940726f16f2e 100644
>>> --- a/arch/x86/include/asm/dovetail.h
>>> +++ b/arch/x86/include/asm/dovetail.h
>>> @@ -9,6 +9,7 @@
>>> #if !defined(__ASSEMBLY__) && defined(CONFIG_DOVETAIL)
>>>
>>> #include <asm/fpu/api.h>
>>> +#include <asm/io_bitmap.h>
>>>
>>> static inline void arch_dovetail_exec_prepare(void)
>>> {
>>> @@ -25,11 +26,18 @@ void arch_dovetail_switch_prepare(bool leave_inband)
>>> static inline
>>> void arch_dovetail_switch_finish(bool enter_inband)
>>> {
>>> -	if (enter_inband)
>>> +	unsigned int ti_work = READ_ONCE(current_thread_info()->flags);
>>> +
>>> +	if (unlikely(ti_work & _TIF_IO_BITMAP))
>>> +		tss_update_io_bitmap();
>>> +
>>> +	if (enter_inband) {
>>> 		fpu__resume_inband();
>>> -	else if (!(current->flags & PF_KTHREAD) &&
>>> -		test_thread_flag(TIF_NEED_FPU_LOAD))
>>> -		switch_fpu_return();
>>> +	} else {
>>> +		  if (unlikely(ti_work & _TIF_NEED_FPU_LOAD &&
>>> +				  !(current->flags & PF_KTHREAD)))
>>> +			  switch_fpu_return();
>>> +	}
>>> }
>>
>> For the whole series:
>> Reviewed-by: Richard Weinberger <richard@nod.at>
This email address is different from the address used in the Reported-
and Tested-By tags in the patch.

>>
> 
> These are the two remaining patches in the final series, ok for both?
> 
> tick: irq_pipeline: fix proxying of a broadcast device
> x86: dovetail: reinstate I/O bitmap on user entry
> 
>> Thanks for addressing the issue so quickly. :-)
>>
> 
> You are welcome, your test case made it easy.

Any chance to integrate the test case into the test suite?

Regards,
Florian

Re: [PATCH v2] x86: dovetail: reinstate I/O bitmap on user entry

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Florian Bezdeka" <florian.bezdeka@siemens.com>
> An: "Philippe Gerum" <rpm@xenomai.org>, "richard" <richard@nod.at>
> CC: "xenomai" <xenomai@xenomai.org>
> Gesendet: Sonntag, 3. April 2022 21:41:51
> Betreff: Re: [PATCH v2] x86: dovetail: reinstate I/O bitmap on user entry
>>> For the whole series:
>>> Reviewed-by: Richard Weinberger <richard@nod.at>
> This email address is different from the address used in the Reported-
> and Tested-By tags in the patch.

Sorry, this is due my wicked mail setup.
I use Google mail as /dev/null for mailing lists. :-)

>> These are the two remaining patches in the final series, ok for both?
>> 
>> tick: irq_pipeline: fix proxying of a broadcast device
>> x86: dovetail: reinstate I/O bitmap on user entry
>> 
>>> Thanks for addressing the issue so quickly. :-)
>>>
>> 
>> You are welcome, your test case made it easy.
> 
> Any chance to integrate the test case into the test suite?

Yes, the only thing we have to keep in mind is that the test
writes to an IO port. I don't know whether this will make
some hardware unhappy.
But I think the test can easily get changed to reading instead
of writing.

Thanks,
//richard

Re: [PATCH v2] x86: dovetail: reinstate I/O bitmap on user entry

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Philippe Gerum" <rpm@xenomai.org>
> These are the two remaining patches in the final series, ok for both?
> 
> tick: irq_pipeline: fix proxying of a broadcast device
> x86: dovetail: reinstate I/O bitmap on user entry

I meant this patches;
[PATCH 1/4] sched: dovetail: pass thread-info bits to arch_dovetail_switch_finish()
[PATCH 2/4] ARM: dovetail: fix up arch_dovetail_switch_finish()
[PATCH 3/4] arm64: dovetail: fix up arch_dovetail_switch_finish()
[PATCH 4/4] x86: dovetail: reinstate I/O bitmap on out-of-band user entry

Where 4/4 got replaced by this v2 one.

I didn't look into "tick: irq_pipeline: fix proxying of a broadcast device" but can later today.

BTW: Do you have a Patchwork instance for the Xenomai mailing list? I makes dealing with patch
series so much easier.

Or even better, can we have Xenomai at lore.kernel.org? Then we can use b4. :-)

Thanks,
//richard

Re: [PATCH v2] x86: dovetail: reinstate I/O bitmap on user entry

[Philippe Gerum]

Richard Weinberger <richard@nod.at> writes:

> ----- Ursprüngliche Mail -----
>> Von: "Philippe Gerum" <rpm@xenomai.org>
>> These are the two remaining patches in the final series, ok for both?
>> 
>> tick: irq_pipeline: fix proxying of a broadcast device
>> x86: dovetail: reinstate I/O bitmap on user entry
>
> I meant this patches;
> [PATCH 1/4] sched: dovetail: pass thread-info bits to arch_dovetail_switch_finish()
> [PATCH 2/4] ARM: dovetail: fix up arch_dovetail_switch_finish()
> [PATCH 3/4] arm64: dovetail: fix up arch_dovetail_switch_finish()
> [PATCH 4/4] x86: dovetail: reinstate I/O bitmap on out-of-band user entry
>
> Where 4/4 got replaced by this v2 one.
>
> I didn't look into "tick: irq_pipeline: fix proxying of a broadcast device" but can later today.
>
> BTW: Do you have a Patchwork instance for the Xenomai mailing list?

Unfortunately not.

> I makes dealing with patch
> series so much easier.
>
> Or even better, can we have Xenomai at lore.kernel.org? Then we can use b4. :-)
>

It seems unlikely that kernel.org would consider hosting us since the
project does not upstream patches to the mainline kernel.

-- 
Philippe.

Re: [PATCH v2] x86: dovetail: reinstate I/O bitmap on user entry

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Philippe Gerum" <rpm@xenomai.org>
> It seems unlikely that kernel.org would consider hosting us since the
> project does not upstream patches to the mainline kernel.

lore hosts many projects that are not directly related to the upstream
kernel.
e.g.
https://lore.kernel.org/qemu-devel
https://lore.kernel.org/u-boot
https://lore.kernel.org/llvm
..
https://lore.kernel.org/dpdk-dev
https://lore.kernel.org/selinux-refpolicy

So, I don't see why Xenomai won't fit.

Thanks,
//richard

Re: [PATCH v2] x86: dovetail: reinstate I/O bitmap on user entry

[Wolfgang Denk]

Dear Philippe,

In message <87ee2colqg.fsf@xenomai.org> you wrote:
> 
> > BTW: Do you have a Patchwork instance for the Xenomai mailing list?
>
> Unfortunately not.
>
> > I makes dealing with patch
> > series so much easier.
> >
> > Or even better, can we have Xenomai at lore.kernel.org? Then we can use b=
> 4. :-)
>
> It seems unlikely that kernel.org would consider hosting us since the
> project does not upstream patches to the mainline kernel.

Did you try it?  I don't thhink this is a mandatory requirement.
For example, U-Boot also does not upstream any kernel patches.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
You got to learn three things. What's  real,  what's  not  real,  and
what's the difference."           - Terry Pratchett, _Witches Abroad_

Re: [PATCH v2] x86: dovetail: reinstate I/O bitmap on user entry

[Jan Kiszka]

On 06.04.22 11:13, Wolfgang Denk via Xenomai wrote:
> Dear Philippe,
> 
> In message <87ee2colqg.fsf@xenomai.org> you wrote:
>>
>>> BTW: Do you have a Patchwork instance for the Xenomai mailing list?
>>
>> Unfortunately not.
>>
>>> I makes dealing with patch
>>> series so much easier.
>>>
>>> Or even better, can we have Xenomai at lore.kernel.org? Then we can use b=
>> 4. :-)
>>
>> It seems unlikely that kernel.org would consider hosting us since the
>> project does not upstream patches to the mainline kernel.
> 
> Did you try it?  I don't thhink this is a mandatory requirement.
> For example, U-Boot also does not upstream any kernel patches.
> 

I agree, we should give that a try [1]. Philippe, would you do as
mailing list hoster or should I (and then ask you to sync with them
regarding the archive)?

Jan

[1] https://korg.docs.kernel.org/lore.html

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH v2] x86: dovetail: reinstate I/O bitmap on user entry

[Philippe Gerum]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> On 06.04.22 11:13, Wolfgang Denk via Xenomai wrote:
>> Dear Philippe,
>> 
>> In message <87ee2colqg.fsf@xenomai.org> you wrote:
>>>
>>>> BTW: Do you have a Patchwork instance for the Xenomai mailing list?
>>>
>>> Unfortunately not.
>>>
>>>> I makes dealing with patch
>>>> series so much easier.
>>>>
>>>> Or even better, can we have Xenomai at lore.kernel.org? Then we can use b=
>>> 4. :-)
>>>
>>> It seems unlikely that kernel.org would consider hosting us since the
>>> project does not upstream patches to the mainline kernel.
>> 
>> Did you try it?  I don't thhink this is a mandatory requirement.
>> For example, U-Boot also does not upstream any kernel patches.
>> 
>
> I agree, we should give that a try [1]. Philippe, would you do as
> mailing list hoster or should I (and then ask you to sync with them
> regarding the archive)?
>
> Jan
>
> [1] https://korg.docs.kernel.org/lore.html

Ok, option #2 is fine by me. I'll provide the sanitized archive when
asked to.

-- 
Philippe.