From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965737AbbDVPd1 (ORCPT ); Wed, 22 Apr 2015 11:33:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41026 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964840AbbDVPd0 (ORCPT ); Wed, 22 Apr 2015 11:33:26 -0400 Date: Wed, 22 Apr 2015 17:32:45 +0200 From: Oleg Nesterov To: Borislav Petkov Cc: Dave Hansen , linux-kernel@vger.kernel.org, x86@kernel.org, tglx@linutronix.de, dave.hansen@linux.intel.com, riel@redhat.com, sbsiddha@gmail.com, luto@amacapital.net, mingo@redhat.com, hpa@zytor.com, fenghua.yu@intel.com Subject: Re: [PATCH 01/16] x86, fpu: wrap get_xsave_addr() to make it safer Message-ID: <20150422153245.GA22825@redhat.com> References: <20150401004623.894DF37A@viggo.jf.intel.com> <20150401004624.49096AD0@viggo.jf.intel.com> <20150422104047.GA6897@pd.tnic> <20150422131618.GA16785@redhat.com> <20150422133146.GE6897@pd.tnic> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150422133146.GE6897@pd.tnic> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/22, Borislav Petkov wrote: > > On Wed, Apr 22, 2015 at 03:16:18PM +0200, Oleg Nesterov wrote: > > I agree, tsk_used_math(tsk) looks better, simpy because we have this > > argument. > > > > But this "tsk" should be always current, otherwise this code is wrong > > This is exactly what I'm asking: is that always the case?... I can't look at these patches now, but iirc - yes. The caller is either prctl() or exception. Dave will correct me. Otherwise, once again, this code is simply buggy. So the comment should probably explain this. > > > Because used_math() is looking at current, maybe even in > > > preemption-enabled paths - I'm eyeing task_get_bounds_dir() - and > > > that current might get changed from under us and it might happen that > > > current != tsk. Yes, no? > > > > Not sure I understand... "current" can't change from under us? > > ... I'm not sure all tsk_get_xsave_field() callers disable preemption. > If not, then current can change from under us... How? I am certainly missing you point... OK, please forge about FPU. Consider this code: tsk = current; for (;;) BUG_ON(tsk != current); it doesn't need to disable preemption. We do not care if CPU switches to another thread, even if this thread executes the same code. Because its tsk/current will differ, but "tsk == current" will be still true. Could you please spell? > > Even if this CPU switches to another thread which executes the same code, > > that thread will obviously see another "current", but its "tsk" variable > > will still match its "current". > > Well, we want to see if @tsk used math, not necessarily if current used > math, especially if it is another task, right? See above... used_math() should be correct because we know that tsk==current, but I agree that tsk_used_math(tsk) looks better. > I read tsk_get_xsave_field(@tsk, ) as give me the xsave field of @tsk > but doing used_math() we're querying current and I'm not sure > > tsk == current > > in all the call sites of tsk_get_xsave_field(). Yes, the name/comment looks confusing a bit, as if you can use it when tsk != current... Oleg.