Skip Menu |
 

History Show all quoted text — Show full headers
# Thu Jan 09 13:59:58 2014 <nalin@redhat.com> - Ticket created
Date: Thu, 9 Jan 2014 13:21:01 -0500
From: Nalin Dahyabhai <nalin@redhat.com>
To: krb5-bugs@mit.edu
Subject: Text relocations in iaesx86.s
Download (untitled) / with headers
text/plain 421B
On 32-bit systems, references to shuffle_mask from _iEncExpandKey128 and
_iEncExpandKey256 currently require that the runtime linker perform
relocations in the text section.

By switching to using offsets relative to the global offset table when
reading shuffle_mask, the attached patch frees the linker from having to
attempt to toggle the mapped text section from being writable to be
being executable.

Thanks,

Nalin
Download krb5-1.12-pic-aes-ni.patch
text/plain 1.4KiB

Message body is not shown because sender requested not to inline it.

# Thu Jan 09 16:55:03 2014 <nalin@redhat.com> - Correspondence added
Date: Thu, 9 Jan 2014 16:54:58 -0500
From: Nalin Dahyabhai <nalin@redhat.com>
To: rt@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #7815] AutoReply: Text relocations in iaesx86.s
RT-Send-Cc:
Download (untitled) / with headers
text/plain 200B
I've managed to remove the need for a PIC relocation by moving the
shuffle_mask definition into the text segment, as was suggested in a
meeting earlier today. Here's a revised patch.

Thanks,

Nalin
Download krb5-1.12-pic-aes-ni.patch
text/plain 898B

Message body is not shown because sender requested not to inline it.

