x86/mem-sharing: short-circuit p2m_is_shared() when MEM_SHARING=n

[PATCH] x86/mem-sharing: short-circuit p2m_is_shared() when MEM_SHARING=n

Posted by Jan Beulich 7 months ago

Some of the uses of dom_cow aren't easily DCE-able (without extra
#ifdef-ary), and hence it being constantly NULL when MEM_SHARING=n
misguides Coverity into thinking that there may be a NULL deref in

        if ( p2m_is_shared(t) )
            d = dom_cow;

        if ( get_page(page, d) )
            return page;

(in get_page_from_mfn_and_type()). Help the situation by making
p2m_is_shared() be compile-time false when MEM_SHARING=n, thus also
permitting the compiler to DCE some other code.

Note that p2m_is_sharable() isn't used outside of mem_sharing.c, and
hence P2M_SHARABLE_TYPES can simply be left undefined when
MEM_SHARING=n.

Coverity ID: 1645573
Fixes: 79d91e178a1a ("dom_cow is needed for mem-sharing only")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Might be nice to also eliminate p2m_ram_shared (and for MEM_PAGING=n
also the three paging types) entirely from such builds, to eliminate the
risk of accidental use. Yet that would apparently also come at the price
of more #ifdef-ary. Opinions?

--- a/xen/arch/x86/include/asm/p2m.h
+++ b/xen/arch/x86/include/asm/p2m.h
@@ -136,11 +136,16 @@ typedef unsigned int p2m_query_t;
 #endif
 
 /* Shared types */
+#ifdef CONFIG_MEM_SHARING
 /* XXX: Sharable types could include p2m_ram_ro too, but we would need to
  * reinit the type correctly after fault */
 #define P2M_SHARABLE_TYPES (p2m_to_mask(p2m_ram_rw) \
                             | p2m_to_mask(p2m_ram_logdirty) )
 #define P2M_SHARED_TYPES   (p2m_to_mask(p2m_ram_shared))
+#else
+/* P2M_SHARABLE_TYPES deliberately not provided. */
+#define P2M_SHARED_TYPES 0
+#endif
 
 /* Types established/cleaned up via special accessors. */
 #define P2M_SPECIAL_TYPES (P2M_GRANT_TYPES | \

Re: [PATCH] x86/mem-sharing: short-circuit p2m_is_shared() when MEM_SHARING=n

Posted by Tamas K Lengyel 6 months, 3 weeks ago

On Thu, Apr 3, 2025 at 4:42 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> Some of the uses of dom_cow aren't easily DCE-able (without extra
> #ifdef-ary), and hence it being constantly NULL when MEM_SHARING=n
> misguides Coverity into thinking that there may be a NULL deref in
>
>         if ( p2m_is_shared(t) )
>             d = dom_cow;
>
>         if ( get_page(page, d) )
>             return page;
>
> (in get_page_from_mfn_and_type()). Help the situation by making
> p2m_is_shared() be compile-time false when MEM_SHARING=n, thus also
> permitting the compiler to DCE some other code.
>
> Note that p2m_is_sharable() isn't used outside of mem_sharing.c, and
> hence P2M_SHARABLE_TYPES can simply be left undefined when
> MEM_SHARING=n.
>
> Coverity ID: 1645573
> Fixes: 79d91e178a1a ("dom_cow is needed for mem-sharing only")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Tamas K Lengyel <tamas@tklengyel.com>

> ---
> Might be nice to also eliminate p2m_ram_shared (and for MEM_PAGING=n
> also the three paging types) entirely from such builds, to eliminate the
> risk of accidental use. Yet that would apparently also come at the price
> of more #ifdef-ary. Opinions?

I don't think the risk of accidental use is a concern. I wouldn't
touch them unless they lead to similar confusion with coverity or some
other tool.

Cheers,
Tamas

Re: [PATCH] x86/mem-sharing: short-circuit p2m_is_shared() when MEM_SHARING=n

Posted by Andrew Cooper 6 months, 3 weeks ago

