perf jit: genelf makes assumptions about endian
PATCH 1/1 perf jit: genelf makes assumptions about endian
Commit 9b07e27f88b9 ("perf inject: Add jitdump mmap injection support") incorrectly assumed that PowerPC is big endian only.
Simplify things by consolidating the define of GEN_ELF_ENDIAN and checking for __BYTE_ORDER == __BIG_ENDIAN.
The PowerPC checks were also incorrect, they do not match what gcc emits. We should first look for __powerpc64__, then __powerpc__.
---
tools/perf/util/genelf.h +10 -14
@@ -9,36 +9,32 @@ int jit_add_debug_info(Elf *e, uint64_t code_addr, void *debug, int nr_debug_ent#if defined(__arm__)#define GEN_ELF_ARCH EM_ARM -#define GEN_ELF_ENDIAN ELFDATA2LSB#define GEN_ELF_CLASS ELFCLASS32#elif defined(__aarch64__)#define GEN_ELF_ARCH EM_AARCH64 -#define GEN_ELF_ENDIAN ELFDATA2LSB#define GEN_ELF_CLASS ELFCLASS64#elif defined(__x86_64__)#define GEN_ELF_ARCH EM_X86_64 -#define GEN_ELF_ENDIAN ELFDATA2LSB#define GEN_ELF_CLASS ELFCLASS64#elif defined(__i386__)#define GEN_ELF_ARCH EM_386 -#define GEN_ELF_ENDIAN ELFDATA2LSB#define GEN_ELF_CLASS ELFCLASS32 -#elif defined(__ppcle__) -#define GEN_ELF_ARCH EM_PPC -#define GEN_ELF_ENDIAN ELFDATA2LSB -#define GEN_ELF_CLASS ELFCLASS64 -#elif defined(__powerpc__) -#define GEN_ELF_ARCH EM_PPC64 -#define GEN_ELF_ENDIAN ELFDATA2MSB -#define GEN_ELF_CLASS ELFCLASS64 -#elif defined(__powerpcle__) +#elif defined(__powerpc64__)#define GEN_ELF_ARCH EM_PPC64 -#define GEN_ELF_ENDIAN ELFDATA2LSB#define GEN_ELF_CLASS ELFCLASS64 +#elif defined(__powerpc__) +#define GEN_ELF_ARCH EM_PPC +#define GEN_ELF_CLASS ELFCLASS32#else #error "unsupported architecture"#endif +#if __BYTE_ORDER == __BIG_ENDIAN +#define GEN_ELF_ENDIAN ELFDATA2MSB +#else +#define GEN_ELF_ENDIAN ELFDATA2LSB +#endif +#if GEN_ELF_CLASS ==ELFCLASS64 #define elf_newehdr elf64_newehdr#define elf_getshdr elf64_getshdr
mailman From rewriting [was perf jit: genelf makes assumptions about endian]
Hi Stephen,
Quoted 4 lines
From the mail that Anton has sent to linuxppc-dev:
From: Anton Blanchard via Linuxppc-dev <[email protected]>
Reply-To: Anton Blanchard <[email protected]>
Do you know why mailman would be re-writing From: there? It's confusing patchwork, as multiple mails are now coming from that address.
Anton: did you do anything special when sending that mail?
Cheers,
Jeremy
Re: mailman From rewriting [was perf jit: genelf makes assumptions about endian]
Hi Jeremy,
Quoted 8 lines · On Tue, 29 Mar 2016 16:56:58 +0800 Jeremy Kerr <[email protected]> wrote:
From the mail that Anton has sent to linuxppc-dev:
Quoted 2 lines
From: Anton Blanchard via Linuxppc-dev <[email protected]>
Reply-To: Anton Blanchard <[email protected]>Do you know why mailman would be re-writing From: there? It's confusing patchwork, as multiple mails are now coming from that address.
Yep, Anton posts from samba.org. They publish a DMARC policy that breaks mailing lists. The best thing we can do is to do the above rewrite of the From header.
The alternative is that all our Yahoo, Gmail and Hotmail subscribers (at least) would bounce the email.
So this rewriting will happen for any From: line that has an address that has such a DMARC policy. We have had posts from Yahoo subscribers bounced in the past.
Signature · 2 lines
Cheers, Stephen Rothwell
Re: mailman From rewriting [was perf jit: genelf makes assumptions about endian]
Quoted 16 lines · Stephen Rothwell
Hi Jeremy,
Quoted 8 lines · On Tue, 29 Mar 2016 16:56:58 +0800 Jeremy Kerr <[email protected]> wrote:
From the mail that Anton has sent to linuxppc-dev:
Quoted 2 lines
From: Anton Blanchard via Linuxppc-dev <[email protected]>
Reply-To: Anton Blanchard <[email protected]>Do you know why mailman would be re-writing From: there? It's confusing patchwork, as multiple mails are now coming from that address.
Yep, Anton posts from samba.org. They publish a DMARC policy that breaks mailing lists. The best thing we can do is to do the above rewrite of the From header.
Quoted 6 lines
The alternative is that all our Yahoo, Gmail and Hotmail subscribers (at least) would bounce the email.
So this rewriting will happen for any From: line that has an address that has such a DMARC policy. We have had posts from Yahoo subscribers bounced in the past.
[adding patchwork list to cc]
So it sounds like this is just something we're going to have to live with.
It's causing problems on patchwork because patchwork only uses the email address to lookup a user. This is causing patches to be misattributed on ozlabs.org, eg:
http://patchwork.ozlabs.org/project/linuxppc-dev/list/?submitter=68366&state=*
Paul was the first person to have a From header rewritten, eg to:
From: Paul Mackerras via Linuxppc-dev <[email protected]>
And so in the patchwork database the user "[email protected]" has a name of "Paul Mackerras via Linuxppc-dev".
The subsequent two patches (ppc4xx-rng and perf jit) are from different people, who also had their From address rewritten due to DMARC.
So I think patchwork needs to:
- detect mails that have been rewritten due to DMARC
- AFAICS the only way to do this is to notice that the From address is the same as the list address.
- when it sees those mails, use the Reply-To header instead.
cheers
Re: mailman From rewriting [was perf jit: genelf makes assumptions about endian]
In reply to Stephen RothwellHi Stephen,
Quoted 5 lines
Quoted 2 lines
Do you know why mailman would be re-writing From: there? It's confusing patchwork, as multiple mails are now coming from that address.
Yep, Anton posts from samba.org. They publish a DMARC policy that breaks mailing lists.
(╯°□°)╯︵ ┻━━┻
This also breaks git-am:
[jk@pudge linux]$ git am incoming.eml
Applying: perf jit: genelf makes assumptions about endian [jk@pudge linux]$ git log --format='format:%an <%ae>' -1 Anton Blanchard via Linuxppc-dev <[email protected]>
Quoted 1 line
The best thing we can do is to do the above rewrite of the From header.
OK, it looks like we're stuck either way with DMARC. Could we make this a little more tolerable by stashing the original From: value in a new header? I know it's already in Reply-To, but that could also be set by arbitrary other (non-mailman-DMARC-rewrite) sources.
Alternatively, if there's some other way to tell that this a mail has been rewritten, we can know to use Reply-To in preference to From.
Otherwise, I guess we could require that *all patch submitters* put their From: line in the content of their mails, as git send-email does when user != author. But that's a little less-than-optimal.
Cheers,
Jeremy
Re: mailman From rewriting [was perf jit: genelf makes assumptions about endian]
Hi Jeremy,
Quoted 8 lines · On Wed, 30 Mar 2016 13:30:12 +0800 Jeremy Kerr <[email protected]> wrote:
Quoted 6 lines
Quoted 3 lines
Do you know why mailman would be re-writing From: there? It's confusing patchwork, as multiple mails are now coming from that address. =20
=20
Yep, Anton posts from samba.org. They publish a DMARC policy that breaks mailing lists. =20=20
(=E2=95=AF=C2=B0=E2=96=A1=C2=B0)=E2=95=AF=EF=B8=B5 =E2=94=BB=E2=94=81=E2=
=94=81=E2=94=BB
Yes :-(
Quoted 6 lines
This also breaks git-am:
=20
[jk@pudge linux]$ git am incoming.eml
Applying: perf jit: genelf makes assumptions about endian [jk@pudge linux]$ git log --format=3D'format:%an <%ae>' -1 Anton Blanchard via Linuxppc-dev <[email protected]>
Of course :-(
Quoted 1 line
Quoted 1 line
The best thing we can do is to do the above rewrite of the From header.=
=20
Quoted 8 lines
=20
OK, it looks like we're stuck either way with DMARC. Could we make this a little more tolerable by stashing the original From: value in a new header? I know it's already in Reply-To, but that could also be set by arbitrary other (non-mailman-DMARC-rewrite) sources.
=20
Alternatively, if there's some other way to tell that this a mail has been rewritten, we can know to use Reply-To in preference to From.
Well, as Michael pointed out, the From header will have "via <list name>" in it ... and also, the address part of the From header will be the list address (unless people get even more creative with that address).
Quoted 3 lines
Otherwise, I guess we could require that *all patch submitters* put their From: line in the content of their mails, as git send-email does when user !=3D author. But that's a little less-than-optimal.
And hopeful :-)
--=20
Cheers,
Stephen Rothwell
Re: mailman From rewriting [was perf jit: genelf makes assumptions about endian]
In reply to Jeremy KerrQuoted 6 lines · Jeremy Kerr
Quoted 5 lines
Quoted 2 lines
Do you know why mailman would be re-writing From: there? It's confusing patchwork, as multiple mails are now coming from that address.
Yep, Anton posts from samba.org. They publish a DMARC policy that breaks mailing lists.
Quoted 8 lines
(╯°□°)╯︵ ┻━━┻
This also breaks git-am:
[jk@pudge linux]$ git am incoming.eml
Applying: perf jit: genelf makes assumptions about endian [jk@pudge linux]$ git log --format='format:%an <%ae>' -1 Anton Blanchard via Linuxppc-dev <[email protected]>
Indeed.
Personally I only apply patches from patchwork, so if it could get the sender right (which I think it needs to anyway), then I'd be OK.
Quoted 6 lines
Quoted 1 line
The best thing we can do is to do the above rewrite of the From header.
OK, it looks like we're stuck either way with DMARC. Could we make this a little more tolerable by stashing the original From: value in a new header? I know it's already in Reply-To, but that could also be set by arbitrary other (non-mailman-DMARC-rewrite) sources.
That'd be nice, or even just an X-did-DMARC-Rewrite: True.
Quoted 2 lines
Alternatively, if there's some other way to tell that this a mail has been rewritten, we can know to use Reply-To in preference to From.
I think checking for mails from the list address should work in practice. And it has the advantage that it works with the existing versions of mailman. If we need a new mechanism then we have to wait to get patched mailman in the wild.
Quoted 3 lines
Otherwise, I guess we could require that *all patch submitters* put their From: line in the content of their mails, as git send-email does when user != author. But that's a little less-than-optimal.
I had a quick look at git-send-email/git-format-patch and there doesn't seem to be any way to force a From line. So I think that's a medium term issue that the git folks will have to look at fixing. Maybe they've already noticed, I'm not on the git list.
cheers
Re: mailman From rewriting [was perf jit: genelf makes assumptions about endian]
In reply to Jeremy KerrQuoted 6 lines · Jeremy Kerr
Quoted 5 lines
Quoted 2 lines
Do you know why mailman would be re-writing From: there? It's confusing patchwork, as multiple mails are now coming from that address.
Yep, Anton posts from samba.org. They publish a DMARC policy that breaks mailing lists.
Quoted 8 lines
(╯°□°)╯︵ ┻━━┻
This also breaks git-am:
[jk@pudge linux]$ git am incoming.eml
Applying: perf jit: genelf makes assumptions about endian [jk@pudge linux]$ git log --format='format:%an <%ae>' -1 Anton Blanchard via Linuxppc-dev <[email protected]>
Actually one thing that makes this less of a problem, is that if you get the mail directly then the From is fine.
ie. for this patch I was on Cc, so the list didn't send me a copy, I just got it straight from Anton. So if I was wanting to git-am this mail I'd be fine.
Not saying that's a solution, but it's something.
cheers
Quoted 11 lines · Anton Blanchard
Commit
9b07e27f88b9("perf inject: Add jitdump mmap injection support") incorrectly assumed that PowerPC is big endian only.Simplify things by consolidating the define of
GEN_ELF_ENDIANand checking for__BYTE_ORDER==__BIG_ENDIAN.The PowerPC checks were also incorrect, they do not match what gcc emits. We should first look for
__powerpc64__, then__powerpc__.Fixes 9b07e27f88b9 ("perf inject: Add jitdump mmap injection support")Signed-off-by Anton Blanchard <[email protected]>
The diff's a little hard to read because you're pulling the endian logic out, if I remove that I get something like:
Quoted 9 lines
#elif
defined(__i386__)
#defineGEN_ELF_ARCHEM_386
#defineGEN_ELF_CLASSELFCLASS32tools/perf/util/genelf.h +0
-6-#elif defined(__ppcle__) -#define GEN_ELF_ARCH EM_PPC -#define GEN_ELF_CLASS ELFCLASS64 -#elif defined(__powerpc__) -#define GEN_ELF_ARCH EM_PPC64 -#define GEN_ELF_CLASS ELFCLASS64
tools/perf/util/genelf.h int jit_add_debug_info(Elf *e, uint64_t code_addr, void *debug, int nr_debug_ent · line 30
Quoted 10 lines
tools/perf/util/genelf.h +4
-1-#elif defined(__powerpcle__) +#elif defined(__powerpc64__)#define GEN_ELF_ARCH EM_PPC64#define GEN_ELF_CLASS ELFCLASS64 +#elif defined(__powerpc__) +#define GEN_ELF_ARCH EM_PPC +#define GEN_ELF_CLASS ELFCLASS32#else #error "unsupported architecture"#endif
Which looks correct to me.
And the consolidation of the endian logic is "obviously correct", so:
cheers
Em Wed, Mar 30, 2016 at 01:38:20PM +1100, Michael Ellerman escreveu:
Quoted 8 lines · Anton Blanchard
Quoted 7 lines
Commit
9b07e27f88b9("perf inject: Add jitdump mmap injection support") incorrectly assumed that PowerPC is big endian only.Simplify things by consolidating the define of
GEN_ELF_ENDIANand checking for__BYTE_ORDER==__BIG_ENDIAN.
Quoted 8 lines
Quoted 5 lines
The PowerPC checks were also incorrect, they do not match what gcc emits. We should first look for
__powerpc64__, then__powerpc__.Fixes 9b07e27f88b9 ("perf inject: Add jitdump mmap injection support")Signed-off-by Anton Blanchard <[email protected]>The diff's a little hard to read because you're pulling the endian logic out, if I remove that I get something like:
Yeah, I'm taking this patch, but would be better next time to break it down in two, one doing the reorg, the other doing the actual fix...
Thanks,
- Arnaldo
Quoted 27 lines
Quoted 9 lines
#elif
defined(__i386__)
#defineGEN_ELF_ARCHEM_386
#defineGEN_ELF_CLASSELFCLASS32tools/perf/util/genelf.h +0
-6-#elif defined(__ppcle__) -#define GEN_ELF_ARCH EM_PPC -#define GEN_ELF_CLASS ELFCLASS64 -#elif defined(__powerpc__) -#define GEN_ELF_ARCH EM_PPC64 -#define GEN_ELF_CLASS ELFCLASS64tools/perf/util/genelf.h int jit_add_debug_info(Elf *e, uint64_t code_addr, void *debug, int nr_debug_ent · line 30
Quoted 10 lines
tools/perf/util/genelf.h +4
-1-#elif defined(__powerpcle__) +#elif defined(__powerpc64__)#define GEN_ELF_ARCH EM_PPC64#define GEN_ELF_CLASS ELFCLASS64 +#elif defined(__powerpc__) +#define GEN_ELF_ARCH EM_PPC +#define GEN_ELF_CLASS ELFCLASS32#else #error "unsupported architecture"#endif Which looks correct to me.
And the consolidation of the endian logic is "obviously correct", so:
Acked-by Michael Ellerman <[email protected]>cheers
tip-bot for Anton Blanchard
merge botperf jit: genelf makes assumptions about endian
Commit-ID: 9f56c092b99b40ce3cf4c6d0134ff7e513c9f1a6 Gitweb: http://git.kernel.org/tip/9f56c092b99b40ce3cf4c6d0134ff7e513c9f1a6 Author: Anton Blanchard <[email protected]> AuthorDate: Tue, 29 Mar 2016 17:59:44 +1100 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Wed, 30 Mar 2016 18:12:06 -0300
perf jit: genelf makes assumptions about endian
Commit 9b07e27f88b9 ("perf inject: Add jitdump mmap injection support") incorrectly assumed that PowerPC is big endian only.
Simplify things by consolidating the define of GEN_ELF_ENDIAN and checking for __BYTE_ORDER == __BIG_ENDIAN.
The PowerPC checks were also incorrect, they do not match what gcc emits. We should first look for __powerpc64__, then __powerpc__.
--- tools/perf/util/genelf.h | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-)
tools/perf/util/genelf.h +10 -14
@@ -9,36 +9,32 @@ int jit_add_debug_info(Elf *e, uint64_t code_addr, void *debug, int nr_debug_ent#if defined(__arm__)#define GEN_ELF_ARCH EM_ARM -#define GEN_ELF_ENDIAN ELFDATA2LSB#define GEN_ELF_CLASS ELFCLASS32#elif defined(__aarch64__)#define GEN_ELF_ARCH EM_AARCH64 -#define GEN_ELF_ENDIAN ELFDATA2LSB#define GEN_ELF_CLASS ELFCLASS64#elif defined(__x86_64__)#define GEN_ELF_ARCH EM_X86_64 -#define GEN_ELF_ENDIAN ELFDATA2LSB#define GEN_ELF_CLASS ELFCLASS64#elif defined(__i386__)#define GEN_ELF_ARCH EM_386 -#define GEN_ELF_ENDIAN ELFDATA2LSB#define GEN_ELF_CLASS ELFCLASS32 -#elif defined(__ppcle__) -#define GEN_ELF_ARCH EM_PPC -#define GEN_ELF_ENDIAN ELFDATA2LSB -#define GEN_ELF_CLASS ELFCLASS64 -#elif defined(__powerpc__) -#define GEN_ELF_ARCH EM_PPC64 -#define GEN_ELF_ENDIAN ELFDATA2MSB -#define GEN_ELF_CLASS ELFCLASS64 -#elif defined(__powerpcle__) +#elif defined(__powerpc64__)#define GEN_ELF_ARCH EM_PPC64 -#define GEN_ELF_ENDIAN ELFDATA2LSB#define GEN_ELF_CLASS ELFCLASS64 +#elif defined(__powerpc__) +#define GEN_ELF_ARCH EM_PPC +#define GEN_ELF_CLASS ELFCLASS32#else #error "unsupported architecture"#endif +#if __BYTE_ORDER == __BIG_ENDIAN +#define GEN_ELF_ENDIAN ELFDATA2MSB +#else +#define GEN_ELF_ENDIAN ELFDATA2LSB +#endif +#if GEN_ELF_CLASS ==ELFCLASS64 #define elf_newehdr elf64_newehdr#define elf_getshdr elf64_getshdr