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

[Don Slutz <dslutz@verizon.com>]

On 01/07/14 18:01, Mukesh Rathor wrote:
> On Tue, 07 Jan 2014 11:24:15 -0500
> Don Slutz <dslutz@verizon.com> 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@verizon.com> 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