| Message ID | 5a261173-d225-44fc-9078-4030ba11cfd8@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/mem-sharing: short-circuit p2m_is_shared() when MEM_SHARING=n | expand |
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
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
--- 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 | \
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?