Message-ID: <CALCETrXUW0cfvk3K4KsYBM3FiiLqGRGZkyDk8751UsXHu+MJSQ@mail.gmail.com>
Date:	Wed, 12 Aug 2015 12:20:48 -0700
From:	Andy Lutomirski <luto@...capital.net>
To:	Stas Sergeev <stsp@...t.ru>
Cc:	X86 ML <x86@...nel.org>,
	Linux kernel <linux-kernel@...r.kernel.org>
Subject: Re: [regression] x86/signal/64: Fix SS handling for signals delivered
 to 64-bit programs breaks dosemu

On Wed, Aug 12, 2015 at 11:55 AM, Stas Sergeev <stsp@...t.ru> wrote:
> 12.08.2015 21:25, Andy Lutomirski пишет:
>>>>>
>>>>>
>>>>> https://github.com/stsp/dosemu2/commit/7898ac60d5e569964127d6cc48f592caecd20b81
>>>>
>>>> So the problem is that dosemu was actually hacking around the old
>>>> buggy behavior and thus relying on it.  Grr.
>>>
>>> What else it could do? :(
>>
>> Going back in time?  Ask the kernel to fix the issue.
>
> Like this?
> http://www.x86-64.org/pipermail/discuss/2007-May/009913.html
> And this:
> http://www.x86-64.org/pipermail/discuss/2007-May/009923.html

I apologize on behalf of the upstream kernel in 2007.  :-/  I wasn't
really involved at that point.

>
>>>>> Good, but have you added any flag for dosemu to even know
>>>>> it can do this? Unless I am mistaken, you didn't. So the fix you
>>>>> suggest, is not easy to detect and make portable with the older
>>>>> kernels. Any suggestions?
>>>>>
>>>> You could probe for it directly: raise a signal, change the saved ss
>>>> and see what's in ss after sigreturn.
>>>
>>> Umm, nope.
>>
>> Why not?  The safest general way to detect new features is to try to use
>> them.
>
> But this is just too many ugly code for nothing.
> Since it is not very urgent to use sigreturn() instead of
> iret, I guess I'll better wait for an API addition that will
> let the check possible.

I'll ponder it.  You'll get a ~300 cycle speedup if you switch over
(IRET is really slow), but I doubt that matters much.

>
>>>> Let me see if I can come up with a clean kernel fix.
>>>
>>> The check for proper sigreturn would be good.
>>
>> I still don't see how sigreturn matters here.  It's signal *delivery*
>> that's the problem.
>
> But the delivery can be easily checked with "if (ss & 4)".
> What remains is just a sigreturn instead of iret.
>
>> I'm thinking of having signal delivery zap ss only if the old ss looks
>> bogus instead of zapping it unconditionally.  IOW, instead of setting
>> regs->ss = __USER_DS unconditionally, we'd do larl on the old regs->ss
>> and keep it if it's DPL 3 RW data (exp-down or otherwise) and present.
>
> I am not sure how good is this.
> Yes, may help for a backward-compatibility.
> But OTOH the 32bit kernel saves _all_ registers, including
> ss, which is IMHO the right thing to do in general.

I agree.  So does x32.

Are you planning on merging your patches into upstream DOSEMU?

> So as long as the things are already "broken", I wonder if the
> new hacks are worth the troubles.
> Please also see here:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66631#c15
> Not saving fs is a pita!
>

That is an incredible can of worms, and it'll only get worse when
WRFSBASE, etc are enabled in Linux.  I don't think I like the current
kernel behavior, but at least it's fully functional, unlike the old SS
behavior.  (Except that FS, GS, and their bases aren't context
switched correctly right now if you pound on them hard enough.)

>> I'll have to check the precise rules in both the SDM and APM.  The
>> idea is that we don't want IRET to fail during signal delivery, which
>> can happen due to a bad sigreturn or a race against modify_ldt.
>
> Well, this is a "very basic" idea, so to say.
> The fact that segregs are not restored, have much more
> consequences, and since now you already broke things,
> I wonder if something can be finally fixed for good...
>
> What alternatives do we have? Can we do something
> really brave, introduce a new sigaction flag perhaps, that
> will just restore all segregs for new apps, and none - for
> old apps? I mean the above gcc bugzilla ticket in particular -
> very annoying one...

We might need to do that.

Here's a nasty case:

void sighandler(...) {
  switch_userspace_thread();
}

Suppose that switch_userspace_thread() changes fs.  Now what?  On
current kernels, it stays switched.  If we change this, it won't stay
switched.  Even ignoring old ABI, it's not really clear to me what the
right thing to do is.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists