On Wed, 20 Jun 2018, Chang S. Bae wrote:
> +void write_fsbase(unsigned long fsbase)
> +{
> + /* set the selector to 0 to not confuse __switch_to */
'to not confuse __switch_to' is not that helpful of a comment as it
requires to stare into __switch_to to figure out what might get confused
there. Please write it out why this needs to be done in technical terms.
> + loadseg(FS, 0);
> + wrmsrl(MSR_FS_BASE, fsbase);
> +}
> +
> +void write_inactive_gsbase(unsigned long gsbase)
> +{
> + /* set the selector to 0 to not confuse __switch_to */
Ditto
> + loadseg(GS, 0);
> + wrmsrl(MSR_KERNEL_GS_BASE, gsbase);
> +}
> +
> +unsigned long read_task_fsbase(struct task_struct *task)
> +{
> + unsigned long fsbase;
> +
> + if (task == current) {
> + fsbase = read_fsbase();
> + } else {
> + /*
> + * XXX: This will not behave as expected if called
> + * if fsindex != 0. This preserves an existing bug
> + * that will be fixed.
I'm late to this party, but let me ask the obvious question:
Why is the existing bug not fixed as the first patch in the series?
We do not preserve bugs when adding new stuff as that makes it a pain to
backport the fix.
> +int write_task_fsbase(struct task_struct *task, unsigned long fsbase)
> +{
> + int cpu;
> +
> + /*
> + * Not strictly needed for fs, but do it for symmetry
> + * with gs
> + */
> + if (unlikely(fsbase >= TASK_SIZE_MAX))
> + return -EPERM;
> +
> + cpu = get_cpu();
What's the point of using get_cpu()? There is nothing at all which needs
'cpu' here. The only point is to prevent preemption, then please use
preempt_disable() and not some random function which happens to disable
preemption underneath.
> + task->thread.fsbase = fsbase;
> + if (task == current)
> + write_fsbase(fsbase);
> + task->thread.fsindex = 0;
> + put_cpu();
> +
> + return 0;
> +}
> +
> +int write_task_gsbase(struct task_struct *task, unsigned long gsbase)
> +{
> + int cpu;
> +
> + if (unlikely(gsbase >= TASK_SIZE_MAX))
> + return -EPERM;
> +
> + cpu = get_cpu();
Ditto
> + task->thread.gsbase = gsbase;
> + if (task == current)
> + write_inactive_gsbase(gsbase);
> + task->thread.gsindex = 0;
> + put_cpu();
> +
> + return 0;
> +}
> +
> int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
> unsigned long arg, struct task_struct *p, unsigned long tls)
> {
> @@ -618,54 +707,27 @@ static long prctl_map_vdso(const struct vdso_image
> *image, unsigned long addr)
> long do_arch_prctl_64(struct task_struct *task, int option, unsigned long
> arg2)
> {
> int ret = 0;
> - int doit = task == current;
> - int cpu;
>
> switch (option) {
> - case ARCH_SET_GS:
> - if (arg2 >= TASK_SIZE_MAX)
> - return -EPERM;
> - cpu = get_cpu();
Ah. You copied it from here where it makes no sense either. Copy and paste
is useful, but you really want to think about it.
> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> index e2ee403..c53c2bcf6 100644
> --- a/arch/x86/kernel/ptrace.c
> +++ b/arch/x86/kernel/ptrace.c
....
> if (child->thread.fsbase != value)
> - return do_arch_prctl_64(child, ARCH_SET_FS, value);
> + return write_task_fsbase(child, value);
This patch wants to be split into:
1) Adding the new functions
2) Convert vdso
3) Convert ptrace
_AFTER_ fixing the existing bug.
Thanks,
tglx
