linux-kernel - Re: [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1312990047.26263.248.camel@zakaz.uk.xensource.com>
Date:	Wed, 10 Aug 2011 16:27:27 +0100
From:	Ian Campbell <Ian.Campbell@...citrix.com>
To:	Peter Zijlstra <a.p.zijlstra@...llo.nl>
CC:	Cyrill Gorcunov <gorcunov@...il.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Pekka Enberg" <penberg@...nel.org>,
	Andi Kleen <ak@...ux.intel.com>, Ingo Molnar <mingo@...e.hu>
Subject: Re: [PATCH 0/3] minor cleanups to EFLAGS initialisation in
 ret_from_fork

On Tue, 2011-07-26 at 15:46 +0100, Peter Zijlstra wrote:
> On Tue, 2011-07-26 at 01:47 +0400, Cyrill Gorcunov wrote:
> > > > schedule (sched.c)
> > > >         ...
> > > >         raw_spin_lock_irq
> > > >         ...
> > > >         context_switch
> > > >                 switch_to
> > > >                         "jnz   ret_from_fork\n\t"
> > > >                         pushq_cfi kernel_eflags(%rip)
> > > >                         popfq_cfi                               # reset kernel eflags
> > > > 
> > > > --->                    irqs are still disabled
> > > > 
> > > >                         call schedule_tail                      # rdi: 'prev' task parameter
> > > >                                 finish_lock_switch
> > > >                                         raw_spin_unlock_irq
> > > > 
> > > > I bet raw_spin_lock_irq at the beginning of the schedule() is set
> > > > for a reason and such change is not safe. Though I may be missing
> > > > something again...
> > > > 
> > > 
> > > This definitely doesn't look "obviously safe" to me.  However, does
> > > anyone see a problem with unconditionally leaving IF disabled even on 32
> > > bits (I haven't traced all the paths yet), i.e. doing the *opposite* of
> > > Ian's patch #2?
> 
> Right, enabling IRQs there isn't cool, currently there's still
> __ARCH_WANT_INTERRUPTS_ON_CTXSW but we're working hard on getting rid of
> that nightmare.
> 
> There's a number of very subtle things that can go wrong when you enable
> interrupts over the context switch.
> 
> Leaving IRQs disabled should be the right thing, on x86 we should
> _never_ have interrupts enabled there.

Just getting back to this after my vac, sorry for the delay.

raw_spin_unlock_irq unconditionally re-enables interrupts so I don't
really see what I've changed since interrupts are enabled by
schedule_tail and I've moved (on 64 bit) the EFLAGS reset to after
schedule_tail, so it should have interrupts enabled at that point
already and so they should remain enabled. Or are you suggesting that
things were already wrong?

However I've switched the order of my second patch anyway, so EFLAGS is
reset to 0x0002 (interrupts disabled) on both 32- and 64-bit before the
call to schedule_tail, since it does seem like the simpler option. I'll
repost shortly.

Ian.

--
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/