do_iret bug in xen

do_iret bug in xen

[Ashish Bijlani]

[-- Attachment #1.1: Type: text/plain, Size: 554 bytes --]

"do_iret" (slow iret via hyercall) can introduce a race condition as
"current" can change during the execution of the function. all hypercalls
run with "sti" on, so an interrupt on a processor causing the control to
enter in "__enter_scheduler" after reading current can change the current
process on that processor.

code excerpt

"
    struct iret_context iret_saved;
    struct vcpu *v = current;

    if ( unlikely(copy_from_user(&iret_saved, (void *)regs->rsp,
                                 sizeof(iret_saved))) )
    {

"

Any thoughts on this?

[-- Attachment #1.2: Type: text/html, Size: 881 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: do_iret bug in xen

[Daniel Stodden]

On Tue, 2007-11-27 at 13:59 -0500, Ashish Bijlani wrote:
> "do_iret" (slow iret via hyercall) can introduce a race condition as
> "current" can change during the execution of the function. all
> hypercalls run with "sti" on, so an interrupt on a processor causing
> the control to enter in "__enter_scheduler" after reading current can
> change the current process on that processor. 

follow the code e.g. surounding ret_from_intr. __enter_scheduler is a
softirq handler. it will only be run upon return to guest context, not
when returning to an interrupted hypervisor. xen is not preemptible.

regards,
daniel

-- 
Daniel Stodden
LRR     -      Lehrstuhl für Rechnertechnik und Rechnerorganisation
Institut für Informatik der TU München             D-85748 Garching
http://www.lrr.in.tum.de/~stodden         mailto:stodden@cs.tum.edu
PGP Fingerprint: F5A4 1575 4C56 E26A 0B33  3D80 457E 82AE B0D8 735B

Re: do_iret bug in xen

[Ashish Bijlani]

[-- Attachment #1.1: Type: text/plain, Size: 1354 bytes --]

since "sti" is on, an interrupt can occur which can cause the control to
enter "__runq_tickle", which then invokes "cpumask_raise_softirq(mask,
SCHEDULE_SOFTIRQ)" to send IPI to a processor causing the control to enter
"__enter_scheduler" which changes the current.

On Nov 27, 2007 2:16 PM, Daniel Stodden <stodden@cs.tum.edu> wrote:

>
> On Tue, 2007-11-27 at 13:59 -0500, Ashish Bijlani wrote:
> > "do_iret" (slow iret via hyercall) can introduce a race condition as
> > "current" can change during the execution of the function. all
> > hypercalls run with "sti" on, so an interrupt on a processor causing
> > the control to enter in "__enter_scheduler" after reading current can
> > change the current process on that processor.
>
> follow the code e.g. surounding ret_from_intr. __enter_scheduler is a
> softirq handler. it will only be run upon return to guest context, not
> when returning to an interrupted hypervisor. xen is not preemptible.
>
> regards,
> daniel
>
> --
> Daniel Stodden
> LRR     -      Lehrstuhl für Rechnertechnik und Rechnerorganisation
> Institut für Informatik der TU München             D-85748 Garching
> http://www.lrr.in.tum.de/~stodden <http://www.lrr.in.tum.de/%7Estodden>
>       mailto:stodden@cs.tum.edu
> PGP Fingerprint: F5A4 1575 4C56 E26A 0B33  3D80 457E 82AE B0D8 735B
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 1894 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: do_iret bug in xen

[Andres Lagar-Cavilla]

raise_softirq is not the same as actually doing it. Raise only sets a 
bit indicating pending softirq. Before returning to the guest, the bin 
triggers actual execution of the softirq

Andres

    since "sti" is on, an interrupt can occur which can cause the
    control to enter "__runq_tickle", which then invokes
    "cpumask_raise_softirq(mask, SCHEDULE_SOFTIRQ)" to send IPI to a
    processor causing the control to enter "__enter_scheduler" which
    changes the current. 

Re: do_iret bug in xen

[Ashish Bijlani]

[-- Attachment #1.1: Type: text/plain, Size: 699 bytes --]

"cpumask_raise_softirq(mask, SCHEDULE_SOFTIRQ)" sends an IPI to the target
processor, this can cause current to change.

On Nov 27, 2007 4:50 PM, Andres Lagar-Cavilla <andreslc@cs.toronto.edu>
wrote:

> raise_softirq is not the same as actually doing it. Raise only sets a
> bit indicating pending softirq. Before returning to the guest, the bin
> triggers actual execution of the softirq
>
> Andres
>
>    since "sti" is on, an interrupt can occur which can cause the
>    control to enter "__runq_tickle", which then invokes
>    "cpumask_raise_softirq(mask, SCHEDULE_SOFTIRQ)" to send IPI to a
>    processor causing the control to enter "__enter_scheduler" which
>    changes the current.
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 1137 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: do_iret bug in xen

[Ian Campbell]

On Tue, 2007-11-27 at 17:41 -0500, Ashish Bijlani wrote:
> "cpumask_raise_softirq(mask, SCHEDULE_SOFTIRQ)" sends an IPI to the
> target processor, this can cause current to change.

No it can't.

The IPI causes smp_event_check_interrupt() to be called which just ACKs
the IPI and returns via ret_from_intr (in entry.S). It is only if a
guest was interrupted that we go down the test_all_events path which
processes softirqs. If it was Xen which was interrupted then we go to
restore_all_xen which just returns to Xen. In this case Xen will
eventually return to the guest and take the test_all_events path and
process the softirq.

Ian.

Re: do_iret bug in xen

[Ashish Bijlani]

[-- Attachment #1.1: Type: text/plain, Size: 1234 bytes --]

yeah but the do_iret function is done on behalf of a guest, therefore
do_iret function forces user cs and user ss

code excerpt
"
    regs->rip    = iret_saved.rip;
    regs->cs     = iret_saved.cs | 3; /* force guest privilege */
    regs->rflags = (iret_saved.rflags & ~(EF_IOPL|EF_VM)) | EF_IE;
    regs->rsp    = iret_saved.rsp;
    regs->ss     = iret_saved.ss | 3; /* force guest privilege */
"
this can cause ret_from_intr go to test_all_events and finally go to
__enter_scheduler

-a

On Nov 27, 2007 6:17 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:

>
> On Tue, 2007-11-27 at 17:41 -0500, Ashish Bijlani wrote:
> > "cpumask_raise_softirq(mask, SCHEDULE_SOFTIRQ)" sends an IPI to the
> > target processor, this can cause current to change.
>
> No it can't.
>
> The IPI causes smp_event_check_interrupt() to be called which just ACKs
> the IPI and returns via ret_from_intr (in entry.S). It is only if a
> guest was interrupted that we go down the test_all_events path which
> processes softirqs. If it was Xen which was interrupted then we go to
> restore_all_xen which just returns to Xen. In this case Xen will
> eventually return to the guest and take the test_all_events path and
> process the softirq.
>
> Ian.
>
>

[-- Attachment #1.2: Type: text/html, Size: 1778 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: do_iret bug in xen

[Daniel Stodden]

On Tue, 2007-11-27 at 18:30 -0500, Ashish Bijlani wrote:
> yeah but the do_iret function is done on behalf of a guest, therefore
> do_iret function forces user cs and user ss
> 
> code excerpt 
> "
>     regs->rip    = iret_saved.rip;
>     regs->cs     = iret_saved.cs | 3; /* force guest privilege */ 
>     regs->rflags = (iret_saved.rflags & ~(EF_IOPL|EF_VM)) | EF_IE;
>     regs->rsp    = iret_saved.rsp;
>     regs->ss     = iret_saved.ss | 3; /* force guest privilege */
> "
> this can cause ret_from_intr go to test_all_events and finally go to
> __enter_scheduler 

that's the guest context saved in _memory_ (xen stack) which gets
modified -- user or kernel. that's where what it ultimately returns _to_
upon return _from_ do_iret. with an interrupted do_iret, the switch in
ret_from_intr would be yet another stack frame above, and that would be
xen being interrupted.

regards,
daniel
 
-- 
Daniel Stodden
LRR     -      Lehrstuhl für Rechnertechnik und Rechnerorganisation
Institut für Informatik der TU München             D-85748 Garching
http://www.lrr.in.tum.de/~stodden         mailto:stodden@cs.tum.edu
PGP Fingerprint: F5A4 1575 4C56 E26A 0B33  3D80 457E 82AE B0D8 735B

Re: do_iret bug in xen

[Ashish Bijlani]

[-- Attachment #1.1: Type: text/plain, Size: 1663 bytes --]

current is extracted from per processor stack area. so are the registers.
right? do_iret gets current and regs from the processor's stack area. so
does ret_from_intr. so they both point to a fixed per processor stack area.
there are not _different_ stack frames.

On Nov 27, 2007 7:51 PM, Daniel Stodden < stodden@cs.tum.edu> wrote:

>
> On Tue, 2007-11-27 at 18:30 -0500, Ashish Bijlani wrote:
> > yeah but the do_iret function is done on behalf of a guest, therefore
> > do_iret function forces user cs and user ss
> >
> > code excerpt
> > "
> >     regs->rip    = iret_saved.rip;
> >     regs->cs     = iret_saved.cs | 3; /* force guest privilege */
> >     regs->rflags = (iret_saved.rflags & ~(EF_IOPL|EF_VM)) | EF_IE;
> >     regs->rsp    = iret_saved.rsp;
> >     regs->ss     = iret_saved.ss | 3; /* force guest privilege */
> > "
> > this can cause ret_from_intr go to test_all_events and finally go to
> > __enter_scheduler
>
> that's the guest context saved in _memory_ (xen stack) which gets
> modified -- user or kernel. that's where what it ultimately returns _to_
> upon return _from_ do_iret. with an interrupted do_iret, the switch in
> ret_from_intr would be yet another stack frame above, and that would be
> xen being interrupted.
>
> regards,
> daniel
>
> --
> Daniel Stodden
> LRR     -      Lehrstuhl für Rechnertechnik und Rechnerorganisation
> Institut für Informatik der TU München             D-85748 Garching
> http://www.lrr.in.tum.de/~stodden <http://www.lrr.in.tum.de/%7Estodden>
>       mailto:stodden@cs.tum.edu
> PGP Fingerprint: F5A4 1575 4C56 E26A 0B33  3D80 457E 82AE B0D8 735B
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 2308 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: do_iret bug in xen

[Daniel Stodden]

On Tue, 2007-11-27 at 20:15 -0500, Ashish Bijlani wrote:
> current is extracted from per processor stack area. so are the
> registers. right? 

yes.

> do_iret gets current and regs from the processor's stack area. so does
> ret_from_intr. 

yes. but the latter from a different stack *frame* in the case you're
concerned about. same stack, though.

> so they both point to a fixed per processor stack area. there are not
> _different_ stack frames. 

i see what you mean. let's look at the stack when an IPI occurs to
trigger a context switch.

--snip--

guest calls __HYPERVISOR_iret (saves guest cs)
	
	do_iret tinkers with guest_cpu_user_regs
	
	IPI caught (saves xen cs)

		ret_from_intr tests xen (!!) cs, not guest cs
	
	do_iret continues and finishes. same current.
	
	test_all_events calls do_softirq. schedules. new current.

return to new current.

--snap--

UREGS_cs in entry.S refers to the interrupted context, not the saved
guest context. this may be the same: the IPI would have mattered if it
interrupted the guest. it did not. so the softirq will only be run
before returning to the guest, not in between.

regards,
daniel

-- 
Daniel Stodden
LRR     -      Lehrstuhl für Rechnertechnik und Rechnerorganisation
Institut für Informatik der TU München             D-85748 Garching
http://www.lrr.in.tum.de/~stodden         mailto:stodden@cs.tum.edu
PGP Fingerprint: F5A4 1575 4C56 E26A 0B33  3D80 457E 82AE B0D8 735B