| 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
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
--- 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?