[PATCH v2] x86: Restore PC16 relocation overflow check

[H.J. Lu]

On Tue, Jun 1, 2021 at 8:08 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 01.06.2021 15:02, Michael Matz wrote:
> > On Tue, 1 Jun 2021, Jan Beulich wrote:
> >>>>>> The issue is how linker should handle overflow for PC16.
> >>>>>
> >>>>> I think as per above.  An value within [-0xffff,0xffff]
> >>>>> (mathematically, i.e. the 32 or 64 bit value of the computation
> >>>>> S+A-P) is defined as not overflowing.  The linker basically needs to
> >>>>> assume that the author knew what he was doing when using a PC16
> >>>>> offset and only needs to flag things that simply cannot be made to
> >>>>> work (like e.g. jumping 0x12345 bytes forward).
> >>>>
> >>>> While the ABI is free to state what it wants, such a wide range is
> >>>> not very useful when the relocation is used outside of 16-bit mode.
> >>>
> >>> Well, as it stands the relocation needs to support uses within and
> >>> outside 16-bit mode, and the linker can't differ between those, so
> >>> what's acceptable needs to be the union of all ranges.
> >>>
> >>>> The main need for the overflow detection is when it's out of range by
> >>>> just a little (e.g. because of some piece of code or data having
> >>>> grown just enough to bring it out of range).
> >>>
> >>> But out of _what_ range?  You are saying that [-0xffff,0xffff] is too
> >>> broad, what range do you want?  (I don't think you can get away with a
> >>> smaller range with PC16 in the x86-64 psABI, but for completeness)
> >>
> >> The true disp16 range - [-0x8000,+0x7fff].
> >
> > Okay, well, yeah, I meanwhile think we can't do that generally :-/.  We
> > could possibly with an option, and I think we could have that option on by
> > default to be disabled by code that needs to embed 16bit code into elf32
> > or elf64 binaries.  (That option would extend the accepted range to
> > [-0xffff,0xffff])
>
> That's one of the things I proposed to H.J. earlier on. He took the
> position that even such projects should continue to work "out-of-the-
> box" with the next binutils release. I disagree, but he's the
> maintainer ...
>
> >> As said before, for the purposes of 32- and 64-bit code (and data), this
> >> (or a new, proper reloc type) needs to match PC32 allowing
> >> [-0x80000000,+0x7fffffff]. And obviously this also ought to match PC8,
> >> allowing [-0x80,+0x7f].
> >
> > I think for PC8 we have an easier time: there's no code model that wraps
> > around at 256, so allowing only [-0x80,0x7f] now and requiring
> > sign-extension to preserve the value ought to not give us any headaches in
> > the wild.
> >
> >> Also note how PC8's current handling in bfd does _not_ fit 16-bit
> >> code (wrapping at the 64k boundary not being accounted for).
> >
> > I don't think we "correctly" do that with any code model or relocation,
> > right?  The linker doesn't know the context of relocations, and hence
> > can't know if it would need to account for wrap-around at 64k, at 4G or
> > at 1<<64.  Or, if we want to get into extremes consider the A20 gate and a
> > wraparound at 1M.  There's only so much we can model.
>
> You don't mention the A20 gate here, do you? ;-)
>
> > I mean you can embed 32bit code in elf64, and use an PC32 reloc to get
> > from 3G to 0G, if you consider the wrap-around.  When only strictly
> > accepting the signed32 range that would be impossible, and we have the
> > same problem as with PC16: should we, or should we not, flag such use, and
> > under which conditions?
>
> Right, I did consider this PC32 issues as well. Embedding smaller
> address size code into an object targeting a larger address size
> architecture is of course always problematic. Hence why I think the
> PC32 handling is okay as is, but the PC16 one would better match in
> how it gets treated.
>
> > In such cases we need to consider existing use in the wild and provide a
> > way to accept existing .o files.  If those cases are seldom then possibly
> > only under an option.
>
> Right - the tradeoff between sufficiently precise checks (with false
> positive errors) and sufficiently imprecise checks (with false
> negatives, i.e. missing diagnostics). That's generally something only
> a human can decide, and only on a project by project or even component
> by component basis.
>
> >>>> This is what my change was about, which - aiui - H.J. has meanwhile
> >>>> reverted.
> >>>
> >>> If your change flagged values within [0x8000,0xffff] or [-0xffff,-0x8001]
> >>> by default then I think the reversal was justified, those need to be
> >>> continued to be accepted, for better or worse.
> >>
> >> Interesting - I took your initial reply to H.J. to mean the opposite.
> >> Obviously a misunderstanding then ...
> >
> > No, I changed my mind in the meantime.  I would have liked to say the
> > obvious thing in the psABI and not consider wrap-around, but now think we
> > can't go the full way for that.  I still would like us to make PC8 and
> > PC16 acceptable, but the latter with a larger than obvious acceptable
> > range.
> >
> > As for a new relocaiton that does have the signed16 restriction, no matter
> > the code model, I remain doubtful: it can introduce more (compatibility)
> > problems than it solves.
>
> Indeed, there is this risk. I continue to be in favor of a new command
> line option, off by default, restoring original PC16 checking and at
> the same time making PC32 in a 64-bit container match. (It could be two
> separate options, but to me this feels like going too far.) As per what
> you say above, I'd be happy to leave PC8 as is.
>
> H.J. - can we get you to re-consider?

The default behavior should stay the same since we are using PC16 only
in 16-bit programs.   We can add a new option to change PC16 overflow
behavior so that it can be used in 32-bit and 64-bit programs.

-- 
H.J.