Re: [PATCH] fix for loading modules from read-only memory area (Re: clean patch for i386-qemu port (Re: [PATCH] i386-qemu port))

[Robert Millan]

On Thu, Jun 25, 2009 at 03:53:28PM -0400, Pavel Roskin wrote:
> On Thu, 2009-06-25 at 01:10 +0200, Robert Millan wrote:
> > On Wed, Jun 24, 2009 at 03:00:32AM +0200, Robert Millan wrote:
> > > A possible solution to this could be to make grub_dl_load_core() create a
> > > copy of the module and work on the copy.  This could even be ifdef'ed,
> > > but I doubt the performance hit would be significant.
> > 
> > I found a better approach;  instead of copiing the whole module, we just
> > need to copy the symbol tab.  A small adjustment to each of the functions
> > that will access it (grub_dl_resolve_symbols and
> > grub_arch_dl_relocate_symbols) will make them use the copy instead of
> > the original.
> 
> Massive use of undef seems inelegant.  Maybe it's better to use
> distinctive names instead, e.g. target_Elf_Ehdr v.s. host_Elf_Ehdr?

You mean for efiemu?  Seems fine.  But I wouldn't use "target/host"
naming, since it doesn't match with existing usage of those names
elsewhere, which makes it very confusing.

How about "native/cross"?

> Also, I'm getting many warnings on i386-pc about redefined Elf_Ehdr when
> compiling on x86_64.

I'll have a look.

> Generally, I like the idea for i386-qemu, but for other architectures,
> the only change is the increased memory consumption.  I would be willing
> to accept this change if you could suggest some universal benefit for
> all platforms.
> 
> Otherwise, it would be better to use wrappers like get_header() and
> put_header() what would do grub_malloc() and grub_free() only if needed
> and return the original header otherwise.
> 
> I'm fine with adding the symtab field unconditionally, it doesn't cost
> much memory.

I agree there's no point in making all architectures do this.  However,
note that we'll probably see more architectures with read-only modules
in the future (other qemu ports, but also any port where GRUB runs
standalone).

I was thinking that we could just #ifdef the symtab allocation (and the
symtab field declaration as well).  Since it's just a few lines, I think
it'll look readable.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."