lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Wed, 30 Nov 2016 23:10:27 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Fenghua Yu <fenghua.yu@...el.com>
cc:     "H. Peter Anvin" <h.peter.anvin@...el.com>,
        Ingo Molnar <mingo@...e.hu>,
        Marcelo Tosatti <mtosatti@...hat.com>,
        Tony Luck <tony.luck@...el.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        Sai Prakhya <sai.praneeth.prakhya@...el.com>,
        Vikas Shivappa <vikas.shivappa@...ux.intel.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>
Subject: Re: [PATCH] x86/intel_rdt.c: Disable preempt for intel_rdt_sched_in()
 in move_myself()

On Wed, 30 Nov 2016, Fenghua Yu wrote:
> From: Fenghua Yu <fenghua.yu@...el.com>

Subject: x86/intel_rdt.c: Disable preempt for intel_rdt_sched_in() in move_myself()

- The prefix is crap. git log arch/x86/kernel/cpu/intel_rdt.c should tell
  you what's the proper prefix is.

It's preemption not preempt and this wants to be a proper sentence:

     x86/intel_rdt: Call intel_rdt_sched_in() with preemption disabled

> intel_rdt_sched_in() calls this_cpu_ptr() read and write pqr_state and
> update PQR_ASSOC on current cpu. If execution of the function is preempted
> and switched to another CPU, the results are wrong. If 
> CONFIG_DEBUG_PREEMPT is turned on, the issue is reported as
> "BUG: smp_processor_id() running in preemptible code." when moving a task
> to a rdtgroup in move_myself().

When will you finally start to describe problems proper instead of
telling fairy tales about how you debugged the symptoms?

  intel_rdt_sched_in() must be called with preemption disabled because the
  function accesses percpu variables (pqr_state and closid).

  If a task moves itself via move_myself() preemption is enabled, which
  violates the calling convention and can result in incorrect closid
  selection when the task gets preempted or migrated. 

  Add the required protection and a comment about the calling convention.

That's concise and precise.

> Disable preempt before intel_rdt_sched_in() and eanble preempt after

s/preempt/preemtion/
s/eanble/enable/

Oh well.

> the function in move_myself() to solve the issue.

And next time you write: Add one before foo() and subtract one after the
function ....

Changelogs need to tell the WHY and not the WHAT. We can see the WHAT from
looking at the patch.

> Signed-off-by: Fenghua Yu <fenghua.yu@...el.com>
> ---
>  arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/include/asm/intel_rdt.h b/arch/x86/include/asm/intel_rdt.h
> index 6e90e87..eaaa765 100644
> --- a/arch/x86/include/asm/intel_rdt.h
> +++ b/arch/x86/include/asm/intel_rdt.h
> @@ -192,6 +192,9 @@ int rdtgroup_schemata_show(struct kernfs_open_file *of,
>   *   resctrl file system.
>   * - Caches the per cpu CLOSid values and does the MSR write only
>   *   when a task with a different CLOSid is scheduled in.
> + * - Caller needs to disable preempt before calling this function.

  s/preempt/preemtion/

  * - Must be called with preemption disabled.

> + *   The function doesn't check preemptible. Scheduler hot path
> + *   disables preempt already.

This part makes no sense whatsoever. -ENOPARSE

The important information is the calling convention, i.e.:

  * - Must be called with preemption disabled.

and that's enough.

>   */
>  static inline void intel_rdt_sched_in(void)
>  {
> diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> index fb8e03e..9c6f732 100644
> --- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> +++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> @@ -326,8 +326,10 @@ static void move_myself(struct callback_head *head)
>  		kfree(rdtgrp);
>  	}
>  
> +	get_cpu();

Bah! What's wrong with preempt_disable()?

>  	/* update PQR_ASSOC MSR to make resource group go into effect */
>  	intel_rdt_sched_in();
> +	put_cpu();
>  
>  	kfree(callback);
>  }

Thanks,

	tglx

Powered by blists - more mailing lists