[Xen-devel] [PATCH] create-diff-object: do not strip STN_UNDEF symbols from *.fixup

[Xen-devel] [PATCH] create-diff-object: do not strip STN_UNDEF symbols from *.fixup

[Pawel Wieczorkiewicz]

The rela groups in the *.fixup sections vary in size. That makes it
more complex to handle in the livepatch_strip_undefined_elements().
It is also unnecessary as the .fixup sections are unlikely to have
any STN_UNDEF symbols anyway.

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
---
 create-diff-object.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/create-diff-object.c b/create-diff-object.c
index 2f0e162..abf3cc7 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -2081,6 +2081,13 @@ static void livepatch_strip_undefined_elements(struct kpatch_elf *kelf)
 		if (!is_rela_section(sec))
 			continue;
 
+		/* The rela groups in the .fixup sections vary in size.
+		 * Ignore them as they are unlikely to have any STN_UNDEF
+		 * symbols anyway.
+		 */
+		if (strstr(sec->name, ".fixup"))
+			continue;
+
 		/* only known, fixed-size entries can be stripped */
 		entry_size = get_section_entry_size(sec->base, kelf);
 		if (entry_size == 0)
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




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

[Xen-devel] [PATCH] create-diff-object: more precisely identify .rodata sections

[Pawel Wieczorkiewicz]

This is needed for more precise patchability verification.
Only non-special .rodata sections should be subject
for such a non-referenced check in kpatch_verify_patchability().
Current check (non-standard, non-rela, non-debug) is too weak and
allows also non-rodata sections without referenced symbols to slip
through.

Detect .rodata section by checking section's type (SHT_PROGBITS),
flags (no exec, no write) and finally name prefix.

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
---
 common.c             |  7 +++++++
 common.h             |  1 +
 create-diff-object.c | 13 ++++++-------
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/common.c b/common.c
index 0ddc9fa..8f553ea 100644
--- a/common.c
+++ b/common.c
@@ -249,6 +249,13 @@ int is_text_section(struct section *sec)
 		(sec->sh.sh_flags & SHF_EXECINSTR));
 }
 
