[PATCH v3] x86: Propery check PC16 reloc overflow in 16-bit mode instructions

Thu May 27 15:53:01 GMT 2021

On Thu, May 27, 2021 at 7:57 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 27.05.2021 15:46, H.J. Lu wrote:
> > On Thu, May 27, 2021 at 6:15 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 27.05.2021 15:08, H.J. Lu wrote:
> >>> On Thu, May 27, 2021 at 5:50 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 27.05.2021 14:29, H.J. Lu wrote:
> >>>>> On Thu, May 27, 2021 at 12:10 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>
> >>>>>> On 26.05.2021 16:16, H.J. Lu wrote:
> >>>>>>> On Tue, May 25, 2021 at 9:31 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>> On Tue, May 25, 2021 at 9:27 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>>
> >>>>>>>>> On 25.05.2021 18:11, H.J. Lu wrote:
> >>>>>>>>>> On Tue, May 25, 2021 at 9:08 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> On 25.05.2021 17:40, H.J. Lu wrote:
> >>>>>>>>>>>> On Tue, May 25, 2021 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On 25.05.2021 15:41, H.J. Lu wrote:
> >>>>>>>>>>>>>> On Mon, May 24, 2021 at 11:40 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> On 25.05.2021 01:42, H.J. Lu via Binutils wrote:
> >>>>>>>>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> >>>>>>>>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
> >>>>>>>>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> caused linker failure when building 16-bit program in a 32-bit ELF
> >>>>>>>>>>>>>>>> container.  Update GNU_PROPERTY_X86_FEATURE_2_USED with
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>  #define GNU_PROPERTY_X86_FEATURE_2_CODE16 (1U << 12)
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> as in
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> https://groups.google.com/g/x86-64-abi/c/UvvXWeHIGMA
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> to indicate that 16-bit mode instructions are used in the object to
> >>>>>>>>>>>>>>>> allow linker to properly perform relocation overflow check for 16-bit
> >>>>>>>>>>>>>>>> PC-relative relocations in 16-bit mode instructions.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> I certainly don't mind the introduction of this flag; I think its
> >>>>>>>>>>>>>>> introduction wants to be separated from the specific use in the
> >>>>>>>>>>>>>>> linker, not the lease because ...
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> 1. Update x86 assembler to always generate the GNU property note with
> >>>>>>>>>>>>>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 for .code16 in ELF object.
> >>>>>>>>>>>>>>>> 2. Update i386 and x86-64 linkers to use 16-bit PC16 relocations if
> >>>>>>>>>>>>>>>> input object is marked with GNU_PROPERTY_X86_FEATURE_2_CODE16.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> ... I don't think this is an appropriate step to take. The majority
> >>>>>>>>>>>>>>> of cases of 16-bit code use that I know of is in projects where this
> >>>>>>>>>>>>>>> is just a small portion of code, and the rest of the code is 32-
> >>>>>>>>>>>>>>> and/or 64-bit. By taking mere presence of a tiny bit of 16-bit code
> >>>>>>>>>>>>>>> as indication to relax overflow checking, you undermine the main
> >>>>>>>>>>>>>>> goal of the earlier change.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
> >>>>>>>>>>>>>> Author: Jan Beulich <jbeulich@suse.com>
> >>>>>>>>>>>>>> Date:   Mon Apr 26 10:41:35 2021 +0200
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>     x86: correct overflow checking for 16-bit PC-relative relocs
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> is technically correct according to psABIs.  But GNU assembler
> >>>>>>>>>>>>>> only generates PC16 relocations for 16-bit codes.  That is why
> >>>>>>>>>>>>>> we never ran into any PC16 relocation overflow before.  My change
> >>>>>>>>>>>>>> restores the old behavior only when input has 16-bit codes.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> When paying specific attention to resulting code size, people may
> >>>>>>>>>>>>> want to encode XBEGIN with 16-bit operand size in 32- or 64-bit
> >>>>>>>>>>>>> code. This not having worked properly is what initially triggered
> >>>>>>>>>>>>> me touching this area. And of course assembler programmers could
> >>>>>>>>>>>>> even cause data to have PC16 relocations attached to it.
> >>>>>>>>>>>>
> >>>>>>>>>>>> As long as it doesn't .code16, PC16 relocation overflow check won't be
> >>>>>>>>>>>> changed.
> >>>>>>>>>>>
> >>>>>>>>>>> Of course, but I expect typical use cases (kernels, hypervisors) to
> >>>>>>>>>>> contain small pieces of 16-bit code.
> >>>>>>>>>>
> >>>>>>>>>> And we never ran into any issues before.  My patch just restores
> >>>>>>>>>> the old behavior for them.
> >>>>>>>>>
> >>>>>>>>> And my point is that the old behavior was wrong (and will be again if
> >>>>>>>>> we go with your change).
> >>>>>>>>
> >>>>>>>> The odd behavior is correct for 16-bit codes.  My patch restores the
> >>>>>>>> old behavior only if input has 16-bit codes.
> >>>>>>>>
> >>>>>>>
> >>>>>>> This is the patch I am going to check in.
> >>>>>>
> >>>>>> Sadly you've sent all but v1 only as attachments, making commenting
> >>>>>> difficult. There are issues:
> >>>>>> 1) The workaround gets triggered from the handling of the actual
> >>>>>> .code* directives, so would come into effect even if no insn at all
> >>>>>> was emitted. Imo this should be tied to the specific case of a PC16
> >>>>>> relocation getting emitted for 16-bit code. This would also take
> >>>>>> care of 4) below.
> >>>>>
> >>>>> We can do this:
> >>>>>
> >>>>> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
> >>>>> index c17f4da16fe..8adf0fce0a6 100644
> >>>>> --- a/gas/config/tc-i386.c
> >>>>> +++ b/gas/config/tc-i386.c
> >>>>> @@ -2695,10 +2695,6 @@ static void
> >>>>>  set_code_flag (int value)
> >>>>>  {
> >>>>>    update_code_flag (value, 0);
> >>>>> -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> >>>>> -  if (value == CODE_16BIT)
> >>>>> -    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> >>>>> -#endif
> >>>>>  }
> >>>>>
> >>>>>  static void
> >>>>> @@ -2710,10 +2706,6 @@ set_16bit_gcc_code_flag (int new_code_flag)
> >>>>>    cpu_arch_flags.bitfield.cpu64 = 0;
> >>>>>    cpu_arch_flags.bitfield.cpuno64 = 1;
> >>>>>    stackop_size = LONG_MNEM_SUFFIX;
> >>>>> -#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> >>>>> -  if (new_code_flag == CODE_16BIT)
> >>>>> -    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> >>>>> -#endif
> >>>>>  }
> >>>>>
> >>>>>  static void
> >>>>> @@ -8784,6 +8776,10 @@ output_branch (void)
> >>>>>    else
> >>>>>      subtype = ENCODE_RELAX_STATE (COND_JUMP86, size);
> >>>>>    subtype |= code16;
> >>>>> +#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> >>>>> +  if (code16)
> >>>>> +    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
> >>>>> +#endif
> >>>>
> >>>> This would get us closer to what I have described, but doesn't go
> >>>> quite as far. However, GNU_PROPERTY_X86_FEATURE_2_CODE16 then is
> >>>> misleading and would probably want naming (and describing)
> >>>> differently.
> >>>
> >>> Do you have any suggestions?
> >>
> >> GNU_PROPERTY_X86_FEATURE_2_BOGUS_PC16_RELOC plus further limiting
> >> when the flag gets set (i.e. only when a PC16 reloc gets actually
> >> generated).
> >
> > How about GNU_PROPERTY_X86_FEATURE_2_PC16_CODE16?
>
> Fine with me - I don't care much about the name as long as it's
> not actively misleading.
>
> > My proposed patch should be sufficient.  If not, do you have a testcase?
>
> The decision whether to emit a relocation happens only during
> relaxation, aiui. IOW it is my understanding that a forward
> branch to a local symbol would also cause the flag to get set.

I may be able to avoid it.  But ...

> >>>>>> 2) Object files created by other tools (including older gas, which
> >>>>>> would include an incremental build after having updated binutils),
> >>>>>> or created with that note's emission suppressed (which, as said in
> >>>>>> bug 27753, shouldn't be emitted by default anyway) will still cause
> >>>>>> problems.
> >>>>>
> >>>>> GNU_PROPERTY_X86_FEATURE_2_CODE16 can't not be
> >>>>> suppressed.
> >>>>
> >>>> Of course it can, via -mx86-used-note=no. Or did you change this
> >>>> kind of silently as a "side effect" of your patch, without me
> >>>> even noticing? That would be evil - as said earlier (and reported
> >>>> as a bug), these notes shouldn't be there by default; even less
> >>>> so should they be there unconditionally.
> >>>
> >>> GNU_PROPERTY_X86_FEATURE_2_CODE16 is mandatory.
> >>> Otherwise, it won't work for seabios.
> >>
> >> Well, for both this and ...
> >>
> >>>> Plus even if it couldn't be suppressed, there would still be the
> >>>> issue with object files created by other / older tools.
> >>>
> >>> This is a workaround/compromise.  There is no perfect solution.
> >>> It works for seabios.
> >>
> >> ... this: Abusing relocations is something that may very well require
> >> projects to adjust their build environment, e.g. by adding a command
> >> line option to one of the tools involved.
> >
> > When I was reading the binutils source, my impression was that x86 PC16
> > relocations were intended for 16-bit codes in an ELF32/ELF64 container.
> > Whatever we do, we shouldn't break existing 16-bit programs.
>
> Well, if the treatment of PC16 isn't ABI-conforming, then perhaps
> the ABI should be changed to express the intended behavior for this
> reloc type (and make explicit that it's different from PC32 and PC8),
> and there then perhaps ought to be a new reloc type with proper PC16
> semantics (usable for 16-bit operand size XBEGIN as well as 16-bit
> data items with PC-relative expressions)?

This sounds promising.  We can update the current PC16 description
for 16-bit objects.   We can add a new PC16 relocation for XBEGIN if
it is really desirable.  I will open an issue for this.

-- 
H.J.