[PATCH 0/3] x86/livepatch: Reinstate the ability to patch .rodata

[PATCH 0/3] x86/livepatch: Reinstate the ability to patch .rodata

[Andrew Cooper]

This capability was lost when fixing liveaptching vs CET, with a TODO to
fix...

Testing (In progress):
  https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1201429148
  https://cirrus-ci.com/build/5510098735333376

Andrew Cooper (3):
  xen/virtual-region: Rename start/end fields
  xen/virtual-region: Include rodata pointers
  x86/livepatch: Relax permissions on rodata too

 xen/arch/x86/livepatch.c         |  4 ++--
 xen/common/livepatch.c           | 10 ++++++++--
 xen/common/virtual_region.c      | 33 +++++++++++++++++++++++---------
 xen/include/xen/virtual_region.h |  8 ++++++--
 4 files changed, 40 insertions(+), 15 deletions(-)


base-commit: fc84b4a5a37b9250d87ef63983b48e1953bba6d1
-- 
2.30.2

[PATCH 1/3] xen/virtual-region: Rename start/end fields

[Andrew Cooper]

... to text_{start,end}.  We're about to introduce another start/end pair.

As minor cleanup, replace ROUNDUP(x, PAGE_SIZE) with the more consice
PAGE_ALIGN() ahead of duplicating the example.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/common/livepatch.c           |  4 ++--
 xen/common/virtual_region.c      | 19 ++++++++++---------
 xen/include/xen/virtual_region.h |  5 +++--
 3 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 5a7d5b7be0ad..888beb273244 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -785,8 +785,8 @@ static int prepare_payload(struct payload *payload,
     region = &payload->region;
 
     region->symbols_lookup = livepatch_symbols_lookup;
-    region->start = payload->text_addr;
-    region->end = payload->text_addr + payload->text_size;
+    region->text_start = payload->text_addr;
+    region->text_end = payload->text_addr + payload->text_size;
 
     /* Optional sections. */
     for ( i = 0; i < BUGFRAME_NR; i++ )
diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
index cefef3e47e73..b74030d70065 100644
--- a/xen/common/virtual_region.c
+++ b/xen/common/virtual_region.c
@@ -11,15 +11,15 @@
 
 static struct virtual_region core = {
     .list = LIST_HEAD_INIT(core.list),
-    .start = _stext,
-    .end = _etext,
+    .text_start = _stext,
+    .text_end = _etext,
 };
 
 /* Becomes irrelevant when __init sections are cleared. */
 static struct virtual_region core_init __initdata = {
     .list = LIST_HEAD_INIT(core_init.list),
-    .start = _sinittext,
-    .end = _einittext,
+    .text_start = _sinittext,
+    .text_end = _einittext,
 };
 
 /*
@@ -39,7 +39,8 @@ const struct virtual_region *find_text_region(unsigned long addr)
     rcu_read_lock(&rcu_virtual_region_lock);
     list_for_each_entry_rcu ( iter, &virtual_region_list, list )
     {
-        if ( (void *)addr >= iter->start && (void *)addr < iter->end )
+        if ( (void *)addr >= iter->text_start &&
+             (void *)addr <  iter->text_end )
         {
             region = iter;
             break;
@@ -88,8 +89,8 @@ void relax_virtual_region_perms(void)
 
     rcu_read_lock(&rcu_virtual_region_lock);
     list_for_each_entry_rcu( region, &virtual_region_list, list )
-        modify_xen_mappings_lite((unsigned long)region->start,
-                                 ROUNDUP((unsigned long)region->end, PAGE_SIZE),
+        modify_xen_mappings_lite((unsigned long)region->text_start,
+                                 PAGE_ALIGN((unsigned long)region->text_end),
                                  PAGE_HYPERVISOR_RWX);
     rcu_read_unlock(&rcu_virtual_region_lock);
 }
@@ -100,8 +101,8 @@ void tighten_virtual_region_perms(void)
 
     rcu_read_lock(&rcu_virtual_region_lock);
     list_for_each_entry_rcu( region, &virtual_region_list, list )
-        modify_xen_mappings_lite((unsigned long)region->start,
-                                 ROUNDUP((unsigned long)region->end, PAGE_SIZE),
+        modify_xen_mappings_lite((unsigned long)region->text_start,
+                                 PAGE_ALIGN((unsigned long)region->text_end),
                                  PAGE_HYPERVISOR_RX);
     rcu_read_unlock(&rcu_virtual_region_lock);
 }
diff --git a/xen/include/xen/virtual_region.h b/xen/include/xen/virtual_region.h
index d05362071135..c76e7d7932ff 100644
--- a/xen/include/xen/virtual_region.h
+++ b/xen/include/xen/virtual_region.h
@@ -12,8 +12,9 @@
 struct virtual_region
 {
     struct list_head list;
-    const void *start;                /* Virtual address start. */
-    const void *end;                  /* Virtual address end. */
+
+    const void *text_start;                /* .text virtual address start. */
+    const void *text_end;                  /* .text virtual address end. */
 
     /* If this is NULL the default lookup mechanism is used. */
     symbols_lookup_t *symbols_lookup;
-- 
2.30.2

[PATCH 2/3] xen/virtual-region: Include rodata pointers

[Andrew Cooper]

These are optional.  .init doesn't distinguish types of data like this, and
livepatches don't necesserily have any .rodata either.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/common/livepatch.c           | 6 ++++++
 xen/common/virtual_region.c      | 2 ++
 xen/include/xen/virtual_region.h | 3 +++
 3 files changed, 11 insertions(+)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 888beb273244..cabfb6391117 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -788,6 +788,12 @@ static int prepare_payload(struct payload *payload,
     region->text_start = payload->text_addr;
     region->text_end = payload->text_addr + payload->text_size;
 
+    if ( payload->ro_size )
+    {
+        region->rodata_start = payload->ro_addr;
+        region->rodata_end = payload->ro_addr + payload->ro_size;
+    }
+
     /* Optional sections. */
     for ( i = 0; i < BUGFRAME_NR; i++ )
     {
diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
index b74030d70065..d2efe9e11492 100644
--- a/xen/common/virtual_region.c
+++ b/xen/common/virtual_region.c
@@ -13,6 +13,8 @@ static struct virtual_region core = {
     .list = LIST_HEAD_INIT(core.list),
     .text_start = _stext,
     .text_end = _etext,
+    .rodata_start = _srodata,
+    .rodata_end = _erodata,
 };
 
 /* Becomes irrelevant when __init sections are cleared. */
diff --git a/xen/include/xen/virtual_region.h b/xen/include/xen/virtual_region.h
index c76e7d7932ff..7712f6ad3632 100644
--- a/xen/include/xen/virtual_region.h
+++ b/xen/include/xen/virtual_region.h
@@ -16,6 +16,9 @@ struct virtual_region
     const void *text_start;                /* .text virtual address start. */
     const void *text_end;                  /* .text virtual address end. */
 
+    const void *rodata_start;              /* .rodata virtual address start (optional). */
+    const void *rodata_end;                /* .rodata virtual address end. */
+
     /* If this is NULL the default lookup mechanism is used. */
     symbols_lookup_t *symbols_lookup;
 
-- 
2.30.2

[PATCH 3/3] x86/livepatch: Relax permissions on rodata too

[Andrew Cooper]

This reinstates the capability to patch .rodata in load/unload hooks, which
was lost when we stopped using CR0.WP=0 to patch.

This turns out to be rather less of a large TODO than I thought at the time.

Fixes: 8676092a0f16 ("x86/livepatch: Fix livepatch application when CET is active")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/livepatch.c    |  4 ++--
 xen/common/virtual_region.c | 12 ++++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index ee539f001b73..4f76127e1f77 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -62,7 +62,7 @@ int arch_livepatch_safety_check(void)
 int noinline arch_livepatch_quiesce(void)
 {
     /*
-     * Relax perms on .text to be RWX, so we can modify them.
+     * Relax perms on .text/.rodata, so we can modify them.
      *
      * This relaxes perms globally, but all other CPUs are waiting on us.
      */
@@ -75,7 +75,7 @@ int noinline arch_livepatch_quiesce(void)
 void noinline arch_livepatch_revive(void)
 {
     /*
-     * Reinstate perms on .text to be RX.  This also cleans out the dirty
+     * Reinstate perms on .text/.rodata.  This also cleans out the dirty
      * bits, which matters when CET Shstk is active.
      *
      * The other CPUs waiting for us could in principle have re-walked while
diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
index d2efe9e11492..f45812483b8e 100644
--- a/xen/common/virtual_region.c
+++ b/xen/common/virtual_region.c
@@ -91,9 +91,15 @@ void relax_virtual_region_perms(void)
 
     rcu_read_lock(&rcu_virtual_region_lock);
     list_for_each_entry_rcu( region, &virtual_region_list, list )
+    {
         modify_xen_mappings_lite((unsigned long)region->text_start,
                                  PAGE_ALIGN((unsigned long)region->text_end),
                                  PAGE_HYPERVISOR_RWX);
+        if ( region->rodata_start )
+            modify_xen_mappings_lite((unsigned long)region->rodata_start,
+                                     ROUNDUP((unsigned long)region->rodata_end, PAGE_SIZE),
+                                     PAGE_HYPERVISOR_RW);
+    }
     rcu_read_unlock(&rcu_virtual_region_lock);
 }
 
@@ -103,9 +109,15 @@ void tighten_virtual_region_perms(void)
 
     rcu_read_lock(&rcu_virtual_region_lock);
     list_for_each_entry_rcu( region, &virtual_region_list, list )
+    {
         modify_xen_mappings_lite((unsigned long)region->text_start,
                                  PAGE_ALIGN((unsigned long)region->text_end),
                                  PAGE_HYPERVISOR_RX);
+        if ( region->rodata_start )
+            modify_xen_mappings_lite((unsigned long)region->rodata_start,
+                                     ROUNDUP((unsigned long)region->rodata_end, PAGE_SIZE),
+                                     PAGE_HYPERVISOR_RO);
+    }
     rcu_read_unlock(&rcu_virtual_region_lock);
 }
 #endif /* CONFIG_X86 */
-- 
2.30.2

Re: [PATCH 3/3] x86/livepatch: Relax permissions on rodata too

[Andrew Cooper]

On 05/03/2024 12:11 pm, Andrew Cooper wrote:
> diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
> index d2efe9e11492..f45812483b8e 100644
> --- a/xen/common/virtual_region.c
> +++ b/xen/common/virtual_region.c
> @@ -91,9 +91,15 @@ void relax_virtual_region_perms(void)
>  
>      rcu_read_lock(&rcu_virtual_region_lock);
>      list_for_each_entry_rcu( region, &virtual_region_list, list )
> +    {
>          modify_xen_mappings_lite((unsigned long)region->text_start,
>                                   PAGE_ALIGN((unsigned long)region->text_end),
>                                   PAGE_HYPERVISOR_RWX);
> +        if ( region->rodata_start )
> +            modify_xen_mappings_lite((unsigned long)region->rodata_start,
> +                                     ROUNDUP((unsigned long)region->rodata_end, PAGE_SIZE),

I missed the final refresh to turn this to PAGE_ALIGN().  Fixed locally.

~Andrew

Re: [PATCH 1/3] xen/virtual-region: Rename start/end fields

[Roger Pau Monné]

On Tue, Mar 05, 2024 at 12:11:19PM +0000, Andrew Cooper wrote:
> ... to text_{start,end}.  We're about to introduce another start/end pair.
> 
> As minor cleanup, replace ROUNDUP(x, PAGE_SIZE) with the more consice
> PAGE_ALIGN() ahead of duplicating the example.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 2/3] xen/virtual-region: Include rodata pointers

[Roger Pau Monné]

On Tue, Mar 05, 2024 at 12:11:20PM +0000, Andrew Cooper wrote:
> These are optional.  .init doesn't distinguish types of data like this, and
> livepatches don't necesserily have any .rodata either.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 3/3] x86/livepatch: Relax permissions on rodata too

[Roger Pau Monné]

On Tue, Mar 05, 2024 at 01:02:37PM +0000, Andrew Cooper wrote:
> On 05/03/2024 12:11 pm, Andrew Cooper wrote:
> > diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
> > index d2efe9e11492..f45812483b8e 100644
> > --- a/xen/common/virtual_region.c
> > +++ b/xen/common/virtual_region.c
> > @@ -91,9 +91,15 @@ void relax_virtual_region_perms(void)
> >  
> >      rcu_read_lock(&rcu_virtual_region_lock);
> >      list_for_each_entry_rcu( region, &virtual_region_list, list )
> > +    {
> >          modify_xen_mappings_lite((unsigned long)region->text_start,
> >                                   PAGE_ALIGN((unsigned long)region->text_end),
> >                                   PAGE_HYPERVISOR_RWX);
> > +        if ( region->rodata_start )
> > +            modify_xen_mappings_lite((unsigned long)region->rodata_start,
> > +                                     ROUNDUP((unsigned long)region->rodata_end, PAGE_SIZE),
> 
> I missed the final refresh to turn this to PAGE_ALIGN().  Fixed locally.

With that:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 2/3] xen/virtual-region: Include rodata pointers

[Jan Beulich]

On 05.03.2024 13:11, Andrew Cooper wrote:
> --- a/xen/include/xen/virtual_region.h
> +++ b/xen/include/xen/virtual_region.h
> @@ -16,6 +16,9 @@ struct virtual_region
>      const void *text_start;                /* .text virtual address start. */
>      const void *text_end;                  /* .text virtual address end. */
>  
> +    const void *rodata_start;              /* .rodata virtual address start (optional). */
> +    const void *rodata_end;                /* .rodata virtual address end. */
> +
>      /* If this is NULL the default lookup mechanism is used. */
>      symbols_lookup_t *symbols_lookup;

While perhaps the least bad one can do without quite a bit more churn,
I'm still irritated by a virtual region (singular) suddenly covering
two ranges of VA space. At the very least I think the description should
say a word of justification in this regard. An alternative, after all,
could have been for livepatch code to register separate regions for
rodata (if present in a patch).

A follow-on question then would be why ordinary data isn't reflected in
a virtual region. Aiui that's just because livepatch presently has no
need for it.

Underlying question to both: Is the virtual region concept indeed meant
to be fully tied to livepatch and its needs?

Jan

Re: [PATCH 2/3] xen/virtual-region: Include rodata pointers

[Ross Lagerwall]

On Tue, Mar 5, 2024 at 2:17 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 05.03.2024 13:11, Andrew Cooper wrote:
> > --- a/xen/include/xen/virtual_region.h
> > +++ b/xen/include/xen/virtual_region.h
> > @@ -16,6 +16,9 @@ struct virtual_region
> >      const void *text_start;                /* .text virtual address start. */
> >      const void *text_end;                  /* .text virtual address end. */
> >
> > +    const void *rodata_start;              /* .rodata virtual address start (optional). */
> > +    const void *rodata_end;                /* .rodata virtual address end. */
> > +
> >      /* If this is NULL the default lookup mechanism is used. */
> >      symbols_lookup_t *symbols_lookup;
>
> While perhaps the least bad one can do without quite a bit more churn,
> I'm still irritated by a virtual region (singular) suddenly covering
> two ranges of VA space. At the very least I think the description should
> say a word of justification in this regard. An alternative, after all,
> could have been for livepatch code to register separate regions for
> rodata (if present in a patch).
>
> A follow-on question then would be why ordinary data isn't reflected in
> a virtual region. Aiui that's just because livepatch presently has no
> need for it.
>
> Underlying question to both: Is the virtual region concept indeed meant
> to be fully tied to livepatch and its needs?
>

Virtual regions were introduced for live patching but I don't think it
is completely tied to live patching. It was introduced so that any code
can participate in symbol lookup, bug frame and exception table entry
search, rather than special casing "if livepatch" in many places.

I agree that the virtual region concept is being abused here - it's just
being used as a convenient place to store rodata start/end and doesn't
really have much to do with the text start & end / bug frame / exception
table entry search that the virtual region is intended for.

Maybe Andrew can explain why he used this approach?

Ross

Re: [PATCH 2/3] xen/virtual-region: Include rodata pointers

[Andrew Cooper]

On 06/03/2024 5:09 pm, Ross Lagerwall wrote:
> On Tue, Mar 5, 2024 at 2:17 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 05.03.2024 13:11, Andrew Cooper wrote:
>>> --- a/xen/include/xen/virtual_region.h
>>> +++ b/xen/include/xen/virtual_region.h
>>> @@ -16,6 +16,9 @@ struct virtual_region
>>>      const void *text_start;                /* .text virtual address start. */
>>>      const void *text_end;                  /* .text virtual address end. */
>>>
>>> +    const void *rodata_start;              /* .rodata virtual address start (optional). */
>>> +    const void *rodata_end;                /* .rodata virtual address end. */
>>> +
>>>      /* If this is NULL the default lookup mechanism is used. */
>>>      symbols_lookup_t *symbols_lookup;
>> While perhaps the least bad one can do without quite a bit more churn,
>> I'm still irritated by a virtual region (singular) suddenly covering
>> two ranges of VA space. At the very least I think the description should
>> say a word of justification in this regard. An alternative, after all,
>> could have been for livepatch code to register separate regions for
>> rodata (if present in a patch).
>>
>> A follow-on question then would be why ordinary data isn't reflected in
>> a virtual region. Aiui that's just because livepatch presently has no
>> need for it.
>>
>> Underlying question to both: Is the virtual region concept indeed meant
>> to be fully tied to livepatch and its needs?
>>
> Virtual regions were introduced for live patching but I don't think it
> is completely tied to live patching. It was introduced so that any code
> can participate in symbol lookup, bug frame and exception table entry
> search, rather than special casing "if livepatch" in many places.
>
> I agree that the virtual region concept is being abused here - it's just
> being used as a convenient place to store rodata start/end and doesn't
> really have much to do with the text start & end / bug frame / exception
> table entry search that the virtual region is intended for.
>
> Maybe Andrew can explain why he used this approach?

I feel the simplicity and obviousness of patch 3 speaks for itself.

How do you propose fixing it differently.

~Andrew

Re: [PATCH 2/3] xen/virtual-region: Include rodata pointers

[Jan Beulich]

On 06.03.2024 18:21, Andrew Cooper wrote:
> On 06/03/2024 5:09 pm, Ross Lagerwall wrote:
>> On Tue, Mar 5, 2024 at 2:17 PM Jan Beulich <jbeulich@suse.com> wrote:
>>> On 05.03.2024 13:11, Andrew Cooper wrote:
>>>> --- a/xen/include/xen/virtual_region.h
>>>> +++ b/xen/include/xen/virtual_region.h
>>>> @@ -16,6 +16,9 @@ struct virtual_region
>>>>      const void *text_start;                /* .text virtual address start. */
>>>>      const void *text_end;                  /* .text virtual address end. */
>>>>
>>>> +    const void *rodata_start;              /* .rodata virtual address start (optional). */
>>>> +    const void *rodata_end;                /* .rodata virtual address end. */
>>>> +
>>>>      /* If this is NULL the default lookup mechanism is used. */
>>>>      symbols_lookup_t *symbols_lookup;
>>> While perhaps the least bad one can do without quite a bit more churn,
>>> I'm still irritated by a virtual region (singular) suddenly covering
>>> two ranges of VA space. At the very least I think the description should
>>> say a word of justification in this regard. An alternative, after all,
>>> could have been for livepatch code to register separate regions for
>>> rodata (if present in a patch).
>>>
>>> A follow-on question then would be why ordinary data isn't reflected in
>>> a virtual region. Aiui that's just because livepatch presently has no
>>> need for it.
>>>
>>> Underlying question to both: Is the virtual region concept indeed meant
>>> to be fully tied to livepatch and its needs?
>>>
>> Virtual regions were introduced for live patching but I don't think it
>> is completely tied to live patching. It was introduced so that any code
>> can participate in symbol lookup, bug frame and exception table entry
>> search, rather than special casing "if livepatch" in many places.
>>
>> I agree that the virtual region concept is being abused here - it's just
>> being used as a convenient place to store rodata start/end and doesn't
>> really have much to do with the text start & end / bug frame / exception
>> table entry search that the virtual region is intended for.
>>
>> Maybe Andrew can explain why he used this approach?
> 
> I feel the simplicity and obviousness of patch 3 speaks for itself.
> 
> How do you propose fixing it differently.

I'm not opposed to doing it the way you do, but I think it then needs
clarifying (up front) what a virtual region really is. It looks to be
morphing into a module description instead ... One easy option might
be to have a comment next to the struct additions here making clear
that this is rather an abuse, but chosen to be this way to keep things
simple elsewhere.

Jan

Re: [PATCH 2/3] xen/virtual-region: Include rodata pointers

[Andrew Cooper]

On 07/03/2024 7:39 am, Jan Beulich wrote:
> On 06.03.2024 18:21, Andrew Cooper wrote:
>> On 06/03/2024 5:09 pm, Ross Lagerwall wrote:
>>> On Tue, Mar 5, 2024 at 2:17 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 05.03.2024 13:11, Andrew Cooper wrote:
>>>>> --- a/xen/include/xen/virtual_region.h
>>>>> +++ b/xen/include/xen/virtual_region.h
>>>>> @@ -16,6 +16,9 @@ struct virtual_region
>>>>>      const void *text_start;                /* .text virtual address start. */
>>>>>      const void *text_end;                  /* .text virtual address end. */
>>>>>
>>>>> +    const void *rodata_start;              /* .rodata virtual address start (optional). */
>>>>> +    const void *rodata_end;                /* .rodata virtual address end. */
>>>>> +
>>>>>      /* If this is NULL the default lookup mechanism is used. */
>>>>>      symbols_lookup_t *symbols_lookup;
>>>> While perhaps the least bad one can do without quite a bit more churn,
>>>> I'm still irritated by a virtual region (singular) suddenly covering
>>>> two ranges of VA space. At the very least I think the description should
>>>> say a word of justification in this regard. An alternative, after all,
>>>> could have been for livepatch code to register separate regions for
>>>> rodata (if present in a patch).
>>>>
>>>> A follow-on question then would be why ordinary data isn't reflected in
>>>> a virtual region. Aiui that's just because livepatch presently has no
>>>> need for it.
>>>>
>>>> Underlying question to both: Is the virtual region concept indeed meant
>>>> to be fully tied to livepatch and its needs?
>>>>
>>> Virtual regions were introduced for live patching but I don't think it
>>> is completely tied to live patching. It was introduced so that any code
>>> can participate in symbol lookup, bug frame and exception table entry
>>> search, rather than special casing "if livepatch" in many places.
>>>
>>> I agree that the virtual region concept is being abused here - it's just
>>> being used as a convenient place to store rodata start/end and doesn't
>>> really have much to do with the text start & end / bug frame / exception
>>> table entry search that the virtual region is intended for.
>>>
>>> Maybe Andrew can explain why he used this approach?
>> I feel the simplicity and obviousness of patch 3 speaks for itself.
>>
>> How do you propose fixing it differently.
> I'm not opposed to doing it the way you do, but I think it then needs
> clarifying (up front) what a virtual region really is. It looks to be
> morphing into a module description instead ... One easy option might
> be to have a comment next to the struct additions here making clear
> that this is rather an abuse, but chosen to be this way to keep things
> simple elsewhere.

The thing called virtual_region already describes 6 ranges, and I'm
adding a 7th.

It has been a module-ish description right from the very outset.  I
don't think it is fair to describe this as an abuse at all.

Is this going to satisfy the outstanding concerns?

diff --git a/xen/include/xen/virtual_region.h
b/xen/include/xen/virtual_region.h
index d05362071135..9d150beb8a87 100644
--- a/xen/include/xen/virtual_region.h
+++ b/xen/include/xen/virtual_region.h
@@ -9,6 +9,12 @@
 #include <xen/list.h>
 #include <xen/symbols.h>
 
+/*
+ * Despite it's name, this is module(ish) description.
+ *
+ * There's one region for .text/etc, one region for .init during boot only,
+ * and one region per livepatch.
+ */
 struct virtual_region
 {
     struct list_head list;

~Andrew

Re: [PATCH 2/3] xen/virtual-region: Include rodata pointers

[Jan Beulich]

On 07.03.2024 12:31, Andrew Cooper wrote:
> On 07/03/2024 7:39 am, Jan Beulich wrote:
>> On 06.03.2024 18:21, Andrew Cooper wrote:
>>> On 06/03/2024 5:09 pm, Ross Lagerwall wrote:
>>>> On Tue, Mar 5, 2024 at 2:17 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>> On 05.03.2024 13:11, Andrew Cooper wrote:
>>>>>> --- a/xen/include/xen/virtual_region.h
>>>>>> +++ b/xen/include/xen/virtual_region.h
>>>>>> @@ -16,6 +16,9 @@ struct virtual_region
>>>>>>      const void *text_start;                /* .text virtual address start. */
>>>>>>      const void *text_end;                  /* .text virtual address end. */
>>>>>>
>>>>>> +    const void *rodata_start;              /* .rodata virtual address start (optional). */
>>>>>> +    const void *rodata_end;                /* .rodata virtual address end. */
>>>>>> +
>>>>>>      /* If this is NULL the default lookup mechanism is used. */
>>>>>>      symbols_lookup_t *symbols_lookup;
>>>>> While perhaps the least bad one can do without quite a bit more churn,
>>>>> I'm still irritated by a virtual region (singular) suddenly covering
>>>>> two ranges of VA space. At the very least I think the description should
>>>>> say a word of justification in this regard. An alternative, after all,
>>>>> could have been for livepatch code to register separate regions for
>>>>> rodata (if present in a patch).
>>>>>
>>>>> A follow-on question then would be why ordinary data isn't reflected in
>>>>> a virtual region. Aiui that's just because livepatch presently has no
>>>>> need for it.
>>>>>
>>>>> Underlying question to both: Is the virtual region concept indeed meant
>>>>> to be fully tied to livepatch and its needs?
>>>>>
>>>> Virtual regions were introduced for live patching but I don't think it
>>>> is completely tied to live patching. It was introduced so that any code
>>>> can participate in symbol lookup, bug frame and exception table entry
>>>> search, rather than special casing "if livepatch" in many places.
>>>>
>>>> I agree that the virtual region concept is being abused here - it's just
>>>> being used as a convenient place to store rodata start/end and doesn't
>>>> really have much to do with the text start & end / bug frame / exception
>>>> table entry search that the virtual region is intended for.
>>>>
>>>> Maybe Andrew can explain why he used this approach?
>>> I feel the simplicity and obviousness of patch 3 speaks for itself.
>>>
>>> How do you propose fixing it differently.
>> I'm not opposed to doing it the way you do, but I think it then needs
>> clarifying (up front) what a virtual region really is. It looks to be
>> morphing into a module description instead ... One easy option might
>> be to have a comment next to the struct additions here making clear
>> that this is rather an abuse, but chosen to be this way to keep things
>> simple elsewhere.
> 
> The thing called virtual_region already describes 6 ranges, and I'm
> adding a 7th.

Hmm, yes, in a way you're right.

> It has been a module-ish description right from the very outset.  I
> don't think it is fair to describe this as an abuse at all.
> 
> Is this going to satisfy the outstanding concerns?

Yes. And thank you for bearing with me.

Jan

> diff --git a/xen/include/xen/virtual_region.h
> b/xen/include/xen/virtual_region.h
> index d05362071135..9d150beb8a87 100644
> --- a/xen/include/xen/virtual_region.h
> +++ b/xen/include/xen/virtual_region.h
> @@ -9,6 +9,12 @@
>  #include <xen/list.h>
>  #include <xen/symbols.h>
>  
> +/*
> + * Despite it's name, this is module(ish) description.
> + *
> + * There's one region for .text/etc, one region for .init during boot only,
> + * and one region per livepatch.
> + */
>  struct virtual_region
>  {
>      struct list_head list;
> 
> ~Andrew

Re: [PATCH 2/3] xen/virtual-region: Include rodata pointers

[Andrew Cooper]

On 07/03/2024 11:58 am, Jan Beulich wrote:
> On 07.03.2024 12:31, Andrew Cooper wrote:
>>
>> The thing called virtual_region already describes 6 ranges, and I'm
>> adding a 7th.
> Hmm, yes, in a way you're right.
>
>> It has been a module-ish description right from the very outset.  I
>> don't think it is fair to describe this as an abuse at all.
>>
>> Is this going to satisfy the outstanding concerns?
> Yes. And thank you for bearing with me.

No problem.  I'm glad that we've come to an agreement.

Ross?

~Andrew

>
> Jan
>
>> diff --git a/xen/include/xen/virtual_region.h
>> b/xen/include/xen/virtual_region.h
>> index d05362071135..9d150beb8a87 100644
>> --- a/xen/include/xen/virtual_region.h
>> +++ b/xen/include/xen/virtual_region.h
>> @@ -9,6 +9,12 @@
>>  #include <xen/list.h>
>>  #include <xen/symbols.h>
>>  
>> +/*
>> + * Despite it's name, this is module(ish) description.
>> + *
>> + * There's one region for .text/etc, one region for .init during boot only,
>> + * and one region per livepatch.
>> + */
>>  struct virtual_region
>>  {
>>      struct list_head list;
>>
>> ~Andrew

Re: [PATCH 2/3] xen/virtual-region: Include rodata pointers

[Ross Lagerwall]

On Thu, Mar 7, 2024 at 12:16 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 07/03/2024 11:58 am, Jan Beulich wrote:
> > On 07.03.2024 12:31, Andrew Cooper wrote:
> >>
> >> The thing called virtual_region already describes 6 ranges, and I'm
> >> adding a 7th.
> > Hmm, yes, in a way you're right.
> >
> >> It has been a module-ish description right from the very outset.  I
> >> don't think it is fair to describe this as an abuse at all.
> >>
> >> Is this going to satisfy the outstanding concerns?
> > Yes. And thank you for bearing with me.
>
> No problem.  I'm glad that we've come to an agreement.
>
> Ross?
>

Yes, I think that is fine with the additional description clarifying it.

With that,

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

>
> >
> > Jan
> >
> >> diff --git a/xen/include/xen/virtual_region.h
> >> b/xen/include/xen/virtual_region.h
> >> index d05362071135..9d150beb8a87 100644
> >> --- a/xen/include/xen/virtual_region.h
> >> +++ b/xen/include/xen/virtual_region.h
> >> @@ -9,6 +9,12 @@
> >>  #include <xen/list.h>
> >>  #include <xen/symbols.h>
> >>
> >> +/*
> >> + * Despite it's name, this is module(ish) description.
> >> + *
> >> + * There's one region for .text/etc, one region for .init during boot only,
> >> + * and one region per livepatch.
> >> + */
> >>  struct virtual_region
> >>  {
> >>      struct list_head list;
> >>
> >> ~Andrew
>

Re: [PATCH 3/3] x86/livepatch: Relax permissions on rodata too

[Ross Lagerwall]

On Tue, Mar 5, 2024 at 1:02 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 05/03/2024 12:11 pm, Andrew Cooper wrote:
> > diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
> > index d2efe9e11492..f45812483b8e 100644
> > --- a/xen/common/virtual_region.c
> > +++ b/xen/common/virtual_region.c
> > @@ -91,9 +91,15 @@ void relax_virtual_region_perms(void)
> >
> >      rcu_read_lock(&rcu_virtual_region_lock);
> >      list_for_each_entry_rcu( region, &virtual_region_list, list )
> > +    {
> >          modify_xen_mappings_lite((unsigned long)region->text_start,
> >                                   PAGE_ALIGN((unsigned long)region->text_end),
> >                                   PAGE_HYPERVISOR_RWX);
> > +        if ( region->rodata_start )
> > +            modify_xen_mappings_lite((unsigned long)region->rodata_start,
> > +                                     ROUNDUP((unsigned long)region->rodata_end, PAGE_SIZE),
>
> I missed the final refresh to turn this to PAGE_ALIGN().  Fixed locally.

In that case,

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Re: [PATCH 1/3] xen/virtual-region: Rename start/end fields

[Ross Lagerwall]

On Tue, Mar 5, 2024 at 12:11 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> ... to text_{start,end}.  We're about to introduce another start/end pair.
>
> As minor cleanup, replace ROUNDUP(x, PAGE_SIZE) with the more consice
> PAGE_ALIGN() ahead of duplicating the example.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Re: [PATCH 2/3] xen/virtual-region: Include rodata pointers

[Andrew Cooper]

On 07/03/2024 1:03 pm, Ross Lagerwall wrote:
> On Thu, Mar 7, 2024 at 12:16 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 07/03/2024 11:58 am, Jan Beulich wrote:
>>> On 07.03.2024 12:31, Andrew Cooper wrote:
>>>> The thing called virtual_region already describes 6 ranges, and I'm
>>>> adding a 7th.
>>> Hmm, yes, in a way you're right.
>>>
>>>> It has been a module-ish description right from the very outset.  I
>>>> don't think it is fair to describe this as an abuse at all.
>>>>
>>>> Is this going to satisfy the outstanding concerns?
>>> Yes. And thank you for bearing with me.
>> No problem.  I'm glad that we've come to an agreement.
>>
>> Ross?
>>
> Yes, I think that is fine with the additional description clarifying it.
>
> With that,
>
> Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Thanks.

It occurs to me that this comment is best in patch 1, which is the patch
that removes the final trace of this looking like a single thing.

~Andrew