From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1756704AbbE2Ssn (ORCPT <rfc822;w@1wt.eu>);
	Fri, 29 May 2015 14:48:43 -0400
Received: from mail-wi0-f175.google.com ([209.85.212.175]:36269 "EHLO
	mail-wi0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752882AbbE2Ssf (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 29 May 2015 14:48:35 -0400
Date: Fri, 29 May 2015 20:48:29 +0200
From: Ingo Molnar <mingo@kernel.org>
To: Dave Hansen <dave@sr71.net>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org, tglx@linutronix.de,
        dave.hansen@linux.intel.com, oleg@redhat.com, bp@alien8.de,
        riel@redhat.com, sbsiddha@gmail.com, luto@amacapital.net,
        mingo@redhat.com, hpa@zytor.com, fenghua.yu@intel.com
Subject: Re: [PATCH 02/19] x86, fpu: Wrap get_xsave_addr() to make it safer
Message-ID: <20150529184829.GB27501@gmail.com>
References: <20150527183609.964CC3BA@viggo.jf.intel.com>
 <20150527183610.56178C96@viggo.jf.intel.com>
 <20150528084140.GA31719@gmail.com>
 <5568981C.7080605@sr71.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5568981C.7080605@sr71.net>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


* Dave Hansen <dave@sr71.net> wrote:

> On 05/28/2015 01:41 AM, Ingo Molnar wrote:
> >> > +	union fpregs_state *xstate;
> >> > +
> >> > +	if (!current->thread.fpu.fpstate_active)
> >> > +		return NULL;
> >> > +	/*
> >> > +	 * fpu__save() takes the CPU's xstate registers
> >> > +	 * and saves them off to the 'fpu memory buffer.
> >> > +	 */
> >> > +	fpu__save(&current->thread.fpu);
> >> > +	xstate = &current->thread.fpu.state;
> >> > +
> >> > +	return get_xsave_addr(&xstate->xsave, xsave_state);
> > Small nit, this would become a lot shorter if you introduced a helper local 
> > variable:
> > 
> > 	struct fpu *fpu = &current->thread.fpu;
> > 
> > But more importantly, for a generic get_xsave_field_ptr() API, fpu__save() is 
> > not enough: fpu__save() will only save FPU registers into memory if necessary 
> > (i.e. if the FPU is already in use), and if you call it on a task with no FPU 
> > state then it will still have an !fpu->fpstate_active FPU state after the 
> > call, with random, invalid data in the xsave area.
> 
> But why does this matter?  We just did a !fpu.fpstate_active check, so we can't 
> have a !fpu.fpstate_active before or after the call.

Ah yes, you are right, I missed this:

> >> > +	if (!current->thread.fpu.fpstate_active)
> >> > +		return NULL;

because the usual pattern is:

		if (!fpu->fpstate_active)
			return NULL;

:-)

So your variant is fine too.

Thanks,

	Ingo