Re: [PATCH v33 15/21] x86/vdso: Add support for exception fixup in vDSO functions

[Sean Christopherson]

On Tue, Jun 30, 2020 at 09:48:22AM -0700, Andy Lutomirski wrote:
> On Tue, Jun 30, 2020 at 1:41 AM Borislav Petkov <bp@xxxxxxxxx> wrote:
> >
> > On Mon, Jun 29, 2020 at 11:00:55PM -0700, Sean Christopherson wrote:
> > > E.g. the vDSO function should get the fixup even if userspace screws
> > > up mmap() and invokes __vdso_sgx_enter_enclave() without being tagged
> > > an SGX task.
> >
> > I sincerely hope you don't mean this seriously.
> >
> > Please add a member to task_struct which denotes that a task is an
> > sgx task, test that member where needed and forget real quickly about
> > running *any* *fixup* for unrelated tasks.
> 
> I don't see the problem.  If you call this magic vDSO function and get
> a fault, it gets handled.  What's the failure mode?
> 
> >
> > > No hard dependency, it's normal kernel code.  My reasoning for dropping it
> > > in .../vdso was largely to co-locate it with vdso/extable.h due to the
> > > dependency on the format of 'struct vdso_exception_table_entry'.
> >
> > A struct which you defined instead of simply using struct
> > exception_table_entry even if it has a handler member which would remain
> > unused?
> 
> Don't forget the cross-arch issue.  We need that structure to have
> constant layout so that the -m32 build from the vDSO has the same
> layout as in the kernel.
> 
> So my only actual objection to the patch is that there should probably
> be a comment above the structure reminding people that it needs to
> have the same layout on 32-bit, x32, and x86_64.  So maybe the entries
> should be s32 instead of int, although this doesn't really matter.

I highly doubt my past self thought about the cross-arch issue.  The main
reason I created 'struct vdso_exception_table_entry' is that interpretation
of the fields is different.  For the kernel, the fields contain offsets
that are relative to the address of the field itself, i.e. of the fixup
itself.  For vDSO, the offsets are relative to the base of the vDSO.

Reusing exception_table_entry felt like it would be all kinds of confusing.