| Message ID | bddbec0e-acc2-03f9-fe4c-167fa5ac0ea1@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: further time/APIC improvements (and more) | expand |
On Thu, Mar 31, 2022 at 11:31:38AM +0200, Jan Beulich wrote: > Go a step further than bed9ae54df44 ("x86/time: switch platform timer > hooks to altcall") did and eliminate the "real" read_tsc() altogether: > It's not used except in pointer comparisons, and hence it looks overall > more safe to simply poison plt_tsc's read_counter hook. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > I wasn't really sure whether it would be better to use simply void * for > the type of the expression, resulting in an undesirable data -> function > pointer conversion, but making it impossible to mistakenly try and call > the (fake) function directly. I think it's slightly better to avoid being able to call the function, hence using void * would be my preference. What's wrong with the data -> function pointer conversion for the comparisons? > --- > v2: Comment wording. > > --- a/xen/arch/x86/time.c > +++ b/xen/arch/x86/time.c > @@ -607,10 +607,12 @@ static s64 __init cf_check init_tsc(stru > return ret; > } > > -static uint64_t __init cf_check read_tsc(void) > -{ > - return rdtsc_ordered(); > -} > +/* > + * plt_tsc's read_counter hook is not (and should not be) invoked via the > + * struct field. To avoid carrying an unused, indirectly reachable function, > + * poison the field with an easily identifiable non-canonical pointer. > + */ > +#define read_tsc ((uint64_t(*)(void))0x75C75C75C75C75C0ul) Instead of naming this like a suitable function, I would rather use READ_TSC_PTR_POISON or some such. Thanks, Roger.
On 04.04.2022 15:22, Roger Pau Monné wrote: > On Thu, Mar 31, 2022 at 11:31:38AM +0200, Jan Beulich wrote: >> Go a step further than bed9ae54df44 ("x86/time: switch platform timer >> hooks to altcall") did and eliminate the "real" read_tsc() altogether: >> It's not used except in pointer comparisons, and hence it looks overall >> more safe to simply poison plt_tsc's read_counter hook. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> I wasn't really sure whether it would be better to use simply void * for >> the type of the expression, resulting in an undesirable data -> function >> pointer conversion, but making it impossible to mistakenly try and call >> the (fake) function directly. > > I think it's slightly better to avoid being able to call the function, > hence using void * would be my preference. What's wrong with the data > -> function pointer conversion for the comparisons? There's no data -> function pointer conversion for the comparisons; the situation there is even less pleasant. What I referred to was actually the initializer, where there would be a data -> function pointer conversion if I used void *. >> --- >> v2: Comment wording. >> >> --- a/xen/arch/x86/time.c >> +++ b/xen/arch/x86/time.c >> @@ -607,10 +607,12 @@ static s64 __init cf_check init_tsc(stru >> return ret; >> } >> >> -static uint64_t __init cf_check read_tsc(void) >> -{ >> - return rdtsc_ordered(); >> -} >> +/* >> + * plt_tsc's read_counter hook is not (and should not be) invoked via the >> + * struct field. To avoid carrying an unused, indirectly reachable function, >> + * poison the field with an easily identifiable non-canonical pointer. >> + */ >> +#define read_tsc ((uint64_t(*)(void))0x75C75C75C75C75C0ul) > > Instead of naming this like a suitable function, I would rather use > READ_TSC_PTR_POISON or some such. I'll be happy to name it something like this; the primary thing to settle on is the type to use. Jan
On Mon, Apr 04, 2022 at 05:33:04PM +0200, Jan Beulich wrote: > On 04.04.2022 15:22, Roger Pau Monné wrote: > > On Thu, Mar 31, 2022 at 11:31:38AM +0200, Jan Beulich wrote: > >> Go a step further than bed9ae54df44 ("x86/time: switch platform timer > >> hooks to altcall") did and eliminate the "real" read_tsc() altogether: > >> It's not used except in pointer comparisons, and hence it looks overall > >> more safe to simply poison plt_tsc's read_counter hook. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> --- > >> I wasn't really sure whether it would be better to use simply void * for > >> the type of the expression, resulting in an undesirable data -> function > >> pointer conversion, but making it impossible to mistakenly try and call > >> the (fake) function directly. > > > > I think it's slightly better to avoid being able to call the function, > > hence using void * would be my preference. What's wrong with the data > > -> function pointer conversion for the comparisons? > > There's no data -> function pointer conversion for the comparisons; the > situation there is even less pleasant. What I referred to was actually > the initializer, where there would be a data -> function pointer > conversion if I used void *. I see, there are architectures with different sizes for function and data pointers. It's also not clear all compilers will be happy with the conversion. > >> --- > >> v2: Comment wording. > >> > >> --- a/xen/arch/x86/time.c > >> +++ b/xen/arch/x86/time.c > >> @@ -607,10 +607,12 @@ static s64 __init cf_check init_tsc(stru > >> return ret; > >> } > >> > >> -static uint64_t __init cf_check read_tsc(void) > >> -{ > >> - return rdtsc_ordered(); > >> -} > >> +/* > >> + * plt_tsc's read_counter hook is not (and should not be) invoked via the > >> + * struct field. To avoid carrying an unused, indirectly reachable function, > >> + * poison the field with an easily identifiable non-canonical pointer. > >> + */ > >> +#define read_tsc ((uint64_t(*)(void))0x75C75C75C75C75C0ul) > > > > Instead of naming this like a suitable function, I would rather use > > READ_TSC_PTR_POISON or some such. > > I'll be happy to name it something like this; the primary thing to > settle on is the type to use. I think it's safer to use a function pointer type like you currently have from a correctness PoV, but in order to prevent stray calls to read_tsc() I would rename to READ_TSC_PTR_POISON. This was already static, so I guess it's hard anyway for any of such direct calls to appear without us realizing. With that: Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
--- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -607,10 +607,12 @@ static s64 __init cf_check init_tsc(stru return ret; } -static uint64_t __init cf_check read_tsc(void) -{ - return rdtsc_ordered(); -} +/* + * plt_tsc's read_counter hook is not (and should not be) invoked via the + * struct field. To avoid carrying an unused, indirectly reachable function, + * poison the field with an easily identifiable non-canonical pointer. + */ +#define read_tsc ((uint64_t(*)(void))0x75C75C75C75C75C0ul) static struct platform_timesource __initdata_cf_clobber plt_tsc = {
Go a step further than bed9ae54df44 ("x86/time: switch platform timer hooks to altcall") did and eliminate the "real" read_tsc() altogether: It's not used except in pointer comparisons, and hence it looks overall more safe to simply poison plt_tsc's read_counter hook. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- I wasn't really sure whether it would be better to use simply void * for the type of the expression, resulting in an undesirable data -> function pointer conversion, but making it impossible to mistakenly try and call the (fake) function directly. --- v2: Comment wording.