| Message ID | 20240305121121.3527944-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/livepatch: Reinstate the ability to patch .rodata | expand |
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.
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
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
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
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
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
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
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
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 >
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
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;
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(+)