Re: [Xen-devel] [PATCH] xen/arm: bug report: mfn_valid checking

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-08-23 at 13:29 +0900, Jaeyong Yoo wrote:
> Hello, 
> 
> mfn_valid is comparing the mfn with max_page (i.e., maximum number 
> of pages). When frametable_base_mfn is non-zero, it does not give 
> right results. Should be comparing the mfn starting from the
> frametable_base_mfn. Unless, mfn_valid does not check correctly,
> and may lead to page fault in some places (especially, when
> initializing heap at booting time).
> 
> Jaeyong
> 
> Signed-off-by: Jaeyong Yoo <jaeyong.yoo@xxxxxxxxxxx>
> ---
>  xen/include/asm-arm/mm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index 27284d0..c5b8367 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -189,7 +189,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, 
> size_t len)
>  }
>  
>  #define mfn_valid(mfn)        ({                                             
>  \
> -    unsigned long __m_f_n = (mfn);                                           
>  \
> +    unsigned long __m_f_n = (mfn - frametable_base_mfn);                     
>  \

Does this do the right thing for invalid mfns which are below
frametable_base_mfn? I suppose it will underflow to a value above
max_page, which is wrong (and even if right it would be too subtle!)

>      likely(__m_f_n < max_page);                                              
>  \

max_page is the PFN of the highest RAM address, not its offset into the
frametable, whereas with your change __m_f_n is the offset.

Is the right fix:
    unsigned long __m_f_n = (mfn);
    likely(__m_f_n >= frametable_base_mfn && __m_f_n < max_page)
?

Ian.


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