Re: [Xen-devel] [PATCH 2/5] xen/alternatives: Plumb a 'live' parameter through apply_alternatives()

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andrew,

On 30/01/18 11:24, Andrew Cooper wrote:
> On 30/01/18 11:05, Julien Grall wrote:
> > Hi Andrew,
> >
> > On 29/01/18 15:38, Andrew Cooper wrote:
> > > On x86, we would like to alter how we patch based on whether there is
> > > any
> > > chance of the code being patched being concurrently executed.
> > >
> > > prepare_payload() passes false (as the livepatch definitely isn't
> > > live at this
> > > point), whereas the boot-time alternatives application passes true.
> > >
> > > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > > ---
> > > CC: Jan Beulich <JBeulich@xxxxxxxx>
> > > CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > CC: Julien Grall <julien.grall@xxxxxxx>
> > > CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > > CC: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> > > ---
> > >    xen/arch/arm/alternative.c        | 10 ++++++----
> > >    xen/arch/x86/alternative.c        |  5 +++--
> > >    xen/common/livepatch.c            |  2 +-
> > >    xen/include/asm-arm/alternative.h |  6 ++++--
> > >    xen/include/asm-x86/alternative.h |  3 ++-
> > >    5 files changed, 16 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> > > index 99112e1..078b259 100644
> > > --- a/xen/arch/arm/alternative.c
> > > +++ b/xen/arch/arm/alternative.c
> > > @@ -98,7 +98,8 @@ static u32 get_alt_insn(const struct alt_instr *alt,
> > >     * The region patched should be read-write to allow
> > > __apply_alternatives
> > >     * to replacing the instructions when necessary.
> > >     */
> > > -static void __apply_alternatives(const struct alt_region *region)
> > > +static void __apply_alternatives(const struct alt_region *region,
> > > +                                 bool live)
> > >    {
> > >        const struct alt_instr *alt;
> > >        const u32 *replptr;
> > > @@ -193,7 +194,7 @@ static int __init
> > > __apply_alternatives_multi_stop(void *unused)
> > >            region.begin = (void *)__alt_instructions - (void *)_start
> > > + xenmap;
> > >            region.end = (void *)__alt_instructions_end - (void
> > > *)_start + xenmap;
> > >    -        __apply_alternatives(&region);
> > > +        __apply_alternatives(&region, true);
> > >              unregister_virtual_region(&patch_region);
> > >    @@ -224,14 +225,15 @@ void __init apply_alternatives_all(void)
> > >    }
> > >      void apply_alternatives(const struct alt_instr *start,
> > > -                        const struct alt_instr *end)
> > > +                        const struct alt_instr *end,
> > > +                        bool live)
> > This function is not able to deal with "live" code, so I think at
> > least need an ASSERT(!live) to prevent mis-usage of the code.
> This passes straight through into __apply_alternatives(), just like
> __apply_alternatives_multi_stop does, and multi_stop is used on live code.
>
> Either both are unsafe (although all evidence to the contrary), or both
> are safe, but I don't think that an assert here is appropriate.
I disagree here. In the commit message you wrote: "On x86, we would like 
to alter how we patch based on whether there is any chance of the code 
being patched being concurrently executed."
I translate this as all the other CPUs may be alive and the code would 
be mapped with read-executable permission (no write permission). It will 
not be easily possible to make the region writable because the processor 
has been configured to forbid it.
__apply_alternatives relies on the region patched to be write accessible 
and the region not executed by any CPUs.
 __apply_alternatives_multi_stop has the logic make the write 
accessible. This is not the case of apply_alternatives.
So the former function is safe while the latter one is unsafe when live 
is true.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel