Re: [Xen-devel] [BUGFIX][PATCH 4/4] XEN_DOMCTL_gdbsx_guestmemio: always do the copyback.

[Don Slutz <dslutz@xxxxxxxxxxx>]

On 01/07/14 18:01, Mukesh Rathor wrote:
> On Tue, 07 Jan 2014 11:24:15 -0500
> Don Slutz <dslutz@xxxxxxxxxxx> wrote:
>
> > On 01/07/14 05:02, Ian Campbell wrote:
> > > On Tue, 2014-01-07 at 10:00 +0000, Ian Campbell wrote:
> > > > On Mon, 2014-01-06 at 17:53 -0800, Mukesh Rathor wrote:
> > > > > On Sat,  4 Jan 2014 12:52:16 -0500
> > > > > Don Slutz <dslutz@xxxxxxxxxxx> wrote:
> > > > >
> > > > > > The gdbsx code expects that domctl->u.gdbsx_guest_memio.remain is
> > > > > > returned.
> .....
>
> > I had assumed that this patch (which is not need to "fix" the bugs I
> > found), was to be dropped in v2.  However I will agree that currently
> > there is no way to know about partial success.  The untested change:
> >
> > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> > index ef6c140..0add07e 100644
> > --- a/xen/arch/x86/domctl.c
> > +++ b/xen/arch/x86/domctl.c
> > @@ -43,7 +43,7 @@ static int gdbsx_guest_mem_io(
> >        iop->remain = dbg_rw_mem(
> >            (dbgva_t)iop->gva, (dbgbyte_t *)l_uva, iop->len, domid,
> >            iop->gwr, iop->pgd3val);
> > -    return (iop->remain ? -EFAULT : 0);
> > +    return 0;
> >    }
> >
> >    long arch_do_domctl(
> >
> >
> > Would appear to allow partial success to be reported and also meet
> > with remain not to be looked at with an error.
> No, the partial success is relevant in other cases, like EAGAIN,
> but not EFAULT. If we make it pre-emptible in future to return
> EAGAIN, we'd need to make sure remain was honored. Again, think of it
> this way, if the first copyin failed, remain would have not been
> initialized.
>
> So, because now the only cause of unfinished copy from dbg_rw_mem is
> EFAULT, we should leave above xen code alone, and just change gdbsx.
>
> thanks
> mukesh
Since it did not look like we would get to an agreement soon on this, I have 
sent out v2 series.

Not at all clear this patch should be in 4.4.0 (domctl api change, high risk, 
no clear use case (as far as I know gdb does not do anything with partial 
success)).

On this topic; you seem to be overlooking the page crossing case.

Using the info that page 1f is good and 20 is bad, a domctl request for 1ffff 
for 2 bytes would call on dbg_rw_mem(), dbg_rw_guest_mem() which calculate 
pagecnt == 1, get a valid mfn and return that byte. The 2nd time pagecnt is 
also 1, but we get INVALID_MFN, so dbg_rw_guest_mem(0 returns 1.  dbg_rw_mem(0 
also returns 1. gdbsx_guest_mem_io() returns -EFAULT so no copyback.

At this point of the 2 requested byte, 1 byte is valid and 1 is not.  Since copyback is 
not done, remain is 0.  So the caller get the error and does not have this "partial 
success" information.

The 1st version of this patch I proposed, the copyback is done, so the caller 
gets remain == 1 and the valid byte and an error.

The 2nd version of this patch (which has now been tested) sets remian == 1, the 
valid byte, and no error.

Hope this helps.

   -Don Slutz




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel