[Qemu-devel] [PATCH] linux-user: Do not subtract offset from end address

[Qemu-devel] [PATCH] linux-user: Do not subtract offset from end address

[Tom Musta]

When computing the upper address of a program segment, do not subtract the
offset from the virtual address; instead compute the sum of the virtual address
and the memory size.

Signed-off-by: Tom Musta <tommusta@gmail.com>
---

Please include this patch in QEMU 2.2.  

Commit a93934fecd4dffc9d4b452b670c9506be5dea30d injected a regression of Linux
User Mode that I was able to detect on PowerPC 64 (but not x86).  I suspect that
large page size on the host has something to do with it.  In any case, that commit
adjusted the lower address of a program segment by the program header's offset 
field.  However, it also inadvertantly adjusted the upper address by the offset also.

 linux-user/elfload.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 84123ba..e2596a4 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1824,7 +1824,7 @@ static void load_elf_image(const char *image_name, int image_fd,
             if (a < loaddr) {
                 loaddr = a;
             }
-            a += phdr[i].p_memsz;
+            a = phdr[i].p_vaddr + phdr[i].p_memsz;
             if (a > hiaddr) {
                 hiaddr = a;
             }
-- 
1.7.1

Re: [Qemu-devel] [PATCH] linux-user: Do not subtract offset from end address

[Riku Voipio]

On Thu, Nov 06, 2014 at 01:43:13PM -0600, Tom Musta wrote:
> When computing the upper address of a program segment, do not subtract the
> offset from the virtual address; instead compute the sum of the virtual address
> and the memory size.

Thanks, I'll test this and try to get it applied ASAP. Jonas, can you
have a look and provide your Acked-by/Tested-by ?

Riku

> Signed-off-by: Tom Musta <tommusta@gmail.com>
> ---
> 
> Please include this patch in QEMU 2.2.  
> 
> Commit a93934fecd4dffc9d4b452b670c9506be5dea30d injected a regression of Linux
> User Mode that I was able to detect on PowerPC 64 (but not x86).  I suspect that
> large page size on the host has something to do with it.  In any case, that commit
> adjusted the lower address of a program segment by the program header's offset 
> field.  However, it also inadvertantly adjusted the upper address by the offset also.
> 
>  linux-user/elfload.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 84123ba..e2596a4 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1824,7 +1824,7 @@ static void load_elf_image(const char *image_name, int image_fd,
>              if (a < loaddr) {
>                  loaddr = a;
>              }
> -            a += phdr[i].p_memsz;
> +            a = phdr[i].p_vaddr + phdr[i].p_memsz;
>              if (a > hiaddr) {
>                  hiaddr = a;
>              }
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH] linux-user: Do not subtract offset from end address

[Jonas Maebe]

On 07 Nov 2014, at 08:23, Riku Voipio wrote:

> On Thu, Nov 06, 2014 at 01:43:13PM -0600, Tom Musta wrote:
>> When computing the upper address of a program segment, do not  
>> subtract the
>> offset from the virtual address; instead compute the sum of the  
>> virtual address
>> and the memory size.
>
> Thanks, I'll test this and try to get it applied ASAP. Jonas, can you
> have a look and provide your Acked-by/Tested-by ?

Good catch! Sorry for not noticing that. I've verified and the patched  
version also still works with my ARM binary.

While looking at that, I noticed that the code under "#ifdef  
CONFIG_USE_FDPIC" in linux-user/elfload.c at line 1858 may need a  
similar adjustment as performed by my original patch. At least http://lxr.free-electrons.com/source/fs/binfmt_elf.c#L829 
  makes the offset adjustment both for binaries with and without a  
"load_bias". I'm not sure what this is for (some uCLinux-specific  
format?), nor do I have binaries that exercise this functionality, so  
I can't/won't provide a patch for this.

Thanks,


Jonas

