| Message ID | 20190918073538.24707-1-wipawel@amazon.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | create-diff-object: more precisely identify .rodata sections | expand |
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
> 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
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 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.
> 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
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,
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,
> 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
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,
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
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
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
> 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
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,
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,
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