Re: [BUGFIX][PATCH 1/4] dbg_rw_guest_mem: need to call put_gfn in error path.

[Don Slutz <dslutz@verizon.com>]

On 01/06/14 08:13, Andrew Cooper wrote:
> On 06/01/14 12:43, Don Slutz wrote:
>> On 01/05/14 18:09, Andrew Cooper wrote:
>>> On 04/01/2014 17:52, Don Slutz wrote:
>>>> Using a 1G hvm domU (in grub) and gdbsx:
>>>>
>>>> (gdb) set arch i8086
>>>> warning: A handler for the OS ABI "GNU/Linux" is not built into this
>>>> configuration
>>>> of GDB.  Attempting to continue with the default i8086 settings.
>>>>
>>>> The target architecture is assumed to be i8086
>>>> (gdb) target remote localhost:9999
>>>> Remote debugging using localhost:9999
>>>> Remote debugging from host 127.0.0.1
>>>> 0x0000d475 in ?? ()
>>>> (gdb) x/1xh 0x6ae9168b
>>>>
>>>> Will reproduce this bug.
>>>>
>>>> With a debug=y build you will get:
>>>>
>>>> Assertion '!preempt_count()' failed at preempt.c:37
>>>>
>>>> For a debug=n build you will get a dom0 VCPU hung (at some point) in:
>>>>
>>>>            [ffff82c4c0126eec] _write_lock+0x3c/0x50
>>>>             ffff82c4c01e43a0  __get_gfn_type_access+0x150/0x230
>>>>             ffff82c4c0158885  dbg_rw_mem+0x115/0x360
>>>>             ffff82c4c0158fc8  arch_do_domctl+0x4b8/0x22f0
>>>>             ffff82c4c01709ed  get_page+0x2d/0x100
>>>>             ffff82c4c01031aa  do_domctl+0x2ba/0x11e0
>>>>             ffff82c4c0179662  do_mmuext_op+0x8d2/0x1b20
>>>>             ffff82c4c0183598  __update_vcpu_system_time+0x288/0x340
>>>>             ffff82c4c015c719  continue_nonidle_domain+0x9/0x30
>>>>             ffff82c4c012938b  add_entry+0x4b/0xb0
>>>>             ffff82c4c02223f9  syscall_enter+0xa9/0xae
>>>>
>>>> And gdb output:
>>>>
>>>> (gdb) x/1xh 0x6ae9168b
>>>> 0x6ae9168b:     0x3024
>>>> (gdb) x/1xh 0x6ae9168b
>>>> 0x6ae9168b:     Ignoring packet error, continuing...
>>>> Reply contains invalid hex digit 116
>>>>
>>>> The 1st one worked because the p2m.lock is recursive and the PCPU
>>>> had not yet changed.
>>>>
>>>> crash reports (for example):
>>>>
>>>> crash> mm_rwlock_t 0xffff83083f913010
>>>> struct mm_rwlock_t {
>>>>     lock = {
>>>>       raw = {
>>>>         lock = 2147483647
>>>>       },
>>>>       debug = {<No data fields>}
>>>>     },
>>>>     unlock_level = 0,
>>>>     recurse_count = 1,
>>>>     locker = 1,
>>>>     locker_function = 0xffff82c4c022c640 <__func__.13514>
>>>> "__get_gfn_type_access"
>>>> }
>>>>
>>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>>>> ---
>>>>    xen/arch/x86/debug.c | 5 ++++-
>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
>>>> index 3e21ca8..eceb805 100644
>>>> --- a/xen/arch/x86/debug.c
>>>> +++ b/xen/arch/x86/debug.c
>>>> @@ -161,8 +161,11 @@ dbg_rw_guest_mem(dbgva_t addr, dbgbyte_t *buf,
>>>> int len, struct domain *dp,
>>>>            mfn = (has_hvm_container_domain(dp)
>>>>                   ? dbg_hvm_va2mfn(addr, dp, toaddr, &gfn)
>>>>                   : dbg_pv_va2mfn(addr, dp, pgd3));
>>>> -        if ( mfn == INVALID_MFN )
>>>> +        if ( mfn == INVALID_MFN ) {
>>> Xen coding style would have this brace on the next line.
>> Will fix.
>>
>>
>>> Content-wise, I think it would be better to fix up the error path in
>>> dbg_hvm_va2mfn() so it doesn't exit with INVALID_MFN having taken a
>>> reference on the gfn.
>> That would only fix "p2m_is_readonly(gfntype) and writing memory" case
>> (see cover letter). To fix "the requested vaddr does not exist" case,
>> I would need to add a check for INVALID_MFN in dbg_hvm_va2mfn() also.
>> As It currently stands it is a smaller fix.
>>
>>     -Don Slutz
> Size really doesn't matter.
>
> What does matter is that the caller of dbg_hvm_va2mfn() should not have
> to cleanup a reference taken when it returns an error.
>
> Anyway, "the requested vaddr does not exist" is covered by
> paging_gva_to_gfn(), which will exit before taking the ref on the gfn.

This is a false statement.  A valid GFN is returned in this case. The lock is taken by get_gfn().

>
> The following (completely untested) patch ought to do:
>
> diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
> index 3e21ca8..3372adb 100644
> --- a/xen/arch/x86/debug.c
> +++ b/xen/arch/x86/debug.c
> @@ -63,10 +63,17 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int
> toaddr,
>       if ( p2m_is_readonly(gfntype) && toaddr )
>       {
>           DBGP2("kdb:p2m_is_readonly: gfntype:%x\n", gfntype);
> -        return INVALID_MFN;
> +        mfn = INVALID_MFN;
>       }
>   
>       DBGP2("X: vaddr:%lx domid:%d mfn:%lx\n", vaddr, dp->domain_id, mfn);
> +
> +    if ( mfn == INVALID_MFN )
> +    {
> +        put_gfn(dp, *gfn);
> +        *gfn = INVALID_GFN;
> +    }
> +
>       return mfn;
>   }
>

However this patch (which I have now tested) works fine.  I will post it soon as part of v2 of this patch set.

    -Don Slutz