| Message ID | 1460018342-5111-1-git-send-email-rcojocaru@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 07.04.16 at 10:39, <rcojocaru@bitdefender.com> wrote: > Theoretically it is possible for mem_access_emulate_each_rep to be > true even when current->arch.vm_event == NULL, so add an extra > check to hvmemul_virtual_to_linear(). Mind saying what those theoretical conditions are when this might happen? > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -514,7 +514,7 @@ static int hvmemul_virtual_to_linear( > * vm_event being triggered for repeated writes to a whole page. > */ > if ( unlikely(current->domain->arch.mem_access_emulate_each_rep) && > - current->arch.vm_event->emulate_flags != 0 ) > + current->arch.vm_event && current->arch.vm_event->emulate_flags != 0 ) That's then the third instance of "current" here - this needs latching into a local variable. Jan
On 04/07/16 20:27, Jan Beulich wrote: >>>> On 07.04.16 at 10:39, <rcojocaru@bitdefender.com> wrote: >> Theoretically it is possible for mem_access_emulate_each_rep to be >> true even when current->arch.vm_event == NULL, so add an extra >> check to hvmemul_virtual_to_linear(). > > Mind saying what those theoretical conditions are when this might > happen? This could happen if someone were to call xc_monitor_emulate_each_rep(), but not xc_monitor_enable() (when current->arch.vm_event gets allocated), or after someone called both, but afterwards called xc_monitor_disable() (when current->arch.vm_event gets freed). >> --- a/xen/arch/x86/hvm/emulate.c >> +++ b/xen/arch/x86/hvm/emulate.c >> @@ -514,7 +514,7 @@ static int hvmemul_virtual_to_linear( >> * vm_event being triggered for repeated writes to a whole page. >> */ >> if ( unlikely(current->domain->arch.mem_access_emulate_each_rep) && >> - current->arch.vm_event->emulate_flags != 0 ) >> + current->arch.vm_event && current->arch.vm_event->emulate_flags != 0 ) > > That's then the third instance of "current" here - this needs > latching into a local variable. No problem. Thanks, Razvan
On 04/07/16 20:54, Razvan Cojocaru wrote: > On 04/07/16 20:27, Jan Beulich wrote: >>>>> On 07.04.16 at 10:39, <rcojocaru@bitdefender.com> wrote: >>> Theoretically it is possible for mem_access_emulate_each_rep to be >>> true even when current->arch.vm_event == NULL, so add an extra >>> check to hvmemul_virtual_to_linear(). >> >> Mind saying what those theoretical conditions are when this might >> happen? > > This could happen if someone were to call xc_monitor_emulate_each_rep(), > but not xc_monitor_enable() (when current->arch.vm_event gets > allocated), or after someone called both, but afterwards called > xc_monitor_disable() (when current->arch.vm_event gets freed). Actually, I need to correct myself here: only the first case applies. I was looking at older Xen source code, but there's a "d->arch.mem_access_emulate_each_rep = 0;" statement in vm_event_cleanup_domain() now, so calling xc_monitor_disable() would not cause a problem. Thanks, Razvan
>>> On 07.04.16 at 19:54, <rcojocaru@bitdefender.com> wrote: > On 04/07/16 20:27, Jan Beulich wrote: >>>>> On 07.04.16 at 10:39, <rcojocaru@bitdefender.com> wrote: >>> Theoretically it is possible for mem_access_emulate_each_rep to be >>> true even when current->arch.vm_event == NULL, so add an extra >>> check to hvmemul_virtual_to_linear(). >> >> Mind saying what those theoretical conditions are when this might >> happen? > > This could happen if someone were to call xc_monitor_emulate_each_rep(), > but not xc_monitor_enable() (when current->arch.vm_event gets > allocated), or after someone called both, but afterwards called > xc_monitor_disable() (when current->arch.vm_event gets freed). Then wouldn't the correct action be to fail xc_monitor_emulate_each_rep() (i.e. whatever hypercall this resolves to) when the monitor is not enabled? (You did already clarify that the other variant isn't applicable)? Jan
On 04/08/16 00:17, Jan Beulich wrote: >>>> On 07.04.16 at 19:54, <rcojocaru@bitdefender.com> wrote: >> On 04/07/16 20:27, Jan Beulich wrote: >>>>>> On 07.04.16 at 10:39, <rcojocaru@bitdefender.com> wrote: >>>> Theoretically it is possible for mem_access_emulate_each_rep to be >>>> true even when current->arch.vm_event == NULL, so add an extra >>>> check to hvmemul_virtual_to_linear(). >>> >>> Mind saying what those theoretical conditions are when this might >>> happen? >> >> This could happen if someone were to call xc_monitor_emulate_each_rep(), >> but not xc_monitor_enable() (when current->arch.vm_event gets >> allocated), or after someone called both, but afterwards called >> xc_monitor_disable() (when current->arch.vm_event gets freed). > > Then wouldn't the correct action be to fail > xc_monitor_emulate_each_rep() (i.e. whatever hypercall this > resolves to) when the monitor is not enabled? (You did already > clarify that the other variant isn't applicable)? Yes, I think that's better. I'll do that instead. Thanks, Razvan
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index f5ab5bc..91413d2 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -514,7 +514,7 @@ static int hvmemul_virtual_to_linear( * vm_event being triggered for repeated writes to a whole page. */ if ( unlikely(current->domain->arch.mem_access_emulate_each_rep) && - current->arch.vm_event->emulate_flags != 0 ) + current->arch.vm_event && current->arch.vm_event->emulate_flags != 0 ) max_reps = 1; /*
Theoretically it is possible for mem_access_emulate_each_rep to be true even when current->arch.vm_event == NULL, so add an extra check to hvmemul_virtual_to_linear(). Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> --- xen/arch/x86/hvm/emulate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)