>> Signed-off-by: Tom Musta <tommusta@gmail.com>
>> ---
>>
>> Please include this patch in QEMU 2.2.
>>
>> Commit a93934fecd4dffc9d4b452b670c9506be5dea30d injected a  
>> regression of Linux
>> User Mode that I was able to detect on PowerPC 64 (but not x86).  I  
>> suspect that
>> large page size on the host has something to do with it.  In any  
>> case, that commit
>> adjusted the lower address of a program segment by the program  
>> header's offset
>> field.  However, it also inadvertantly adjusted the upper address  
>> by the offset also.
>>
>> linux-user/elfload.c |    2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> index 84123ba..e2596a4 100644
>> --- a/linux-user/elfload.c
>> +++ b/linux-user/elfload.c
>> @@ -1824,7 +1824,7 @@ static void load_elf_image(const char  
>> *image_name, int image_fd,
>>             if (a < loaddr) {
>>                 loaddr = a;
>>             }
>> -            a += phdr[i].p_memsz;
>> +            a = phdr[i].p_vaddr + phdr[i].p_memsz;
>>             if (a > hiaddr) {
>>                 hiaddr = a;
>>             }
>> -- 
>> 1.7.1

Re: [Qemu-devel] [PATCH] linux-user: Do not subtract offset from end address

[Andreas Färber]

Am 06.11.2014 um 20:43 schrieb Tom Musta:
> When computing the upper address of a program segment, do not subtract the
> offset from the virtual address; instead compute the sum of the virtual address
> and the memory size.

Note that this reads a bit weird as both old and new code are adding,
not subtracting.

Regards,
Andreas

> 
> Signed-off-by: Tom Musta <tommusta@gmail.com>
> ---
> 
> Please include this patch in QEMU 2.2.  
> 
> Commit a93934fecd4dffc9d4b452b670c9506be5dea30d injected a regression of Linux
> User Mode that I was able to detect on PowerPC 64 (but not x86).  I suspect that
> large page size on the host has something to do with it.  In any case, that commit
> adjusted the lower address of a program segment by the program header's offset 
> field.  However, it also inadvertantly adjusted the upper address by the offset also.
> 
>  linux-user/elfload.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 84123ba..e2596a4 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1824,7 +1824,7 @@ static void load_elf_image(const char *image_name, int image_fd,
>              if (a < loaddr) {
>                  loaddr = a;
>              }
> -            a += phdr[i].p_memsz;
> +            a = phdr[i].p_vaddr + phdr[i].p_memsz;
>              if (a > hiaddr) {
>                  hiaddr = a;
>              }
> 


-- 
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 21284 AG Nürnberg

Re: [Qemu-devel] [PATCH] linux-user: Do not subtract offset from end address

[Tom Musta]

On 11/8/2014 6:22 PM, Andreas Färber wrote:
> Am 06.11.2014 um 20:43 schrieb Tom Musta:
>> When computing the upper address of a program segment, do not subtract the
>> offset from the virtual address; instead compute the sum of the virtual address
>> and the memory size.
> 
> Note that this reads a bit weird as both old and new code are adding,
> not subtracting.
> 
> Regards,
> Andreas
> 

I agree that it is not obvious from the patch, which needed one more line of
context:

            abi_ulong a = phdr[i].p_vaddr - phdr[i].p_offset;
            if (a < loaddr) {
                loaddr = a;
            }
            a = phdr[i].p_vaddr + phdr[i].p_memsz;
            if (a > hiaddr) {
                hiaddr = a;
            }

I think the description accurately captures what is being changed in the code.
But if you still disagree, I will reword and respin V2.

>>
>> Signed-off-by: Tom Musta <tommusta@gmail.com>
>> ---
>>
>> Please include this patch in QEMU 2.2.  
>>
>> Commit a93934fecd4dffc9d4b452b670c9506be5dea30d injected a regression of Linux
>> User Mode that I was able to detect on PowerPC 64 (but not x86).  I suspect that
>> large page size on the host has something to do with it.  In any case, that commit
>> adjusted the lower address of a program segment by the program header's offset 
>> field.  However, it also inadvertantly adjusted the upper address by the offset also.
>>
>>  linux-user/elfload.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> index 84123ba..e2596a4 100644
>> --- a/linux-user/elfload.c
>> +++ b/linux-user/elfload.c
>> @@ -1824,7 +1824,7 @@ static void load_elf_image(const char *image_name, int image_fd,
>>              if (a < loaddr) {
>>                  loaddr = a;
>>              }
>> -            a += phdr[i].p_memsz;
>> +            a = phdr[i].p_vaddr + phdr[i].p_memsz;
>>              if (a > hiaddr) {
>>                  hiaddr = a;
>>              }
>>
> 
>