* [PATCH] x86/shadow: replace stale literal numbers in hash_{vcpu,domain}_foreach()
@ 2021-01-25 11:07 Jan Beulich
2021-01-25 17:58 ` Tim Deegan
2021-01-27 15:13 ` Jan Beulich
0 siblings, 2 replies; 3+ messages in thread
From: Jan Beulich @ 2021-01-25 11:07 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, Tim Deegan, George Dunlap
15 apparently once used to be the last valid type to request a callback
for, and the dimension of the respective array. The arrays meanwhile are
larger than this (in a benign way, i.e. no caller ever sets a mask bit
higher than 15), dimensioned by SH_type_unused. Have the ASSERT()s
follow suit and add build time checks at the call sites.
Also adjust a comment naming the wrong of the two functions.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The ASSERT()s being adjusted look redundant with the BUILD_BUG_ON()s
being added, so I wonder whether dropping them wouldn't be the better
route.
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -1623,6 +1623,9 @@ void shadow_hash_delete(struct domain *d
typedef int (*hash_vcpu_callback_t)(struct vcpu *v, mfn_t smfn, mfn_t other_mfn);
typedef int (*hash_domain_callback_t)(struct domain *d, mfn_t smfn, mfn_t other_mfn);
+#define HASH_CALLBACKS_CHECK(mask) \
+ BUILD_BUG_ON((mask) > (1U << ARRAY_SIZE(callbacks)) - 1)
+
static void hash_vcpu_foreach(struct vcpu *v, unsigned int callback_mask,
const hash_vcpu_callback_t callbacks[],
mfn_t callback_mfn)
@@ -1658,7 +1661,7 @@ static void hash_vcpu_foreach(struct vcp
{
if ( callback_mask & (1 << x->u.sh.type) )
{
- ASSERT(x->u.sh.type <= 15);
+ ASSERT(x->u.sh.type < SH_type_unused);
ASSERT(callbacks[x->u.sh.type] != NULL);
done = callbacks[x->u.sh.type](v, page_to_mfn(x),
callback_mfn);
@@ -1705,7 +1708,7 @@ static void hash_domain_foreach(struct d
{
if ( callback_mask & (1 << x->u.sh.type) )
{
- ASSERT(x->u.sh.type <= 15);
+ ASSERT(x->u.sh.type < SH_type_unused);
ASSERT(callbacks[x->u.sh.type] != NULL);
done = callbacks[x->u.sh.type](d, page_to_mfn(x),
callback_mfn);
@@ -2009,6 +2012,7 @@ int sh_remove_write_access(struct domain
perfc_incr(shadow_writeable_bf_1);
else
perfc_incr(shadow_writeable_bf);
+ HASH_CALLBACKS_CHECK(callback_mask);
hash_domain_foreach(d, callback_mask, callbacks, gmfn);
/* If that didn't catch the mapping, then there's some non-pagetable
@@ -2080,6 +2084,7 @@ int sh_remove_all_mappings(struct domain
/* Brute-force search of all the shadows, by walking the hash */
perfc_incr(shadow_mappings_bf);
+ HASH_CALLBACKS_CHECK(callback_mask);
hash_domain_foreach(d, callback_mask, callbacks, gmfn);
/* If that didn't catch the mapping, something is very wrong */
@@ -2246,10 +2251,12 @@ void sh_remove_shadows(struct domain *d,
/* Search for this shadow in all appropriate shadows */
perfc_incr(shadow_unshadow);
- /* Lower-level shadows need to be excised from upper-level shadows.
- * This call to hash_vcpu_foreach() looks dangerous but is in fact OK: each
+ /*
+ * Lower-level shadows need to be excised from upper-level shadows. This
+ * call to hash_domain_foreach() looks dangerous but is in fact OK: each
* call will remove at most one shadow, and terminate immediately when
- * it does remove it, so we never walk the hash after doing a deletion. */
+ * it does remove it, so we never walk the hash after doing a deletion.
+ */
#define DO_UNSHADOW(_type) do { \
t = (_type); \
if( !(pg->count_info & PGC_page_table) \
@@ -2270,6 +2277,7 @@ void sh_remove_shadows(struct domain *d,
if( !fast \
&& (pg->count_info & PGC_page_table) \
&& (pg->shadow_flags & (1 << t)) ) \
+ HASH_CALLBACKS_CHECK(SHF_page_type_mask); \
hash_domain_foreach(d, masks[t], callbacks, smfn); \
} while (0)
@@ -2370,6 +2378,7 @@ void sh_reset_l3_up_pointers(struct vcpu
};
static const unsigned int callback_mask = SHF_L3_64;
+ HASH_CALLBACKS_CHECK(callback_mask);
hash_vcpu_foreach(v, callback_mask, callbacks, INVALID_MFN);
}
@@ -3420,6 +3429,7 @@ void shadow_audit_tables(struct vcpu *v)
}
}
+ HASH_CALLBACKS_CHECK(SHF_page_type_mask);
hash_vcpu_foreach(v, mask, callbacks, INVALID_MFN);
}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] x86/shadow: replace stale literal numbers in hash_{vcpu,domain}_foreach()
2021-01-25 11:07 [PATCH] x86/shadow: replace stale literal numbers in hash_{vcpu,domain}_foreach() Jan Beulich
@ 2021-01-25 17:58 ` Tim Deegan
2021-01-27 15:13 ` Jan Beulich
1 sibling, 0 replies; 3+ messages in thread
From: Tim Deegan @ 2021-01-25 17:58 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap
At 12:07 +0100 on 25 Jan (1611576438), Jan Beulich wrote:
> 15 apparently once used to be the last valid type to request a callback
> for, and the dimension of the respective array. The arrays meanwhile are
> larger than this (in a benign way, i.e. no caller ever sets a mask bit
> higher than 15), dimensioned by SH_type_unused. Have the ASSERT()s
> follow suit and add build time checks at the call sites.
>
> Also adjust a comment naming the wrong of the two functions.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Tim Deegan <tim@xen.org>
> ---
> The ASSERT()s being adjusted look redundant with the BUILD_BUG_ON()s
> being added, so I wonder whether dropping them wouldn't be the better
> route.
I'm happy to keep both, as they do slightly different things.
Thanks for fixing this up!
Tim.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] x86/shadow: replace stale literal numbers in hash_{vcpu,domain}_foreach()
2021-01-25 11:07 [PATCH] x86/shadow: replace stale literal numbers in hash_{vcpu,domain}_foreach() Jan Beulich
2021-01-25 17:58 ` Tim Deegan
@ 2021-01-27 15:13 ` Jan Beulich
1 sibling, 0 replies; 3+ messages in thread
From: Jan Beulich @ 2021-01-27 15:13 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, Tim Deegan, George Dunlap
On 25.01.2021 12:07, Jan Beulich wrote:
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -1623,6 +1623,9 @@ void shadow_hash_delete(struct domain *d
> typedef int (*hash_vcpu_callback_t)(struct vcpu *v, mfn_t smfn, mfn_t other_mfn);
> typedef int (*hash_domain_callback_t)(struct domain *d, mfn_t smfn, mfn_t other_mfn);
>
> +#define HASH_CALLBACKS_CHECK(mask) \
> + BUILD_BUG_ON((mask) > (1U << ARRAY_SIZE(callbacks)) - 1)
Sadly at least with Clang5 this doesn't work for some of the uses
further down, e.g. ...
> @@ -2009,6 +2012,7 @@ int sh_remove_write_access(struct domain
> perfc_incr(shadow_writeable_bf_1);
> else
> perfc_incr(shadow_writeable_bf);
> + HASH_CALLBACKS_CHECK(callback_mask);
... this one. I've made it
#ifndef __clang__ /* At least some versions dislike some of the uses. */
#define HASH_CALLBACKS_CHECK(mask) \
BUILD_BUG_ON((mask) > (1U << ARRAY_SIZE(callbacks)) - 1)
#else
#define HASH_CALLBACKS_CHECK(mask) ((void)(mask))
#endif
for the time being - if anyone has any better idea, I'll be
happy to take suggestions.
Jan
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-01-27 15:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-25 11:07 [PATCH] x86/shadow: replace stale literal numbers in hash_{vcpu,domain}_foreach() Jan Beulich
2021-01-25 17:58 ` Tim Deegan
2021-01-27 15:13 ` Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).