# Thu Jan 09 18:51:32 2014 tlyu (Taylor Yu) - Correspondence added
To: rt@krbdev.MIT.EDU
Subject: Re: [krbdev.mit.edu #7815] AutoReply: Text relocations in iaesx86.s
From: Tom Yu <tlyu@MIT.EDU>
Date: Thu, 09 Jan 2014 18:51:30 -0500
RT-Send-Cc:
Download (untitled) / with headers
text/plain 296B
I dug around a bit, and .rodata might be more idomatic. (I think it
gets consolidated into the text segment at runtime.) Also, merely
using the register relative addressing mode seems to be enough to
avoid generating a runtime text relocation, even if shuffle_mask
remains in the data segment.
# Thu Jan 09 20:10:17 2014 tlyu (Taylor Yu) - Correspondence added
Download (untitled) / with headers
text/plain 64B
Also, why the change from movdqa to movdqu in the revised patch?
# Fri Jan 10 11:09:55 2014 <nalin@redhat.com> - Correspondence added
Date: Fri, 10 Jan 2014 11:09:52 -0500
From: Nalin Dahyabhai <nalin@redhat.com>
To: rt@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #7815] Text relocations in iaesx86.s
RT-Send-Cc:
Download (untitled) / with headers
text/plain 283B
On Thu, Jan 09, 2014 at 08:10:17PM -0500, Tom Yu via RT wrote:
Show quoted text
> Also, why the change from movdqa to movdqu in the revised patch?

I think at one point I had moved the variable to the end of the text
segment, so it wasn't aligned properly, causing the tests to hit faults
and break.
# Fri Jan 10 14:23:49 2014 <nalin@redhat.com> - Correspondence added
Date: Fri, 10 Jan 2014 14:23:46 -0500
From: Nalin Dahyabhai <nalin@redhat.com>
To: rt@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #7815] AutoReply: Text relocations in iaesx86.s
RT-Send-Cc:
Download (untitled) / with headers
text/plain 850B
On Thu, Jan 09, 2014 at 06:51:32PM -0500, Tom Yu via RT wrote:
Show quoted text
> I dug around a bit, and .rodata might be more idomatic. (I think it
> gets consolidated into the text segment at runtime.) Also, merely
> using the register relative addressing mode seems to be enough to
> avoid generating a runtime text relocation, even if shuffle_mask
> remains in the data segment.

I hadn't verified it in the linker, but it looks like you're right about
that - while the object file includes relocations , the shared library
doesn't.

Switching from using ebx (which
http://refspecs.linuxbase.org/elf/abi386-4.pdf says we need to preserve
for the caller, and which in the earlier revision was being used to hold
the location of global offset table) to using ecx (which we don't need
to preserve) also trims the changes down, resulting in the attached.

Thanks!
Download krb5-1.12-pic-aes-ni.patch
text/plain 740B

Message body is not shown because sender requested not to inline it.

# Fri Jan 10 14:38:47 2014 tlyu (Taylor Yu) - Correspondence added
To: rt@krbdev.MIT.EDU
Subject: Re: [krbdev.mit.edu #7815] AutoReply: Text relocations in iaesx86.s
From: Tom Yu <tlyu@MIT.EDU>
Date: Fri, 10 Jan 2014 14:38:44 -0500
RT-Send-Cc:
Download (untitled) / with headers
text/plain 446B
"nalin@redhat.com via RT" <rt-comment@krbdev.mit.edu> writes:

Show quoted text
> --- krb5-1.12/src/lib/crypto/builtin/aes/iaesx86.s
> +++ krb5-1.12/src/lib/crypto/builtin/aes/iaesx86.s
> @@ -224,6 +224,8 @@
> DD 07060504h
> DD 0B0A0908h
>
> +section .data
> +align 16
> byte_swap_16:
> DDQ 0x000102030405060708090A0B0C0D0E0F
>

I'm pretty sure those added directives are redundant if we're leaving
shuffle_mask in .data rather than moving it to .rodata.
# Fri Jan 10 15:18:16 2014 <nalin@redhat.com> - Correspondence added
Date: Fri, 10 Jan 2014 15:18:12 -0500
From: Nalin Dahyabhai <nalin@redhat.com>
To: rt@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #7815] AutoReply: Text relocations in iaesx86.s
RT-Send-Cc:
Download (untitled) / with headers
text/plain 714B
On Fri, Jan 10, 2014 at 02:38:47PM -0500, Tom Yu via RT wrote:
Show quoted text
> "nalin@redhat.com via RT" <rt-comment@krbdev.mit.edu> writes:
>
> > --- krb5-1.12/src/lib/crypto/builtin/aes/iaesx86.s
> > +++ krb5-1.12/src/lib/crypto/builtin/aes/iaesx86.s
> > @@ -224,6 +224,8 @@
> > DD 07060504h
> > DD 0B0A0908h
> >
> > +section .data
> > +align 16
> > byte_swap_16:
> > DDQ 0x000102030405060708090A0B0C0D0E0F
>
> I'm pretty sure those added directives are redundant if we're leaving
> shuffle_mask in .data rather than moving it to .rodata.

Yes, of course you're right, they are. Either we move shuffle_mask to
.rodata, or this hunk is redundant. I have a mild preference for using
.rodata, as you suggested earlier.
# Fri Jan 10 15:29:51 2014 tlyu (Taylor Yu) - Correspondence added
To: rt@krbdev.MIT.EDU
Subject: Re: [krbdev.mit.edu #7815] AutoReply: Text relocations in iaesx86.s
From: Tom Yu <tlyu@MIT.EDU>
Date: Fri, 10 Jan 2014 15:29:49 -0500
RT-Send-Cc:
Download (untitled) / with headers
text/plain 799B
"nalin@redhat.com via RT" <rt-comment@krbdev.mit.edu> writes:

Show quoted text
>> I'm pretty sure those added directives are redundant if we're leaving
>> shuffle_mask in .data rather than moving it to .rodata.
>
> Yes, of course you're right, they are. Either we move shuffle_mask to
> .rodata, or this hunk is redundant. I have a mild preference for using
> .rodata, as you suggested earlier.

I have a preference for leaving it in .data for now, for minimizing
the change and to match iaesx64.s. We can make a cleanup pass later
that moves stuff to .rodata and gets rid of some dead code and data.
I think shuffle_mask is the only data label actually used in that
file.

Proposed commit in the aesni-reloc branch on my GitHub fork:
https://github.com/tlyu/krb5/commit/3847aa109e8ff3f2781d53315f81e8d29ee35892
# Fri Jan 10 16:11:24 2014 <nalin@redhat.com> - Correspondence added
Date: Fri, 10 Jan 2014 16:11:21 -0500
From: Nalin Dahyabhai <nalin@redhat.com>
To: rt@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #7815] AutoReply: Text relocations in iaesx86.s
RT-Send-Cc:
Download (untitled) / with headers
text/plain 248B
On Fri, Jan 10, 2014 at 03:29:51PM -0500, Tom Yu via RT wrote:
Show quoted text
> Proposed commit in the aesni-reloc branch on my GitHub fork:
> https://github.com/tlyu/krb5/commit/3847aa109e8ff3f2781d53315f81e8d29ee35892

FWIW, that works on my test box.

Thanks!
# Fri Jan 10 16:25:53 2014 tlyu (Taylor Yu) - Given to tlyu (Taylor Yu)
# Fri Jan 10 16:25:53 2014 tlyu (Taylor Yu) - Target_Version 1.12.1 added
# Fri Jan 10 16:25:53 2014 tlyu (Taylor Yu) - Status changed from 'new' to 'review'
# Fri Jan 10 16:25:53 2014 tlyu (Taylor Yu) - Tags pullup added
# Fri Jan 10 16:25:53 2014 tlyu (Taylor Yu) - Correspondence added
From: tlyu@mit.edu
Subject: git commit
Download (untitled) / with headers
text/plain 423B

Avoid text relocations in iaesx86.s

Use PC-relative addressing to avoid runtime text relocations on i386.

Adapted patch from Nalin Dahyabhai.

https://github.com/krb5/krb5/commit/3847aa109e8ff3f2781d53315f81e8d29ee35892
Author: Tom Yu <tlyu@mit.edu>
Commit: 3847aa109e8ff3f2781d53315f81e8d29ee35892
Branch: master
src/lib/crypto/builtin/aes/iaesx86.s | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
# Fri Jan 10 22:15:33 2014 tlyu (Taylor Yu) - Comments added
Download (untitled) / with headers
text/plain 96B
There's some more context toward the end of https://bugzilla.redhat.com/show_bug.cgi?
id=1045699
# Mon Jan 13 15:16:13 2014 tlyu (Taylor Yu) - Status changed from 'review' to 'resolved'
# Mon Jan 13 15:16:13 2014 tlyu (Taylor Yu) - Version_Fixed 1.12.1 added
# Mon Jan 13 15:16:13 2014 tlyu (Taylor Yu) - Correspondence added
From: tlyu@mit.edu
Subject: git commit
Download (untitled) / with headers
text/plain 496B

Avoid text relocations in iaesx86.s

Use PC-relative addressing to avoid runtime text relocations on i386.

Adapted patch from Nalin Dahyabhai.

(cherry picked from commit 3847aa109e8ff3f2781d53315f81e8d29ee35892)

https://github.com/krb5/krb5/commit/782ced86e178a91163c4aad7a93de589d2e60854
Author: Tom Yu <tlyu@mit.edu>
Commit: 782ced86e178a91163c4aad7a93de589d2e60854
Branch: krb5-1.12
src/lib/crypto/builtin/aes/iaesx86.s | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
# Wed Dec 16 18:03:01 2015 tlyu (Taylor Yu) - CustomField pullup deleted