+int is_rodata_section(struct section *sec)
+{
+	return sec->sh.sh_type == SHT_PROGBITS &&
+	       !(sec->sh.sh_flags & (SHF_EXECINSTR | SHF_WRITE)) &&
+	       !strncmp(sec->name, ".rodata", 7);
+}
+
 int is_debug_section(struct section *sec)
 {
 	char *name;
diff --git a/common.h b/common.h
index 7c6fb73..b6489db 100644
--- a/common.h
+++ b/common.h
@@ -159,6 +159,7 @@ struct symbol *find_symbol_by_index(struct list_head *list, size_t index);
 struct symbol *find_symbol_by_name(struct list_head *list, const char *name);
 
 int is_text_section(struct section *sec);
+int is_rodata_section(struct section *sec);
 int is_debug_section(struct section *sec);
 int is_rela_section(struct section *sec);
 int is_standard_section(struct section *sec);
diff --git a/create-diff-object.c b/create-diff-object.c
index e4592a6..2f0e162 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1672,13 +1672,12 @@ static void kpatch_verify_patchability(struct kpatch_elf *kelf)
 		}
 
 		if (sec->include) {
-			if (!is_standard_section(sec) && !is_rela_section(sec) &&
-			    !is_debug_section(sec) && !is_special_section(sec)) {
-				if (!is_referenced_section(sec, kelf)) {
-					log_normal("section %s included, but not referenced\n",
-						   sec->name);
-					errs++;
-				}
+			if (is_rodata_section(sec) &&
+			    !is_special_section(sec) &&
+			    !is_referenced_section(sec, kelf)) {
+				log_normal(".rodata section %s included, but not referenced\n",
+					   sec->name);
+				errs++;
 			}
 
 			/* Check if a RELA section does not contain any entries with
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




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

Re: [Xen-devel] [PATCH] create-diff-object: do not strip STN_UNDEF symbols from *.fixup

[Ross Lagerwall]

On 11/5/19 3:37 PM, Pawel Wieczorkiewicz wrote:
> The rela groups in the *.fixup sections vary in size. That makes it
> more complex to handle in the livepatch_strip_undefined_elements().
> It is also unnecessary as the .fixup sections are unlikely to have
> any STN_UNDEF symbols anyway.
> 
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> ---

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

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

Re: [Xen-devel] [PATCH] create-diff-object: more precisely identify .rodata sections

[Ross Lagerwall]

On 11/5/19 3:37 PM, Pawel Wieczorkiewicz wrote:
> This is needed for more precise patchability verification.
> Only non-special .rodata sections should be subject
> for such a non-referenced check in kpatch_verify_patchability().
> Current check (non-standard, non-rela, non-debug) is too weak and
> allows also non-rodata sections without referenced symbols to slip
> through.
> 
> Detect .rodata section by checking section's type (SHT_PROGBITS),
> flags (no exec, no write) and finally name prefix.
> 
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
> ---
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

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

Re: [Xen-devel] [PATCH] create-diff-object: more precisely identify .rodata sections

[Julien Grall]

Hi Lars,

On 18/09/2019 13:14, Lars Kurth wrote:
> 
> 
> On 18/09/2019, 12:15, "Julien Grall" <julien.grall@arm.com> wrote:
> 
>      Hi Ian,
>      
>      On 18/09/2019 11:41, Ian Jackson wrote:
>      > Julien Grall writes ("Re: [PATCH] create-diff-object: more precisely identify .rodata sections"):
>      >> On 18/09/2019 10:52, Wieczorkiewicz, Pawel wrote:
>      >>> $ scripts/./add_maintainers.pl -d ~/git/livepatch-build-tools
>      >>
>      >> '-d' only tells you where the patches files are. The script will look up for the
>      >> MAINTAINERS file in the current directory.
>      >
>      > Hmmm.  I wonder if we could detect this situation somehow.  This will
>      > be a common user error I think.
>      I think it would be possible for patch modifying file. We could check whether
>      the file modified exist in the repo. Though, I am not sure how difficult it
>      would be to implement.
>      
> That might be doable, but won't be easy as I will essentially need to parse the patch
> And it won't be reliable.
> 
> The only workable way of doing this may be to have a strong convention
> that requires to use the [REPONAME PATCH] via --subject-prefix when generating the
> patch and for add_maintainers.pl to verify this somehow based on the current
> directory and the patches.
> 
> We already have strong conventions in some cases, e.g. for OSSTEST we always use
> [OSSTEST PATCH]. This would potentially be helpful for the CI loop plans aso.
> 
> Assuming there is a git config setting for --subject-prefix then this could be made
> to work. I could add a section under [1] to document the convention with the
> appropriate git command. We could include a script (e.g. xen.git:scrips/git-setup)
> which does this based on the repo name automatically.

I saw a conversation on IRC about this. But it is not clear if there was any 
conclusion?

My only slight concern about tagging by default is the length of the subject, 
when directly receiving from xen-devel (i.e. not CCed), the subject is already 
[xen-devel][PATCH XX/XX]. I am assuming the tag will not be dropped so it will 
appear on the mailing list. For repo such as LIVEPATCH-BUILD, this would end up 
to lengthy prefix.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH] create-diff-object: more precisely identify .rodata sections

[Julien Grall]

Hi Lars,

On 18/09/2019 12:50, Lars Kurth wrote:
> 
> 
> On 18/09/2019, 11:44, "Wieczorkiewicz, Pawel" <wipawel@amazon.de> wrote:
> 
>      > On 18. Sep 2019, at 12:41, Ian Jackson <ian.jackson@citrix.com> wrote:
>      >
>      > Julien Grall writes ("Re: [PATCH] create-diff-object: more precisely identify .rodata sections"):
>      >> On 18/09/2019 10:52, Wieczorkiewicz, Pawel wrote:
>      >>> $ scripts/./add_maintainers.pl -d ~/git/livepatch-build-tools
>      >>
>      >> '-d' only tells you where the patches files are. The script will look up for the
>      >> MAINTAINERS file in the current directory.
>      >
>      > Hmmm.  I wonder if we could detect this situation somehow.  This will
>      > be a common user error I think.
> 
> I don't think it is possible to detect that situation as git format-patch does not tell you which tree a patch was generated from.
>      
>      I should have looked twice before sending the patch out. But, what would be very helpful for me
>      is additional option to the add_maintainers.pl script like: -m ./MAINTAINERS
>      
> In my view this is only really an issue if you create a patch or series and then do something else before finalizing and sending the patch, otherwise I would have tripped over this myself. But of course, if you work on multiple series at the same time that is an easy mistake to make.
> 
> I would expect that the most common directory structure for people is to have a directory structure such as
> ~/code/xen.git
> ~/code/livepatch-build-tools
> ...
> ~/code/patches
> 
> and that people switch between git directories. Looking at the code, I should be able to add a -m option, which would work out the directory in which MAINTAINERS is, then switch to it, do the processing and switch back to where we started from.
> 
> However, this would only really work, if there was a strong recommendation in https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Using_add_maintainers.pl_.28or_get_maintainer.pl.29_from_outside_of_xen.git telling people to use -m $path/MAKEFILE when working on multiple directories

I don't really see any advantage of this option if you still allow as a fallback 
to run on the current directory.

Ok, the user is saving 2 instructions, but there are still way for that users to 
mess it up such as it may forget the -m option because it misread the wiki.

So I would strongly suggest to either drop the fallback or not adding a new option.

Furthermore, if you let the user specific the existing MAINTAINERS file, then 
he/she might as well pass something different (like MAKEFILE in your example 
;)). It would be best if the users is not allow to give anything else than 
MAINTAINERS.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH] create-diff-object: more precisely identify .rodata sections

[Wieczorkiewicz, Pawel]

> On 18. Sep 2019, at 17:23, Lars Kurth <lars.kurth@citrix.com> wrote:
> 
> 
> 
> On 18/09/2019, 12:27, "Wieczorkiewicz, Pawel" <wipawel@amazon.de> wrote:
> 
> 
> 
>> On 18. Sep 2019, at 13:19, Julien Grall <julien.grall@arm.com> wrote:
>> 
>> Hi Pawel,
>> 
>> On 18/09/2019 11:44, Wieczorkiewicz, Pawel wrote:
>>>> On 18. Sep 2019, at 12:41, Ian Jackson <ian.jackson@citrix.com> wrote:
>>>> 
>>>> Julien Grall writes ("Re: [PATCH] create-diff-object: more precisely identify .rodata sections"):
>>>>> On 18/09/2019 10:52, Wieczorkiewicz, Pawel wrote:
>>>>>> $ scripts/./add_maintainers.pl -d ~/git/livepatch-build-tools
>>>>> 
>>>>> '-d' only tells you where the patches files are. The script will look up for the
>>>>> MAINTAINERS file in the current directory.
>>>> 
>>>> Hmmm.  I wonder if we could detect this situation somehow.  This will
>>>> be a common user error I think.
>>>> 
>>> I should have looked twice before sending the patch out. But, what would be very helpful for me
>>> is additional option to the add_maintainers.pl script like: -m ./MAINTAINERS
>> 
>> Well the filename will always be the same, so at best you will provide redundant information.
> 
>    Not if I create a git-ignored symlink to the other repo.
> 
> You could also put add_maintainers.pl on the path
> 
> Until I implement a fix,  I fixed the docs. See
> * https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Using_add_maintainers.pl_.28or_get_maintainer.pl.29_from_outside_of_xen.git 
> * https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Setting_that_help_save_you_time 
> 

Thank you Lars. I updated my scripts and workflows accordingly.

> Lars

Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


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

Re: [Xen-devel] [PATCH] create-diff-object: more precisely identify .rodata sections

[Lars Kurth]

On 18/09/2019, 12:27, "Wieczorkiewicz, Pawel" <wipawel@amazon.de> wrote:

    
    
    > On 18. Sep 2019, at 13:19, Julien Grall <julien.grall@arm.com> wrote:
    > 
    > Hi Pawel,
    > 
    > On 18/09/2019 11:44, Wieczorkiewicz, Pawel wrote:
    >>> On 18. Sep 2019, at 12:41, Ian Jackson <ian.jackson@citrix.com> wrote:
    >>> 
    >>> Julien Grall writes ("Re: [PATCH] create-diff-object: more precisely identify .rodata sections"):
    >>>> On 18/09/2019 10:52, Wieczorkiewicz, Pawel wrote:
    >>>>> $ scripts/./add_maintainers.pl -d ~/git/livepatch-build-tools
    >>>> 
    >>>> '-d' only tells you where the patches files are. The script will look up for the
    >>>> MAINTAINERS file in the current directory.
    >>> 
    >>> Hmmm.  I wonder if we could detect this situation somehow.  This will
    >>> be a common user error I think.
    >>> 
    >> I should have looked twice before sending the patch out. But, what would be very helpful for me
    >> is additional option to the add_maintainers.pl script like: -m ./MAINTAINERS
    > 
    > Well the filename will always be the same, so at best you will provide redundant information.
    
    Not if I create a git-ignored symlink to the other repo.

You could also put add_maintainers.pl on the path

Until I implement a fix,  I fixed the docs. See
* https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Using_add_maintainers.pl_.28or_get_maintainer.pl.29_from_outside_of_xen.git 
* https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Setting_that_help_save_you_time 

Lars
 

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

Re: [Xen-devel] [PATCH] create-diff-object: more precisely identify .rodata sections

[Lars Kurth]

On 18/09/2019, 12:15, "Julien Grall" <julien.grall@arm.com> wrote:

    Hi Ian,
    
    On 18/09/2019 11:41, Ian Jackson wrote:
    > Julien Grall writes ("Re: [PATCH] create-diff-object: more precisely identify .rodata sections"):
    >> On 18/09/2019 10:52, Wieczorkiewicz, Pawel wrote:
    >>> $ scripts/./add_maintainers.pl -d ~/git/livepatch-build-tools
    >>
    >> '-d' only tells you where the patches files are. The script will look up for the
    >> MAINTAINERS file in the current directory.
    > 
    > Hmmm.  I wonder if we could detect this situation somehow.  This will
    > be a common user error I think.
    I think it would be possible for patch modifying file. We could check whether 
    the file modified exist in the repo. Though, I am not sure how difficult it 
    would be to implement.
    
That might be doable, but won't be easy as I will essentially need to parse the patch    
And it won't be reliable. 

The only workable way of doing this may be to have a strong convention
that requires to use the [REPONAME PATCH] via --subject-prefix when generating the 
patch and for add_maintainers.pl to verify this somehow based on the current
directory and the patches.

We already have strong conventions in some cases, e.g. for OSSTEST we always use
[OSSTEST PATCH]. This would potentially be helpful for the CI loop plans aso. 

Assuming there is a git config setting for --subject-prefix then this could be made 
to work. I could add a section under [1] to document the convention with the
appropriate git command. We could include a script (e.g. xen.git:scrips/git-setup) 
which does this based on the repo name automatically.

Any views?

Lars

[1] https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Sending_the_patches_to_the_list 
 

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

Re: [Xen-devel] [PATCH] create-diff-object: more precisely identify .rodata sections

[Lars Kurth]

On 18/09/2019, 11:44, "Wieczorkiewicz, Pawel" <wipawel@amazon.de> wrote:

    > On 18. Sep 2019, at 12:41, Ian Jackson <ian.jackson@citrix.com> wrote:
    > 
    > Julien Grall writes ("Re: [PATCH] create-diff-object: more precisely identify .rodata sections"):
    >> On 18/09/2019 10:52, Wieczorkiewicz, Pawel wrote:
    >>> $ scripts/./add_maintainers.pl -d ~/git/livepatch-build-tools
    >> 
    >> '-d' only tells you where the patches files are. The script will look up for the 
    >> MAINTAINERS file in the current directory.
    > 
    > Hmmm.  I wonder if we could detect this situation somehow.  This will
    > be a common user error I think.

I don't think it is possible to detect that situation as git format-patch does not tell you which tree a patch was generated from.
    
    I should have looked twice before sending the patch out. But, what would be very helpful for me
    is additional option to the add_maintainers.pl script like: -m ./MAINTAINERS
    
In my view this is only really an issue if you create a patch or series and then do something else before finalizing and sending the patch, otherwise I would have tripped over this myself. But of course, if you work on multiple series at the same time that is an easy mistake to make.

I would expect that the most common directory structure for people is to have a directory structure such as
~/code/xen.git
~/code/livepatch-build-tools
...
~/code/patches 

and that people switch between git directories. Looking at the code, I should be able to add a -m option, which would work out the directory in which MAINTAINERS is, then switch to it, do the processing and switch back to where we started from.

However, this would only really work, if there was a strong recommendation in https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Using_add_maintainers.pl_.28or_get_maintainer.pl.29_from_outside_of_xen.git telling people to use -m $path/MAKEFILE when working on multiple directories

Would that work?

Lars
 

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

Re: [Xen-devel] [PATCH] create-diff-object: more precisely identify .rodata sections

[Julien Grall]

On 18/09/2019 12:27, Wieczorkiewicz, Pawel wrote:
> 
> 
>> On 18. Sep 2019, at 13:19, Julien Grall <julien.grall@arm.com> wrote:
>>
>> Hi Pawel,
>>
>> On 18/09/2019 11:44, Wieczorkiewicz, Pawel wrote:
>>>> On 18. Sep 2019, at 12:41, Ian Jackson <ian.jackson@citrix.com> wrote:
>>>>
>>>> Julien Grall writes ("Re: [PATCH] create-diff-object: more precisely identify .rodata sections"):
>>>>> On 18/09/2019 10:52, Wieczorkiewicz, Pawel wrote:
>>>>>> $ scripts/./add_maintainers.pl -d ~/git/livepatch-build-tools
>>>>>
>>>>> '-d' only tells you where the patches files are. The script will look up for the
>>>>> MAINTAINERS file in the current directory.
>>>>
>>>> Hmmm.  I wonder if we could detect this situation somehow.  This will
>>>> be a common user error I think.
>>>>
>>> I should have looked twice before sending the patch out. But, what would be very helpful for me
>>> is additional option to the add_maintainers.pl script like: -m ./MAINTAINERS
>>
>> Well the filename will always be the same, so at best you will provide redundant information.
> 
> Not if I create a git-ignored symlink to the other repo.

Why would you do that...?

add_maintainers.pl and get_maintainers.pl relies to be used on ./MAINTAINERS. I 
am quite reluctant to allow any other use as you increase the risk for the user 
to misuse the scripts.

> 
>>
>> However, it is not uncommon to have script that needs to apply on the current directory. What would a new option add? Is it just avoid to do a "cd ..." before?
>>
> 
> Mainly the new option will avoid the 'cd', but it will also force me to specify the right file.
> 
> The option can be totally optional with a CWD as a default fallback.

Which completely defeats the purpose of forcing you the specify the right file...

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH] create-diff-object: more precisely identify .rodata sections

[Wieczorkiewicz, Pawel]

> On 18. Sep 2019, at 13:19, Julien Grall <julien.grall@arm.com> wrote:
> 
> Hi Pawel,
> 
> On 18/09/2019 11:44, Wieczorkiewicz, Pawel wrote:
>>> On 18. Sep 2019, at 12:41, Ian Jackson <ian.jackson@citrix.com> wrote:
>>> 
>>> Julien Grall writes ("Re: [PATCH] create-diff-object: more precisely identify .rodata sections"):
>>>> On 18/09/2019 10:52, Wieczorkiewicz, Pawel wrote:
>>>>> $ scripts/./add_maintainers.pl -d ~/git/livepatch-build-tools
>>>> 
>>>> '-d' only tells you where the patches files are. The script will look up for the
>>>> MAINTAINERS file in the current directory.
>>> 
>>> Hmmm.  I wonder if we could detect this situation somehow.  This will
>>> be a common user error I think.
>>> 
>> I should have looked twice before sending the patch out. But, what would be very helpful for me
>> is additional option to the add_maintainers.pl script like: -m ./MAINTAINERS
> 
> Well the filename will always be the same, so at best you will provide redundant information.

Not if I create a git-ignored symlink to the other repo.

> 
> However, it is not uncommon to have script that needs to apply on the current directory. What would a new option add? Is it just avoid to do a "cd ..." before?
> 

Mainly the new option will avoid the 'cd', but it will also force me to specify the right file.

The option can be totally optional with a CWD as a default fallback.

> Cheers,
> 
> -- 
> Julien Grall

Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




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

Re: [Xen-devel] [PATCH] create-diff-object: more precisely identify .rodata sections

[Julien Grall]

Hi Pawel,

On 18/09/2019 11:44, Wieczorkiewicz, Pawel wrote:
> 
> 
>> On 18. Sep 2019, at 12:41, Ian Jackson <ian.jackson@citrix.com> wrote:
>>
>> Julien Grall writes ("Re: [PATCH] create-diff-object: more precisely identify .rodata sections"):
>>> On 18/09/2019 10:52, Wieczorkiewicz, Pawel wrote:
>>>> $ scripts/./add_maintainers.pl -d ~/git/livepatch-build-tools
>>>
>>> '-d' only tells you where the patches files are. The script will look up for the
>>> MAINTAINERS file in the current directory.
>>
>> Hmmm.  I wonder if we could detect this situation somehow.  This will
>> be a common user error I think.
>>
> 
> I should have looked twice before sending the patch out. But, what would be very helpful for me
> is additional option to the add_maintainers.pl script like: -m ./MAINTAINERS

Well the filename will always be the same, so at best you will provide redundant 
information.

However, it is not uncommon to have script that needs to apply on the current 
directory. What would a new option add? Is it just avoid to do a "cd ..." before?

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH] create-diff-object: more precisely identify .rodata sections

[Julien Grall]

Hi Ian,

On 18/09/2019 11:41, Ian Jackson wrote:
> Julien Grall writes ("Re: [PATCH] create-diff-object: more precisely identify .rodata sections"):
>> On 18/09/2019 10:52, Wieczorkiewicz, Pawel wrote:
>>> $ scripts/./add_maintainers.pl -d ~/git/livepatch-build-tools
>>
>> '-d' only tells you where the patches files are. The script will look up for the
>> MAINTAINERS file in the current directory.
> 
> Hmmm.  I wonder if we could detect this situation somehow.  This will
> be a common user error I think.
I think it would be possible for patch modifying file. We could check whether 
the file modified exist in the repo. Though, I am not sure how difficult it 
would be to implement.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH] create-diff-object: more precisely identify .rodata sections

[Wieczorkiewicz, Pawel]

> On 18. Sep 2019, at 12:41, Ian Jackson <ian.jackson@citrix.com> wrote:
> 
> Julien Grall writes ("Re: [PATCH] create-diff-object: more precisely identify .rodata sections"):
>> On 18/09/2019 10:52, Wieczorkiewicz, Pawel wrote:
>>> $ scripts/./add_maintainers.pl -d ~/git/livepatch-build-tools
>> 
>> '-d' only tells you where the patches files are. The script will look up for the 
>> MAINTAINERS file in the current directory.
> 
> Hmmm.  I wonder if we could detect this situation somehow.  This will
> be a common user error I think.
> 

I should have looked twice before sending the patch out. But, what would be very helpful for me
is additional option to the add_maintainers.pl script like: -m ./MAINTAINERS

> Ian.

Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




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

Re: [Xen-devel] [PATCH] create-diff-object: more precisely identify .rodata sections

[Ian Jackson]

Julien Grall writes ("Re: [PATCH] create-diff-object: more precisely identify .rodata sections"):
> On 18/09/2019 10:52, Wieczorkiewicz, Pawel wrote:
> > $ scripts/./add_maintainers.pl -d ~/git/livepatch-build-tools
> 
> '-d' only tells you where the patches files are. The script will look up for the 
> MAINTAINERS file in the current directory.

Hmmm.  I wonder if we could detect this situation somehow.  This will
be a common user error I think.

Ian.

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

Re: [Xen-devel] [PATCH] create-diff-object: more precisely identify .rodata sections

[Julien Grall]

Hi,

On 18/09/2019 10:52, Wieczorkiewicz, Pawel wrote:
> 
> 
>> On 18. Sep 2019, at 11:49, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 18.09.2019 09:35, Pawel Wieczorkiewicz wrote:
>>> This is needed for more precise patchability verification.
>>> Only non-special .rodata sections should be subject
>>> for such a non-referenced check in kpatch_verify_patchability().
>>> Current check (non-standard, non-rela, non-debug) is too weak and
>>> allows also non-rodata sections without referenced symbols to slip
>>> through.
>>>
>>> Detect .rodata section by checking section's type (SHT_PROGBITS),
>>> flags (no exec, no write) and finally name prefix.
>>>
>>> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
>>> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
>>> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
>>> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
>>> ---
>>> common.c             |  7 +++++++
>>> common.h             |  1 +
>>> create-diff-object.c | 13 ++++++-------
>>> 3 files changed, 14 insertions(+), 7 deletions(-)
>>
>> Seeing that I have been Cc-ed here - what tree is this against? I
>> don't recognize the file names as anything I'm a maintainer for.
>>
>> Jan
> 
> You have been probably added because I have used the following command:
> 
> $ scripts/./add_maintainers.pl -d ~/git/livepatch-build-tools

'-d' only tells you where the patches files are. The script will look up for the 
MAINTAINERS file in the current directory.

 From you command line, it would be xen.git. So it is not normal the wrong 
maintainers are CCed.

What you want is:

42sh> cd livepatch-build-tools
42sh> ../xen/scripts/add_maintainers.pl -d .

Note that you would need the patch [1] in order to get the script working.

Cheers,

[1] https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg01139.html

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH] create-diff-object: more precisely identify .rodata sections

[Wieczorkiewicz, Pawel]

> On 18. Sep 2019, at 11:49, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 18.09.2019 09:35, Pawel Wieczorkiewicz wrote:
>> This is needed for more precise patchability verification.
>> Only non-special .rodata sections should be subject
>> for such a non-referenced check in kpatch_verify_patchability().
>> Current check (non-standard, non-rela, non-debug) is too weak and
>> allows also non-rodata sections without referenced symbols to slip
>> through.
>> 
>> Detect .rodata section by checking section's type (SHT_PROGBITS),
>> flags (no exec, no write) and finally name prefix.
>> 
>> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
>> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
>> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
>> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
>> ---
>> common.c             |  7 +++++++
>> common.h             |  1 +
>> create-diff-object.c | 13 ++++++-------
>> 3 files changed, 14 insertions(+), 7 deletions(-)
> 
> Seeing that I have been Cc-ed here - what tree is this against? I
> don't recognize the file names as anything I'm a maintainer for.
> 
> Jan

You have been probably added because I have used the following command:

$ scripts/./add_maintainers.pl -d ~/git/livepatch-build-tools

@Lars: could you confirm?

Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




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

Re: [Xen-devel] [PATCH] create-diff-object: more precisely identify .rodata sections

[Jan Beulich]

On 18.09.2019 09:35, Pawel Wieczorkiewicz wrote:
> This is needed for more precise patchability verification.
> Only non-special .rodata sections should be subject
> for such a non-referenced check in kpatch_verify_patchability().
> Current check (non-standard, non-rela, non-debug) is too weak and
> allows also non-rodata sections without referenced symbols to slip
> through.
> 
> Detect .rodata section by checking section's type (SHT_PROGBITS),
> flags (no exec, no write) and finally name prefix.
> 
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
> ---
>  common.c             |  7 +++++++
>  common.h             |  1 +
>  create-diff-object.c | 13 ++++++-------
>  3 files changed, 14 insertions(+), 7 deletions(-)

Seeing that I have been Cc-ed here - what tree is this against? I
don't recognize the file names as anything I'm a maintainer for.

Jan

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

[Xen-devel] [PATCH] create-diff-object: more precisely identify .rodata sections

[Pawel Wieczorkiewicz]

This is needed for more precise patchability verification.
Only non-special .rodata sections should be subject
for such a non-referenced check in kpatch_verify_patchability().
Current check (non-standard, non-rela, non-debug) is too weak and
allows also non-rodata sections without referenced symbols to slip
through.

Detect .rodata section by checking section's type (SHT_PROGBITS),
flags (no exec, no write) and finally name prefix.

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
---
 common.c             |  7 +++++++
 common.h             |  1 +
 create-diff-object.c | 13 ++++++-------
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/common.c b/common.c
index 0ddc9fa..8f553ea 100644
--- a/common.c
+++ b/common.c
@@ -249,6 +249,13 @@ int is_text_section(struct section *sec)
 		(sec->sh.sh_flags & SHF_EXECINSTR));
 }
 
+int is_rodata_section(struct section *sec)
+{
+	return sec->sh.sh_type == SHT_PROGBITS &&
+	       !(sec->sh.sh_flags & (SHF_EXECINSTR | SHF_WRITE)) &&
+	       !strncmp(sec->name, ".rodata", 7);
+}
+
 int is_debug_section(struct section *sec)
 {
 	char *name;
diff --git a/common.h b/common.h
index 7c6fb73..b6489db 100644
--- a/common.h
+++ b/common.h
@@ -159,6 +159,7 @@ struct symbol *find_symbol_by_index(struct list_head *list, size_t index);
 struct symbol *find_symbol_by_name(struct list_head *list, const char *name);
 
 int is_text_section(struct section *sec);
+int is_rodata_section(struct section *sec);
 int is_debug_section(struct section *sec);
 int is_rela_section(struct section *sec);
 int is_standard_section(struct section *sec);
diff --git a/create-diff-object.c b/create-diff-object.c
index e4592a6..2f0e162 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1672,13 +1672,12 @@ static void kpatch_verify_patchability(struct kpatch_elf *kelf)
 		}
 
 		if (sec->include) {
-			if (!is_standard_section(sec) && !is_rela_section(sec) &&
-			    !is_debug_section(sec) && !is_special_section(sec)) {
-				if (!is_referenced_section(sec, kelf)) {
-					log_normal("section %s included, but not referenced\n",
-						   sec->name);
-					errs++;
-				}
+			if (is_rodata_section(sec) &&
+			    !is_special_section(sec) &&
+			    !is_referenced_section(sec, kelf)) {
+				log_normal(".rodata section %s included, but not referenced\n",
+					   sec->name);
+				errs++;
 			}
 
 			/* Check if a RELA section does not contain any entries with
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




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