Re: [PATCH v9 11/27] xsplice: Implement payload loading

From: "Jan Beulich" <JBeulich@suse.com>
To: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Keir Fraser <keir@xen.org>,
	ross.lagerwall@citrix.com, andrew.cooper3@citrix.com,
	mpohlack@amazon.de, Julien Grall <julien.grall@arm.com>,
	sasha.levin@oracle.com, xen-devel@lists.xenproject.org
Subject: Re: [PATCH v9 11/27] xsplice: Implement payload loading
Date: Wed, 27 Apr 2016 01:57:32 -0600	[thread overview]
Message-ID: <57208D0C02000078000E630A@prv-mh.provo.novell.com> (raw)
In-Reply-To: <20160427014654.GA26540@localhost.localdomain>

>>> On 27.04.16 at 03:47, <konrad.wilk@oracle.com> wrote:
>> > +static int move_payload(struct payload *payload, struct xsplice_elf *elf)
>> > +{
> .. snip..
>> > +    /* Compute size of different regions. */
>> > +    for ( i = 1; i < elf->hdr->e_shnum; i++ )
>> > +    {
>> > +        if ( (elf->sec[i].sec->sh_flags & (SHF_ALLOC|SHF_EXECINSTR)) ==
>> > +             (SHF_ALLOC|SHF_EXECINSTR) )
>> > +            calc_section(&elf->sec[i], &payload->text_size, &offset[i]);
>> 
>> This silently accepts writable text sections, yet the portion of the
>> memory this gets placed in will be mapped RX.
> 
> I am not sure I follow. We only accept if sh_flags have AX. Not WAX?
> How am I accepting writable text sections?

Because the & masks off SHF_WRITE, i.e. you only check that
SHF_ALLOC and SHF_EXECINSTR are set, but not that SHF_WRITE
is clear.

>> > +        else if ( (elf->sec[i].sec->sh_flags & SHF_ALLOC) &&
>> > +                  !(elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&
>> > +                  (elf->sec[i].sec->sh_flags & SHF_WRITE) )
>> > +            calc_section(&elf->sec[i], &payload->rw_size, &offset[i]);
>> > +        else if ( (elf->sec[i].sec->sh_flags & SHF_ALLOC) &&
>> > +                  !(elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&
>> > +                  !(elf->sec[i].sec->sh_flags & SHF_WRITE) )
>> > +            calc_section(&elf->sec[i], &payload->ro_size, &offset[i]);
>> > +        else if ( !elf->sec[i].sec->sh_flags ||
>> > +                  (elf->sec[i].sec->sh_flags & SHF_EXECINSTR) ||
>> > +                  (elf->sec[i].sec->sh_flags & SHF_MASKPROC) )
>> > +            /* Do nothing.*/;
>> > +        else if ( (elf->sec[i].sec->sh_flags & SHF_ALLOC) &&
>> > +                  (elf->sec[i].sec->sh_type == SHT_NOBITS) )
>> > +        {
>> > +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Not supporting %s section!\n",
>> > +                    elf->name, elf->sec[i].name);
>> > +            rc = -EOPNOTSUPP;
>> > +            goto out;
>> > +        }
>> 
>> I saw this in the changelog, but I don't really understand these last
>> two conditionals. Wouldn't you want to bail on _any_ sections which
> 
> The first (/Do nothing/) is for sections such as .rela.* (which we can
> ditch after we are done), .symtab, .strtab (for which in later patches in
> build_symbol_table construct a copy), and:
> 
> [ 1] .note.gnu.build-i NOTE 0000000000000000  00000040
>        0000000000000024  0000000000000000   A       0     0     4
> 
> which value we just copy in struct payload->id.
> (also in later patch).

All of which would fall under the "ignore sections with SHF_ALLOC
clear" rule, as mentioned ...

>> have SHF_ALLOC set but don't get mapped to one of the three
>> blocks? And wouldn't you (silently) ignore any sections with SHF_ALLOC
>> clear?

... here.

>> > +int xsplice_elf_resolve_symbols(struct xsplice_elf *elf)
>> > +{
>> > +    unsigned int i;
>> > +    int rc = 0;
>> > +
>> > +    ASSERT(elf->sym);
>> > +
>> > +    for ( i = 1; i < elf->nsym; i++ )
>> > +    {
>> > +        unsigned int idx = elf->sym[i].sym->st_shndx;
>> > +        Elf_Sym *sym = (Elf_Sym *)elf->sym[i].sym;
>> 
>> Well, I admit that this is the more straightforward solution, but it
>> opens up all of what sym points to for writing. I.e. I'd have
>> considered it much better to really only do the casting away of
>> const in the one spot where you need it (see below).
> 
> OK. That may become a bit cumbersome. We would have in the later
> patches (xsplice,symbols: Implement symbol name resolution on addres)
> the SHN_UNDEF doing symbol lookup. And that one tries to set
> sym->st_value twice.
> 
> I can certainly cast it twice there, and then once in the default
> case if you would like.

How about calculating the new value into a local variable, and doing
the cast needed for the assignment just once after the switch()?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel