Re: vdso handling

[Alan Modra <amodra@gmail.com>]

On Tue, Mar 18, 2014 at 03:09:58PM +0000, Metzger, Markus T wrote:
> diff --git a/bfd/elfcode.h b/bfd/elfcode.h
> index 20101be..601d7ea 100644
> --- a/bfd/elfcode.h
> +++ b/bfd/elfcode.h
> @@ -1616,7 +1616,7 @@ NAME(_bfd_elf,bfd_from_remote_memory)
>    bfd_byte *contents;
>    int err;
>    unsigned int i;
> -  bfd_vma loadbase;
> +  bfd_vma loadbase, shdr_begin, shdr_end;
>    bfd_boolean loadbase_set;
>  
>    /* Read in the ELF header in external format.  */
> @@ -1728,20 +1728,16 @@ NAME(_bfd_elf,bfd_from_remote_memory)
>      }
>  
>    /* Trim the last segment so we don't bother with zeros in the last page
> -     that are off the end of the file.  However, if the extra bit in that
> -     page includes the section headers, keep them.  */
> -  if ((bfd_vma) contents_size > last_phdr->p_offset + last_phdr->p_filesz
> -      && (bfd_vma) contents_size >= (i_ehdr.e_shoff
> -				     + i_ehdr.e_shnum * i_ehdr.e_shentsize))
> -    {
> -      contents_size = last_phdr->p_offset + last_phdr->p_filesz;
> -      if ((bfd_vma) contents_size < (i_ehdr.e_shoff
> -				     + i_ehdr.e_shnum * i_ehdr.e_shentsize))
> -	contents_size = i_ehdr.e_shoff + i_ehdr.e_shnum * i_ehdr.e_shentsize;
> -    }
> -  else
> +     that are off the end of the file.  */
> +  if ((bfd_vma) contents_size > last_phdr->p_offset + last_phdr->p_filesz)
>      contents_size = last_phdr->p_offset + last_phdr->p_filesz;
>  
> +  /* Keep the section headers.  */
> +  shdr_begin = i_ehdr.e_shoff;
> +  shdr_end = shdr_begin + i_ehdr.e_shnum * i_ehdr.e_shentsize;
> +  if (shdr_end > (bfd_vma) contents_size)
> +    contents_size = shdr_end;
> +

I don't think this is a good idea.  If/when bfd_from_remote_memory is
used for something other than the linux kernel vdso, we can't assume
the section headers are loaded.  The original code made the assumption
that the highest address loaded from a PT_LOAD header was rounded up
to a page, and that the page size could be inferred from p_align.
Here:
	segment_end = (i_phdrs[i].p_offset + i_phdrs[i].p_filesz
		       + i_phdrs[i].p_align - 1) & -i_phdrs[i].p_align;
	if (segment_end > (bfd_vma) contents_size)
	  contents_size = segment_end;

Now, p_align is generally set from the page size if using GNU ld, but
I'm wondering if your vdso somehow doesn't have that property.  Can
you show us your vdso readelf -e output?  If p_align isn't set to a
page, then the change in heuristic I envision is to make use of
elf_backend_data maxpagesize to figure out which parts of the image
might be loaded.  If that isn't enough then perhaps we should add
another parameter to bfd_from_remote_memory to allow its caller to
specify the end of the image.

> Would it be OK to send this patch as part of a GDB patch series with
> binutils-patches and you CC'ed?  Or do you want a separate patch
> only to binutils-patches?

I don't mind either way.  This part of bfd belongs to gdb, so gdb
maintainers really have the final say on patch approval.  I'm just
someone who happened to become interested in the problem..

-- 
Alan Modra
Australia Development Lab, IBM