Re: [PATCH v4 1/7] x86/fsgsbase/64: Introduce FS/GS base helper functions

[Thomas Gleixner]

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