Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From:   Andy Lutomirski <luto@amacapital.net>
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH V3.1] entry: Pass irqentry_state_t by reference
Date:   Thu, 17 Dec 2020 07:35:18 -0800
Message-Id: <24F5DC49-1FB3-42CF-8323-B0B39D936F7F@amacapital.net>
References: <20201217131924.GW3040@hirez.programming.kicks-ass.net>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Andy Lutomirski <luto@kernel.org>,
        Weiny Ira <ira.weiny@intel.com>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        X86 ML <x86@kernel.org>, LKML <linux-kernel@vger.kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Fenghua Yu <fenghua.yu@intel.com>,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
        linux-nvdimm <linux-nvdimm@lists.01.org>,
        Linux-MM <linux-mm@kvack.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        Dan Williams <dan.j.williams@intel.com>,
        Greg KH <gregkh@linuxfoundation.org>
In-Reply-To: <20201217131924.GW3040@hirez.programming.kicks-ass.net>
To:     Peter Zijlstra <peterz@infradead.org>
Precedence: bulk


> On Dec 17, 2020, at 5:19 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>=20
> =EF=BB=BFOn Thu, Dec 17, 2020 at 02:07:01PM +0100, Thomas Gleixner wrote:
>>> On Fri, Dec 11 2020 at 14:14, Andy Lutomirski wrote:
>>>> On Mon, Nov 23, 2020 at 10:10 PM <ira.weiny@intel.com> wrote:
>>> After contemplating this for a bit, I think this isn't really the
>>> right approach.  It *works*, but we've mostly just created a bit of an
>>> unfortunate situation.  Our stack, on a (possibly nested) entry looks
>>> like:
>>>=20
>>> previous frame (or empty if we came from usermode)
>>> ---
>>> SS
>>> RSP
>>> FLAGS
>>> CS
>>> RIP
>>> rest of pt_regs
>>>=20
>>> C frame
>>>=20
>>> irqentry_state_t (maybe -- the compiler is within its rights to play
>>> almost arbitrary games here)
>>>=20
>>> more C stuff
>>>=20
>>> So what we've accomplished is having two distinct arch register
>>> regions, one called pt_regs and the other stuck in irqentry_state_t.
>>> This is annoying because it means that, if we want to access this
>>> thing without passing a pointer around or access it at all from outer
>>> frames, we need to do something terrible with the unwinder, and we
>>> don't want to go there.
>>>=20
>>> So I propose a somewhat different solution: lay out the stack like this.=

>>>=20
>>> SS
>>> RSP
>>> FLAGS
>>> CS
>>> RIP
>>> rest of pt_regs
>>> PKS
>>> ^^^^^^^^ extended_pt_regs points here
>>>=20
>>> C frame
>>> more C stuff
>>> ...
>>>=20
>>> IOW we have:
>>>=20
>>> struct extended_pt_regs {
>>>  bool rcu_whatever;
>>>  other generic fields here;
>>>  struct arch_extended_pt_regs arch_regs;
>>>  struct pt_regs regs;
>>> };
>>>=20
>>> and arch_extended_pt_regs has unsigned long pks;
>>>=20
>>> and instead of passing a pointer to irqentry_state_t to the generic
>>> entry/exit code, we just pass a pt_regs pointer.
>>=20
>> While I agree vs. PKS which is architecture specific state and needed in
>> other places e.g. #PF, I'm not convinced that sticking the existing
>> state into the same area buys us anything more than an indirect access.
>>=20
>> Peter?
>=20
> Agreed; that immediately solves the confusion Ira had as well. While
> extending pt_regs sounds scary, I think we've isolated our pt_regs
> implementation from actual ABI pretty well, but of course, that would
> need an audit. We don't want to leak this into signals for example.
>=20

I=E2=80=99m okay with this.

My suggestion for having an extended pt_regs that contains pt_regs is to kee=
p extensions like this invisible to unsuspecting parts of the kernel. In par=
ticular, BPF seems to pass around struct pt_regs *, and I don=E2=80=99t know=
 what the implications of effectively offsetting all the registers relative t=
o the pointer would be.

Anything that actually broke the signal regs ABI should be noticed by the x8=
6 selftests =E2=80=94 the tests read and write registers through ucontext.

>=20