On 03/04/2025 9:41 am, Jan Beulich wrote:
> Some of the uses of dom_cow aren't easily DCE-able (without extra
> #ifdef-ary), and hence it being constantly NULL when MEM_SHARING=n
> misguides Coverity into thinking that there may be a NULL deref in
>
>         if ( p2m_is_shared(t) )
>             d = dom_cow;
>
>         if ( get_page(page, d) )
>             return page;
>
> (in get_page_from_mfn_and_type()). Help the situation by making
> p2m_is_shared() be compile-time false when MEM_SHARING=n, thus also
> permitting the compiler to DCE some other code.
>
> Note that p2m_is_sharable() isn't used outside of mem_sharing.c, and
> hence P2M_SHARABLE_TYPES can simply be left undefined when
> MEM_SHARING=n.
>
> Coverity ID: 1645573
> Fixes: 79d91e178a1a ("dom_cow is needed for mem-sharing only")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

We'll be swapping this for a different issue, but least "logical and
with 0" is easier to filter.

> ---
> Might be nice to also eliminate p2m_ram_shared (and for MEM_PAGING=n
> also the three paging types) entirely from such builds, to eliminate the
> risk of accidental use. Yet that would apparently also come at the price
> of more #ifdef-ary. Opinions?

Hard to say without seeing how it looks.  I wouldn't worry for now.

>
> --- a/xen/arch/x86/include/asm/p2m.h
> +++ b/xen/arch/x86/include/asm/p2m.h
> @@ -136,11 +136,16 @@ typedef unsigned int p2m_query_t;
>  #endif
>  
>  /* Shared types */
> +#ifdef CONFIG_MEM_SHARING
>  /* XXX: Sharable types could include p2m_ram_ro too, but we would need to
>   * reinit the type correctly after fault */
>  #define P2M_SHARABLE_TYPES (p2m_to_mask(p2m_ram_rw) \
>                              | p2m_to_mask(p2m_ram_logdirty) )
>  #define P2M_SHARED_TYPES   (p2m_to_mask(p2m_ram_shared))
> +#else
> +/* P2M_SHARABLE_TYPES deliberately not provided. */
> +#define P2M_SHARED_TYPES 0

You need P2M_SHARABLE_TYPES too, or p2m_is_sharable() will start
becoming a syntax error.

~Andrew

Re: [PATCH] x86/mem-sharing: short-circuit p2m_is_shared() when MEM_SHARING=n

Posted by Jan Beulich 6 months, 3 weeks ago

On 07.04.2025 16:50, Andrew Cooper wrote:
> On 03/04/2025 9:41 am, Jan Beulich wrote:
>> Some of the uses of dom_cow aren't easily DCE-able (without extra
>> #ifdef-ary), and hence it being constantly NULL when MEM_SHARING=n
>> misguides Coverity into thinking that there may be a NULL deref in
>>
>>         if ( p2m_is_shared(t) )
>>             d = dom_cow;
>>
>>         if ( get_page(page, d) )
>>             return page;
>>
>> (in get_page_from_mfn_and_type()). Help the situation by making
>> p2m_is_shared() be compile-time false when MEM_SHARING=n, thus also
>> permitting the compiler to DCE some other code.
>>
>> Note that p2m_is_sharable() isn't used outside of mem_sharing.c, and
>> hence P2M_SHARABLE_TYPES can simply be left undefined when
>> MEM_SHARING=n.

Note this for ...

>> --- a/xen/arch/x86/include/asm/p2m.h
>> +++ b/xen/arch/x86/include/asm/p2m.h
>> @@ -136,11 +136,16 @@ typedef unsigned int p2m_query_t;
>>  #endif
>>  
>>  /* Shared types */
>> +#ifdef CONFIG_MEM_SHARING
>>  /* XXX: Sharable types could include p2m_ram_ro too, but we would need to
>>   * reinit the type correctly after fault */
>>  #define P2M_SHARABLE_TYPES (p2m_to_mask(p2m_ram_rw) \
>>                              | p2m_to_mask(p2m_ram_logdirty) )
>>  #define P2M_SHARED_TYPES   (p2m_to_mask(p2m_ram_shared))
>> +#else
>> +/* P2M_SHARABLE_TYPES deliberately not provided. */
>> +#define P2M_SHARED_TYPES 0
> 
> You need P2M_SHARABLE_TYPES too, or p2m_is_sharable() will start
> becoming a syntax error.

... what you're saying here. If you did take that into consideration, then
please be more specific about the concern(s) you have.

Jan