diff mbox

perf jit: genelf makes assumptions about endian

Message ID 20160329175944.33a211cc@kryten (mailing list archive)
State Not Applicable
Headers show

Commit Message

Anton Blanchard March 29, 2016, 6:59 a.m. UTC 
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__.

Fixes: 9b07e27f88b9 ("perf inject: Add jitdump mmap injection support")
Signed-off-by: Anton Blanchard <anton@samba.org>
---

Comments

Jeremy Kerr March 29, 2016, 8:56 a.m. UTC | #1 
Hi Stephen,

From the mail that Anton has sent to linuxppc-dev:

> From: Anton Blanchard via Linuxppc-dev <linuxppc-dev@lists.ozlabs.org>
> Reply-To: Anton Blanchard <anton@samba.org>

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
Stephen Rothwell March 29, 2016, 10:06 a.m. UTC | #2 
Hi Jeremy,

On Tue, 29 Mar 2016 16:56:58 +0800 Jeremy Kerr <jk@ozlabs.org> wrote:
>
> From the mail that Anton has sent to linuxppc-dev:
> 
> > From: Anton Blanchard via Linuxppc-dev <linuxppc-dev@lists.ozlabs.org>
> > Reply-To: Anton Blanchard <anton@samba.org>  
> 
> 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.
Michael Ellerman March 30, 2016, 2:38 a.m. UTC | #3 
On Tue, 2016-03-29 at 17:59 +1100, Anton Blanchard wrote:

> 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__.
> 
> Fixes: 9b07e27f88b9 ("perf inject: Add jitdump mmap injection support")
> Signed-off-by: Anton Blanchard <anton@samba.org>

The diff's a little hard to read because you're pulling the endian logic out,
if I remove that I get something like:

>  #elif defined(__i386__)
>  #define GEN_ELF_ARCH	EM_386
>  #define GEN_ELF_CLASS	ELFCLASS32
> -#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
> -#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 <mpe@ellerman.id.au>

cheers
Michael Ellerman March 30, 2016, 2:55 a.m. UTC | #4 
On Tue, 2016-03-29 at 21:06 +1100, Stephen Rothwell wrote:
> Hi Jeremy,
> 
> On Tue, 29 Mar 2016 16:56:58 +0800 Jeremy Kerr <jk@ozlabs.org> wrote:
> > 
> > From the mail that Anton has sent to linuxppc-dev:
> > 
> > > From: Anton Blanchard via Linuxppc-dev <linuxppc-dev@lists.ozlabs.org>
> > > Reply-To: Anton Blanchard <anton@samba.org>  
> > 
> > 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.

[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 <linuxppc-dev@lists.ozlabs.org>

And so in the patchwork database the user "linuxppc-dev@lists.ozlabs.org" 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
Jeremy Kerr March 30, 2016, 5:30 a.m. UTC | #5 
Hi Stephen,

>> 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 <linuxppc-dev@lists.ozlabs.org>

> 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
Stephen Rothwell March 30, 2016, 6:57 a.m. UTC | #6 
Hi Jeremy,

On Wed, 30 Mar 2016 13:30:12 +0800 Jeremy Kerr <jk@ozlabs.org> wrote:
>
> >> 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.  
> 
> (╯°□°)╯︵ ┻━━┻

Yes :-(

> 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 <linuxppc-dev@lists.ozlabs.org>

Of course :-(

> > 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.

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).

> 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.

And hopeful :-)
Michael Ellerman March 30, 2016, 9:03 a.m. UTC | #7 
On Wed, 2016-03-30 at 13:30 +0800, Jeremy Kerr wrote:
> > > 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 <linuxppc-dev@lists.ozlabs.org>

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.

> > 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.

> 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.

> 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
Michael Ellerman March 30, 2016, 9:08 a.m. UTC | #8 
On Wed, 2016-03-30 at 13:30 +0800, Jeremy Kerr wrote:
> > > 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 <linuxppc-dev@lists.ozlabs.org>

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
Arnaldo Carvalho de Melo March 30, 2016, 9:15 p.m. UTC | #9 
Em Wed, Mar 30, 2016 at 01:38:20PM +1100, Michael Ellerman escreveu:
> On Tue, 2016-03-29 at 17:59 +1100, Anton Blanchard wrote:
> 
> > 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__.
> > 
> > Fixes: 9b07e27f88b9 ("perf inject: Add jitdump mmap injection support")
> > Signed-off-by: Anton Blanchard <anton@samba.org>
> 
> 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
 
> >  #elif defined(__i386__)
> >  #define GEN_ELF_ARCH	EM_386
> >  #define GEN_ELF_CLASS	ELFCLASS32
> > -#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
> > -#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 <mpe@ellerman.id.au>
> 
> cheers
diff mbox

Patch

diff --git a/tools/perf/util/genelf.h b/tools/perf/util/genelf.h
index cd67e64..2fbeb59 100644
--- a/tools/perf/util/genelf.h
+++ b/tools/perf/util/genelf.h
@@ -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