Denys is right that KERNEL_STACK_OFFSET is a mess. Let's start fixing
it.
This removes all C code that *reads* kernel_stack. It also fixes the
KERNEL_STACK_OFFSET abomination in ia32_sysenter_target.
It does not fix the KERNEL_STACK_OFFSET abomination in GET_THREAD_INFO
and THREAD_INFO. I think that should be its own patch.
It also doesn't change the two syscall targets. To fix them, we should
make a decision. Either we should make KERNEL_STACK_OFFSET have the
correct nonzero value to save an instruction or we should get rid of
kernel_stack entirely.
Andy Lutomirski (3):
x86: Add this_cpu_sp0() to read sp0 for the current cpu
x86: Switch all C consumers of kernel_stack to this_cpu_sp0
x86, asm: Change the 32-bit sysenter code to use sp0
arch/x86/ia32/ia32entry.S | 3 +--
arch/x86/include/asm/processor.h | 5 +++++
arch/x86/include/asm/thread_info.h | 3 +--
arch/x86/kernel/asm-offsets_64.c | 1 +
arch/x86/kernel/traps.c | 2 +-
arch/x86/lguest/boot.c | 1 +
arch/x86/xen/enlighten.c | 1 +
7 files changed, 11 insertions(+), 5 deletions(-)
--
2.1.0
We currently store references to the top of the kernel stack in
multiple places: kernel_stack (with an offset) and
init_tss.x86_tss.sp0 (no offset). The latter is defined by hardware
and is a clean canonical way to find the top of the stack. Add an
accessor so we can start using it.
This needs minor paravirt tweaks to ensure that On native, sp0
defines the top of the kernel stack. On Xen and lguest, the
hypervisor tracks it, but we want to start reading sp0 in the
kernel. Fixing this is simple: just update our local copy of sp0 as
well as the hypervisor's copy on task switches.
Signed-off-by: Andy Lutomirski <[email protected]>
---
arch/x86/include/asm/processor.h | 5 +++++
arch/x86/lguest/boot.c | 1 +
arch/x86/xen/enlighten.c | 1 +
3 files changed, 7 insertions(+)
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index a092a0cce0b7..477b1ff4a6c9 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -564,6 +564,11 @@ static inline void native_swapgs(void)
#endif
}
+static inline unsigned long this_cpu_sp0(void)
+{
+ return this_cpu_read_stable(init_tss.x86_tss.sp0);
+}
+
#ifdef CONFIG_PARAVIRT
#include <asm/paravirt.h>
#else
diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c
index c1c1544b8485..77a609d1898d 100644
--- a/arch/x86/lguest/boot.c
+++ b/arch/x86/lguest/boot.c
@@ -1053,6 +1053,7 @@ static void lguest_load_sp0(struct tss_struct *tss,
{
lazy_hcall3(LHCALL_SET_STACK, __KERNEL_DS | 0x1, thread->sp0,
THREAD_SIZE / PAGE_SIZE);
+ tss->x86_tss.sp0 = sp0;
}
/* Let's just say, I wouldn't do debugging under a Guest. */
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 78a881b7fc41..d8195ca15346 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -912,6 +912,7 @@ static void xen_load_sp0(struct tss_struct *tss,
mcs = xen_mc_entry(0);
MULTI_stack_switch(mcs.mc, __KERNEL_DS, thread->sp0);
xen_mc_issue(PARAVIRT_LAZY_CPU);
+ tss->x86_tss.sp0 = thread->sp0;
}
static void xen_set_iopl_mask(unsigned mask)
--
2.1.0
This will make modifying the semantics of kernel_stack easier.
Cc: Konrad Rzeszutek Wilk <[email protected]>
Cc: Boris Ostrovsky <[email protected]>
Cc: Rusty Russell <[email protected]>
Signed-off-by: Andy Lutomirski <[email protected]>
---
arch/x86/include/asm/thread_info.h | 3 +--
arch/x86/kernel/traps.c | 2 +-
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index e82e95abc92b..92549053d86d 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
static inline struct thread_info *current_thread_info(void)
{
struct thread_info *ti;
- ti = (void *)(this_cpu_read_stable(kernel_stack) +
- KERNEL_STACK_OFFSET - THREAD_SIZE);
+ ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
return ti;
}
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index c74f2f5652da..d287ea779728 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -174,7 +174,7 @@ void ist_begin_non_atomic(struct pt_regs *regs)
* will catch asm bugs and any attempt to use ist_preempt_enable
* from double_fault.
*/
- BUG_ON(((current_stack_pointer() ^ this_cpu_read_stable(kernel_stack))
+ BUG_ON(((current_stack_pointer() ^ (this_cpu_sp0() - 1))
& ~(THREAD_SIZE - 1)) != 0);
preempt_count_sub(HARDIRQ_OFFSET);
--
2.1.0
The ia32 sysenter code loaded the top of the kernel stack into rsp
by loading kernel_stack and then adjusting it. It can be simplified
to just read sp0 directly.
This requires the addition of a new asm-offsets entry for sp0.
Signed-off-by: Andy Lutomirski <[email protected]>
---
arch/x86/ia32/ia32entry.S | 3 +--
arch/x86/kernel/asm-offsets_64.c | 1 +
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
index ed9746340363..719db63b35c4 100644
--- a/arch/x86/ia32/ia32entry.S
+++ b/arch/x86/ia32/ia32entry.S
@@ -113,8 +113,7 @@ ENTRY(ia32_sysenter_target)
CFI_DEF_CFA rsp,0
CFI_REGISTER rsp,rbp
SWAPGS_UNSAFE_STACK
- movq PER_CPU_VAR(kernel_stack), %rsp
- addq $(KERNEL_STACK_OFFSET),%rsp
+ movq PER_CPU_VAR(init_tss + TSS_sp0), %rsp
/*
* No need to follow this irqs on/off section: the syscall
* disabled irqs, here we enable it straight after entry:
diff --git a/arch/x86/kernel/asm-offsets_64.c b/arch/x86/kernel/asm-offsets_64.c
index fdcbb4d27c9f..5ce6f2da8763 100644
--- a/arch/x86/kernel/asm-offsets_64.c
+++ b/arch/x86/kernel/asm-offsets_64.c
@@ -81,6 +81,7 @@ int main(void)
#undef ENTRY
OFFSET(TSS_ist, tss_struct, x86_tss.ist);
+ OFFSET(TSS_sp0, tss_struct, x86_tss.sp0);
BLANK();
DEFINE(__NR_syscall_max, sizeof(syscalls_64) - 1);
--
2.1.0
On 02/27/2015 01:07 AM, Andy Lutomirski wrote:
> This will make modifying the semantics of kernel_stack easier.
>
> Cc: Konrad Rzeszutek Wilk <[email protected]>
> Cc: Boris Ostrovsky <[email protected]>
> Cc: Rusty Russell <[email protected]>
> Signed-off-by: Andy Lutomirski <[email protected]>
> ---
> arch/x86/include/asm/thread_info.h | 3 +--
> arch/x86/kernel/traps.c | 2 +-
> 2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> index e82e95abc92b..92549053d86d 100644
> --- a/arch/x86/include/asm/thread_info.h
> +++ b/arch/x86/include/asm/thread_info.h
> @@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
> static inline struct thread_info *current_thread_info(void)
> {
> struct thread_info *ti;
> - ti = (void *)(this_cpu_read_stable(kernel_stack) +
> - KERNEL_STACK_OFFSET - THREAD_SIZE);
> + ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
> return ti;
> }
>
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index c74f2f5652da..d287ea779728 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -174,7 +174,7 @@ void ist_begin_non_atomic(struct pt_regs *regs)
> * will catch asm bugs and any attempt to use ist_preempt_enable
> * from double_fault.
> */
> - BUG_ON(((current_stack_pointer() ^ this_cpu_read_stable(kernel_stack))
> + BUG_ON(((current_stack_pointer() ^ (this_cpu_sp0() - 1))
> & ~(THREAD_SIZE - 1)) != 0);
While we are at it, I propose a more readable version of this check:
BUG_ON(this_cpu_sp0() - current_stack_pointer() >= THREAD_SIZE);
Yes, I am aware that it is not equivalent to the existing condition
- it uses the fact that this_cpu_sp0(), previous check
wasn't making that assumption. But that assumption is true,
so shouldn't be a problem.
On 02/27/2015 01:07 AM, Andy Lutomirski wrote:
> This will make modifying the semantics of kernel_stack easier.
>
> Cc: Konrad Rzeszutek Wilk <[email protected]>
> Cc: Boris Ostrovsky <[email protected]>
> Cc: Rusty Russell <[email protected]>
> Signed-off-by: Andy Lutomirski <[email protected]>
> ---
> arch/x86/include/asm/thread_info.h | 3 +--
> arch/x86/kernel/traps.c | 2 +-
> 2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> index e82e95abc92b..92549053d86d 100644
> --- a/arch/x86/include/asm/thread_info.h
> +++ b/arch/x86/include/asm/thread_info.h
> @@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
> static inline struct thread_info *current_thread_info(void)
> {
> struct thread_info *ti;
> - ti = (void *)(this_cpu_read_stable(kernel_stack) +
> - KERNEL_STACK_OFFSET - THREAD_SIZE);
> + ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
> return ti;
> }
This makes modpost unhappy, it says these modules need init_tss symbol
to be visible to them:
Building modules, stage 2.
MODPOST 2556 modules
ERROR: "init_tss" [sound/usb/usx2y/snd-usb-usx2y.ko] undefined!
ERROR: "init_tss" [sound/pci/hda/snd-hda-codec.ko] undefined!
ERROR: "init_tss" [sound/pci/emu10k1/snd-emu10k1.ko] undefined!
ERROR: "init_tss" [sound/core/snd-pcm.ko] undefined!
ERROR: "init_tss" [sound/core/snd-hwdep.ko] undefined!
ERROR: "init_tss" [sound/core/seq/snd-seq.ko] undefined!
ERROR: "init_tss" [sound/core/oss/snd-pcm-oss.ko] undefined!
ERROR: "init_tss" [net/sunrpc/sunrpc.ko] undefined!
ERROR: "init_tss" [net/sctp/sctp.ko] undefined!
ERROR: "init_tss" [net/rds/rds_tcp.ko] undefined!
ERROR: "init_tss" [net/core/pktgen.ko] undefined!
ERROR: "init_tss" [net/batman-adv/batman-adv.ko] undefined!
ERROR: "init_tss" [net/9p/9pnet.ko] undefined!
ERROR: "init_tss" [fs/reiserfs/reiserfs.ko] undefined!
ERROR: "init_tss" [fs/ocfs2/dlmfs/ocfs2_dlmfs.ko] undefined!
ERROR: "init_tss" [fs/ocfs2/cluster/ocfs2_nodemanager.ko] undefined!
ERROR: "init_tss" [fs/nfsd/nfsd.ko] undefined!
ERROR: "init_tss" [fs/fscache/fscache.ko] undefined!
ERROR: "init_tss" [fs/fat/fat.ko] undefined!
ERROR: "init_tss" [fs/ecryptfs/ecryptfs.ko] undefined!
ERROR: "init_tss" [fs/cachefiles/cachefiles.ko] undefined!
ERROR: "init_tss" [fs/btrfs/btrfs.ko] undefined!
ERROR: "init_tss" [fs/9p/9p.ko] undefined!
ERROR: "init_tss" [drivers/xen/xen-privcmd.ko] undefined!
ERROR: "init_tss" [drivers/vhost/vhost.ko] undefined!
ERROR: "init_tss" [drivers/tty/n_hdlc.ko] undefined!
ERROR: "init_tss" [drivers/target/target_core_file.ko] undefined!
ERROR: "init_tss" [drivers/scsi/pmcraid.ko] undefined!
ERROR: "init_tss" [drivers/scsi/lpfc/lpfc.ko] undefined!
ERROR: "init_tss" [drivers/parport/parport.ko] undefined!
ERROR: "init_tss" [drivers/net/ethernet/broadcom/tg3.ko] undefined!
ERROR: "init_tss" [drivers/net/bonding/bonding.ko] undefined!
ERROR: "init_tss" [drivers/mtd/mtd.ko] undefined!
ERROR: "init_tss" [drivers/misc/vmw_vmci/vmw_vmci.ko] undefined!
ERROR: "init_tss" [drivers/media/v4l2-core/videodev.ko] undefined!
ERROR: "init_tss" [drivers/media/usb/uvc/uvcvideo.ko] undefined!
ERROR: "init_tss" [drivers/media/pci/saa7134/saa7134.ko] undefined!
ERROR: "init_tss" [drivers/media/pci/ivtv/ivtvfb.ko] undefined!
ERROR: "init_tss" [drivers/media/i2c/vpx3220.ko] undefined!
ERROR: "init_tss" [drivers/md/bcache/bcache.ko] undefined!
ERROR: "init_tss" [drivers/isdn/i4l/isdn.ko] undefined!
ERROR: "init_tss" [drivers/input/misc/uinput.ko] undefined!
ERROR: "init_tss" [drivers/infiniband/hw/qib/ib_qib.ko] undefined!
ERROR: "init_tss" [drivers/infiniband/hw/ipath/ib_ipath.ko] undefined!
ERROR: "init_tss" [drivers/infiniband/core/ib_uverbs.ko] undefined!
ERROR: "init_tss" [drivers/hid/uhid.ko] undefined!
ERROR: "init_tss" [drivers/gpu/drm/radeon/radeon.ko] undefined!
ERROR: "init_tss" [drivers/gpu/drm/qxl/qxl.ko] undefined!
ERROR: "init_tss" [drivers/gpu/drm/i915/i915.ko] undefined!
ERROR: "init_tss" [drivers/gpu/drm/drm.ko] undefined!
ERROR: "init_tss" [drivers/firewire/firewire-core.ko] undefined!
ERROR: "init_tss" [drivers/char/tpm/tpm_tis.ko] undefined!
ERROR: "init_tss" [drivers/char/tpm/tpm.ko] undefined!
ERROR: "init_tss" [drivers/char/pcmcia/cm4000_cs.ko] undefined!
ERROR: "init_tss" [drivers/char/lp.ko] undefined!
ERROR: "init_tss" [drivers/char/ipmi/ipmi_devintf.ko] undefined!
ERROR: "init_tss" [drivers/block/drbd/drbd.ko] undefined!
ERROR: "init_tss" [drivers/acpi/acpi_pad.ko] undefined!
ERROR: "init_tss" [arch/x86/oprofile/oprofile.ko] undefined!
ERROR: "init_tss" [arch/x86/kvm/kvm.ko] undefined!
ERROR: "init_tss" [arch/x86/kvm/kvm-intel.ko] undefined!
make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2
I took one and investigated how it references init_tss.
drivers/hid/uhid.c:
static int uhid_event_from_user(const char __user *buffer, size_t len,
struct uhid_event *event)
{
if (is_compat_task()) { <=========== HERE
u32 type;
if (get_user(type, buffer))
return -EFAULT;
if (type == UHID_CREATE) {
--
vda
On Feb 27, 2015 8:13 AM, "Denys Vlasenko" <[email protected]> wrote:
>
> On 02/27/2015 01:07 AM, Andy Lutomirski wrote:
> > This will make modifying the semantics of kernel_stack easier.
> >
> > Cc: Konrad Rzeszutek Wilk <[email protected]>
> > Cc: Boris Ostrovsky <[email protected]>
> > Cc: Rusty Russell <[email protected]>
> > Signed-off-by: Andy Lutomirski <[email protected]>
> > ---
> > arch/x86/include/asm/thread_info.h | 3 +--
> > arch/x86/kernel/traps.c | 2 +-
> > 2 files changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> > index e82e95abc92b..92549053d86d 100644
> > --- a/arch/x86/include/asm/thread_info.h
> > +++ b/arch/x86/include/asm/thread_info.h
> > @@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
> > static inline struct thread_info *current_thread_info(void)
> > {
> > struct thread_info *ti;
> > - ti = (void *)(this_cpu_read_stable(kernel_stack) +
> > - KERNEL_STACK_OFFSET - THREAD_SIZE);
> > + ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
> > return ti;
> > }
> >
> > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > index c74f2f5652da..d287ea779728 100644
> > --- a/arch/x86/kernel/traps.c
> > +++ b/arch/x86/kernel/traps.c
> > @@ -174,7 +174,7 @@ void ist_begin_non_atomic(struct pt_regs *regs)
> > * will catch asm bugs and any attempt to use ist_preempt_enable
> > * from double_fault.
> > */
> > - BUG_ON(((current_stack_pointer() ^ this_cpu_read_stable(kernel_stack))
> > + BUG_ON(((current_stack_pointer() ^ (this_cpu_sp0() - 1))
> > & ~(THREAD_SIZE - 1)) != 0);
>
> While we are at it, I propose a more readable version of this check:
>
> BUG_ON(this_cpu_sp0() - current_stack_pointer() >= THREAD_SIZE);
>
> Yes, I am aware that it is not equivalent to the existing condition
> - it uses the fact that this_cpu_sp0(), previous check
> wasn't making that assumption. But that assumption is true,
> so shouldn't be a problem.
You're missing an absolute value in here, though. This isn't a check
for overflow; it's a check that we aren't on an IST or other per cpu
stack.
--Andy
On Fri, Feb 27, 2015 at 8:52 AM, Denys Vlasenko <[email protected]> wrote:
> On 02/27/2015 01:07 AM, Andy Lutomirski wrote:
>> This will make modifying the semantics of kernel_stack easier.
>>
>> Cc: Konrad Rzeszutek Wilk <[email protected]>
>> Cc: Boris Ostrovsky <[email protected]>
>> Cc: Rusty Russell <[email protected]>
>> Signed-off-by: Andy Lutomirski <[email protected]>
>> ---
>> arch/x86/include/asm/thread_info.h | 3 +--
>> arch/x86/kernel/traps.c | 2 +-
>> 2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
>> index e82e95abc92b..92549053d86d 100644
>> --- a/arch/x86/include/asm/thread_info.h
>> +++ b/arch/x86/include/asm/thread_info.h
>> @@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
>> static inline struct thread_info *current_thread_info(void)
>> {
>> struct thread_info *ti;
>> - ti = (void *)(this_cpu_read_stable(kernel_stack) +
>> - KERNEL_STACK_OFFSET - THREAD_SIZE);
>> + ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
>> return ti;
>> }
>
> This makes modpost unhappy, it says these modules need init_tss symbol
> to be visible to them:
>
> Building modules, stage 2.
> MODPOST 2556 modules
> ERROR: "init_tss" [sound/usb/usx2y/snd-usb-usx2y.ko] undefined!
> ERROR: "init_tss" [sound/pci/hda/snd-hda-codec.ko] undefined!
> ERROR: "init_tss" [sound/pci/emu10k1/snd-emu10k1.ko] undefined!
> ERROR: "init_tss" [sound/core/snd-pcm.ko] undefined!
> ERROR: "init_tss" [sound/core/snd-hwdep.ko] undefined!
> ERROR: "init_tss" [sound/core/seq/snd-seq.ko] undefined!
> ERROR: "init_tss" [sound/core/oss/snd-pcm-oss.ko] undefined!
> ERROR: "init_tss" [net/sunrpc/sunrpc.ko] undefined!
> ERROR: "init_tss" [net/sctp/sctp.ko] undefined!
> ERROR: "init_tss" [net/rds/rds_tcp.ko] undefined!
> ERROR: "init_tss" [net/core/pktgen.ko] undefined!
> ERROR: "init_tss" [net/batman-adv/batman-adv.ko] undefined!
> ERROR: "init_tss" [net/9p/9pnet.ko] undefined!
> ERROR: "init_tss" [fs/reiserfs/reiserfs.ko] undefined!
> ERROR: "init_tss" [fs/ocfs2/dlmfs/ocfs2_dlmfs.ko] undefined!
> ERROR: "init_tss" [fs/ocfs2/cluster/ocfs2_nodemanager.ko] undefined!
> ERROR: "init_tss" [fs/nfsd/nfsd.ko] undefined!
> ERROR: "init_tss" [fs/fscache/fscache.ko] undefined!
> ERROR: "init_tss" [fs/fat/fat.ko] undefined!
> ERROR: "init_tss" [fs/ecryptfs/ecryptfs.ko] undefined!
> ERROR: "init_tss" [fs/cachefiles/cachefiles.ko] undefined!
> ERROR: "init_tss" [fs/btrfs/btrfs.ko] undefined!
> ERROR: "init_tss" [fs/9p/9p.ko] undefined!
> ERROR: "init_tss" [drivers/xen/xen-privcmd.ko] undefined!
> ERROR: "init_tss" [drivers/vhost/vhost.ko] undefined!
> ERROR: "init_tss" [drivers/tty/n_hdlc.ko] undefined!
> ERROR: "init_tss" [drivers/target/target_core_file.ko] undefined!
> ERROR: "init_tss" [drivers/scsi/pmcraid.ko] undefined!
> ERROR: "init_tss" [drivers/scsi/lpfc/lpfc.ko] undefined!
> ERROR: "init_tss" [drivers/parport/parport.ko] undefined!
> ERROR: "init_tss" [drivers/net/ethernet/broadcom/tg3.ko] undefined!
> ERROR: "init_tss" [drivers/net/bonding/bonding.ko] undefined!
> ERROR: "init_tss" [drivers/mtd/mtd.ko] undefined!
> ERROR: "init_tss" [drivers/misc/vmw_vmci/vmw_vmci.ko] undefined!
> ERROR: "init_tss" [drivers/media/v4l2-core/videodev.ko] undefined!
> ERROR: "init_tss" [drivers/media/usb/uvc/uvcvideo.ko] undefined!
> ERROR: "init_tss" [drivers/media/pci/saa7134/saa7134.ko] undefined!
> ERROR: "init_tss" [drivers/media/pci/ivtv/ivtvfb.ko] undefined!
> ERROR: "init_tss" [drivers/media/i2c/vpx3220.ko] undefined!
> ERROR: "init_tss" [drivers/md/bcache/bcache.ko] undefined!
> ERROR: "init_tss" [drivers/isdn/i4l/isdn.ko] undefined!
> ERROR: "init_tss" [drivers/input/misc/uinput.ko] undefined!
> ERROR: "init_tss" [drivers/infiniband/hw/qib/ib_qib.ko] undefined!
> ERROR: "init_tss" [drivers/infiniband/hw/ipath/ib_ipath.ko] undefined!
> ERROR: "init_tss" [drivers/infiniband/core/ib_uverbs.ko] undefined!
> ERROR: "init_tss" [drivers/hid/uhid.ko] undefined!
> ERROR: "init_tss" [drivers/gpu/drm/radeon/radeon.ko] undefined!
> ERROR: "init_tss" [drivers/gpu/drm/qxl/qxl.ko] undefined!
> ERROR: "init_tss" [drivers/gpu/drm/i915/i915.ko] undefined!
> ERROR: "init_tss" [drivers/gpu/drm/drm.ko] undefined!
> ERROR: "init_tss" [drivers/firewire/firewire-core.ko] undefined!
> ERROR: "init_tss" [drivers/char/tpm/tpm_tis.ko] undefined!
> ERROR: "init_tss" [drivers/char/tpm/tpm.ko] undefined!
> ERROR: "init_tss" [drivers/char/pcmcia/cm4000_cs.ko] undefined!
> ERROR: "init_tss" [drivers/char/lp.ko] undefined!
> ERROR: "init_tss" [drivers/char/ipmi/ipmi_devintf.ko] undefined!
> ERROR: "init_tss" [drivers/block/drbd/drbd.ko] undefined!
> ERROR: "init_tss" [drivers/acpi/acpi_pad.ko] undefined!
> ERROR: "init_tss" [arch/x86/oprofile/oprofile.ko] undefined!
> ERROR: "init_tss" [arch/x86/kvm/kvm.ko] undefined!
> ERROR: "init_tss" [arch/x86/kvm/kvm-intel.ko] undefined!
> make[1]: *** [__modpost] Error 1
> make: *** [modules] Error 2
>
> I took one and investigated how it references init_tss.
>
> drivers/hid/uhid.c:
>
> static int uhid_event_from_user(const char __user *buffer, size_t len,
> struct uhid_event *event)
> {
> if (is_compat_task()) { <=========== HERE
> u32 type;
>
> if (get_user(type, buffer))
> return -EFAULT;
>
> if (type == UHID_CREATE) {
>
>
Whoops! I fixed it locally with an EXPORT_SYMBOL_GPL.
--Andy
On 02/27/2015 08:56 PM, Andy Lutomirski wrote:
> On Feb 27, 2015 8:13 AM, "Denys Vlasenko" <[email protected]> wrote:
>>
>> On 02/27/2015 01:07 AM, Andy Lutomirski wrote:
>>> This will make modifying the semantics of kernel_stack easier.
>>>
>>> Cc: Konrad Rzeszutek Wilk <[email protected]>
>>> Cc: Boris Ostrovsky <[email protected]>
>>> Cc: Rusty Russell <[email protected]>
>>> Signed-off-by: Andy Lutomirski <[email protected]>
>>> ---
>>> arch/x86/include/asm/thread_info.h | 3 +--
>>> arch/x86/kernel/traps.c | 2 +-
>>> 2 files changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
>>> index e82e95abc92b..92549053d86d 100644
>>> --- a/arch/x86/include/asm/thread_info.h
>>> +++ b/arch/x86/include/asm/thread_info.h
>>> @@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
>>> static inline struct thread_info *current_thread_info(void)
>>> {
>>> struct thread_info *ti;
>>> - ti = (void *)(this_cpu_read_stable(kernel_stack) +
>>> - KERNEL_STACK_OFFSET - THREAD_SIZE);
>>> + ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
>>> return ti;
>>> }
>>>
>>> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
>>> index c74f2f5652da..d287ea779728 100644
>>> --- a/arch/x86/kernel/traps.c
>>> +++ b/arch/x86/kernel/traps.c
>>> @@ -174,7 +174,7 @@ void ist_begin_non_atomic(struct pt_regs *regs)
>>> * will catch asm bugs and any attempt to use ist_preempt_enable
>>> * from double_fault.
>>> */
>>> - BUG_ON(((current_stack_pointer() ^ this_cpu_read_stable(kernel_stack))
>>> + BUG_ON(((current_stack_pointer() ^ (this_cpu_sp0() - 1))
>>> & ~(THREAD_SIZE - 1)) != 0);
>>
>> While we are at it, I propose a more readable version of this check:
>>
>> BUG_ON(this_cpu_sp0() - current_stack_pointer() >= THREAD_SIZE);
>>
>> Yes, I am aware that it is not equivalent to the existing condition
>> - it uses the fact that this_cpu_sp0(), previous check
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Oops...
I meant to say "...the fact that this_cpu_sp0() points to the very top
of the stack, previous check ..."
>> wasn't making that assumption. But that assumption is true,
>> so shouldn't be a problem.
>
> You're missing an absolute value in here, though. This isn't a check
> for overflow; it's a check that we aren't on an IST or other per cpu
> stack.
Yes, that's exactly what the condition checks for. It reads
"is current stack pointer below task's kernel stack by no more
than THREAD_SIZE?"
which is only possible if we are on task's kernel stack:
If current_stack_pointer() is elsewhere, it is either
(a) much smaller than this_cpu_sp0(), and BUG_ON condition
obviously triggers; or
(b) it is somewhere above this_cpu_sp0(), in which case subtraction
overflows and condition triggers too.
How would you write this condition so that it's easily readable?
Evidently, my version isn't as readable sa I hoped...
On Feb 27, 2015 1:12 PM, "Denys Vlasenko" <[email protected]> wrote:
>
> On 02/27/2015 08:56 PM, Andy Lutomirski wrote:
> > On Feb 27, 2015 8:13 AM, "Denys Vlasenko" <[email protected]> wrote:
> >>
> >> On 02/27/2015 01:07 AM, Andy Lutomirski wrote:
> >>> This will make modifying the semantics of kernel_stack easier.
> >>>
> >>> Cc: Konrad Rzeszutek Wilk <[email protected]>
> >>> Cc: Boris Ostrovsky <[email protected]>
> >>> Cc: Rusty Russell <[email protected]>
> >>> Signed-off-by: Andy Lutomirski <[email protected]>
> >>> ---
> >>> arch/x86/include/asm/thread_info.h | 3 +--
> >>> arch/x86/kernel/traps.c | 2 +-
> >>> 2 files changed, 2 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> >>> index e82e95abc92b..92549053d86d 100644
> >>> --- a/arch/x86/include/asm/thread_info.h
> >>> +++ b/arch/x86/include/asm/thread_info.h
> >>> @@ -163,8 +163,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
> >>> static inline struct thread_info *current_thread_info(void)
> >>> {
> >>> struct thread_info *ti;
> >>> - ti = (void *)(this_cpu_read_stable(kernel_stack) +
> >>> - KERNEL_STACK_OFFSET - THREAD_SIZE);
> >>> + ti = (void *)(this_cpu_sp0() - THREAD_SIZE);
> >>> return ti;
> >>> }
> >>>
> >>> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> >>> index c74f2f5652da..d287ea779728 100644
> >>> --- a/arch/x86/kernel/traps.c
> >>> +++ b/arch/x86/kernel/traps.c
> >>> @@ -174,7 +174,7 @@ void ist_begin_non_atomic(struct pt_regs *regs)
> >>> * will catch asm bugs and any attempt to use ist_preempt_enable
> >>> * from double_fault.
> >>> */
> >>> - BUG_ON(((current_stack_pointer() ^ this_cpu_read_stable(kernel_stack))
> >>> + BUG_ON(((current_stack_pointer() ^ (this_cpu_sp0() - 1))
> >>> & ~(THREAD_SIZE - 1)) != 0);
> >>
> >> While we are at it, I propose a more readable version of this check:
> >>
> >> BUG_ON(this_cpu_sp0() - current_stack_pointer() >= THREAD_SIZE);
> >>
> >> Yes, I am aware that it is not equivalent to the existing condition
> >> - it uses the fact that this_cpu_sp0(), previous check
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Oops...
> I meant to say "...the fact that this_cpu_sp0() points to the very top
> of the stack, previous check ..."
>
> >> wasn't making that assumption. But that assumption is true,
> >> so shouldn't be a problem.
> >
> > You're missing an absolute value in here, though. This isn't a check
> > for overflow; it's a check that we aren't on an IST or other per cpu
> > stack.
>
> Yes, that's exactly what the condition checks for. It reads
>
> "is current stack pointer below task's kernel stack by no more
> than THREAD_SIZE?"
>
> which is only possible if we are on task's kernel stack:
>
> If current_stack_pointer() is elsewhere, it is either
> (a) much smaller than this_cpu_sp0(), and BUG_ON condition
> obviously triggers; or
> (b) it is somewhere above this_cpu_sp0(), in which case subtraction
> overflows and condition triggers too.
>
Right, it's unsigned. I like yours better if I add a comment. It
also avoids relying on the stack being aligned to its full size.
--Andy
> How would you write this condition so that it's easily readable?
> Evidently, my version isn't as readable sa I hoped...