On Tue, Apr 23, 2024 at 03:51:31PM +0200, Jan Beulich wrote:
> On 23.04.2024 15:12, Roger Pau Monne wrote:
> > Ensure the entries of a payload exception table only apply to text regions
> > in
> > the payload itself. Since the payload exception table needs to be loaded
> > and
> > active even before a patch is applied (because hooks might already rely on
> > it),
> > make sure the exception table (if any) only contains fixups for the payload
> > text section.
> >
> > Signed-off-by: Roger Pau Monné <roger....@citrix.com>
>
> In principle
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
> Still two comments:
>
> > --- a/xen/arch/x86/extable.c
> > +++ b/xen/arch/x86/extable.c
> > @@ -228,3 +228,21 @@ unsigned long asmlinkage
> > search_pre_exception_table(struct cpu_user_regs *regs)
> > }
> > return fixup;
> > }
> > +
> > +#ifdef CONFIG_LIVEPATCH
> > +bool extable_is_between_bounds(const struct exception_table_entry
> > *ex_start,
>
> s/between/in/ or even s/is_between/in/? "Between", to me at least, reads
> very much like meaning "exclusive at both ends".
Oh, OK, I don't associate any boundary inclusion with 'between' or
'in'. The result is shorter, so I like it.
> > + const struct exception_table_entry *ex_end,
> > + const void *start, const void *end)
> > +{
> > + for ( ; ex_start < ex_end; ex_start++ )
> > + {
> > + const void *addr = (void *)ex_addr(ex_start);
> > + const void *cont = (void *)ex_cont(ex_start);
>
> Might be nicer to use _p() here, or not do the comparisons with pointers, but
> instead with unsigned long-s.
No strong opinion regarding whether to use unsigned longs or pointers.
I've used pointers because I think the function parameters should be
pointers, and that avoided doing a cast in the comparison with
obfuscates it (or introducing yet another local variable).
I can switch to _p(), that's indeed better.
Let me know if you have a strong opinion for using unsigned longs,
otherwise my preference would be to leave it with pointers.
Thanks, Roger.
