Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RFC PATCH 08/13] x86/process/64: Clean up uintr task fork and
 exit paths
To:     Thomas Gleixner <tglx@linutronix.de>, <x86@kernel.org>
CC:     Tony Luck <tony.luck@intel.com>,
        Dave Hansen <dave.hansen@intel.com>,
        "Ingo Molnar" <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        "H . Peter Anvin" <hpa@zytor.com>,
        Andy Lutomirski <luto@kernel.org>,
        Jens Axboe <axboe@kernel.dk>,
        Christian Brauner <christian@brauner.io>,
        Peter Zijlstra <peterz@infradead.org>,
        Shuah Khan <shuah@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
        Jonathan Corbet <corbet@lwn.net>,
        Ashok Raj <ashok.raj@intel.com>,
        Jacob Pan <jacob.jun.pan@linux.intel.com>,
        "Gayatri Kammela" <gayatri.kammela@intel.com>,
        Zeng Guang <guang.zeng@intel.com>,
        "Dan Williams" <dan.j.williams@intel.com>,
        Randy E Witt <randy.e.witt@intel.com>,
        Ravi V Shankar <ravi.v.shankar@intel.com>,
        Ramesh Thomas <ramesh.thomas@intel.com>,
        <linux-api@vger.kernel.org>, <linux-arch@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-kselftest@vger.kernel.org>
References: <20210913200132.3396598-1-sohil.mehta@intel.com>
 <20210913200132.3396598-9-sohil.mehta@intel.com> <8735pug50i.ffs@tglx>
From:   Sohil Mehta <sohil.mehta@intel.com>
Message-ID: <b58d369d-f4a7-f4e4-51ea-88cbfc0d8ac3@intel.com>
Date:   Mon, 27 Sep 2021 18:23:11 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Firefox/78.0 Thunderbird/78.14.0
In-Reply-To: <8735pug50i.ffs@tglx>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VllXNjJpSnN3b1Mrd25KaDlMdW1oRHhOdnpIQmE4aVJvZ3EvWTI5YUMvbit2?=
 =?utf-8?B?OUJnM2Q3YUhLY0lsTWI0LzNmZ2pFYjFWSXVJYy9qa0NDdEYwK2F3M0xtOTJQ?=
 =?utf-8?B?dmJQbVhtTXVpdUtOOG9SL2ZrTVJ2anY4RmRsM21TRjZjNVVodGhmUDZrUkdX?=
 =?utf-8?B?SU9RYWFDbVRMQUxhd3Z1K3dBdmcyZEtVNzJ1TFl2d3lnaHM4anVVaHJQUFpy?=
 =?utf-8?B?RGZ1N3ZiMUhGRmFjalhmQ2dJYWxBbFZkcFpMQnBHNkxrNno3VzZCb2dLNmRi?=
 =?utf-8?B?UStjaWVGNjBuWjFPM25NTWMyYnhXTnd4UVVxTmJXVnRMMUhDdGlxcmVFRzFt?=
 =?utf-8?B?WlJJd2pKT0NZbmpRai9jTEpTanczVmJDcUhuU0lnU3Zma0JHVTN6WTM1b2l3?=
 =?utf-8?B?b3AyenJiMDhnN2prU1NselNRZlFkOEU1ZUhsRkk0S0hIZ3k5aGduQ2VUU2hH?=
 =?utf-8?B?YkFiL0hCSDQrc21vcTQyOUxLZWNYS2xUOFVwSDFyd094cFE0NTlyZGVtYWN6?=
 =?utf-8?B?ek1XUTBkRW1nS0JoOTdvd3QrbGdSOE51R0tiT1g0OGpNbThPb0RTbEZWSFBp?=
 =?utf-8?B?NWMvUk1aemZxSDdxT0Z6R1RQVjgyakJPSVRscDVFSCtYdWszdmN3MFRtWGdU?=
 =?utf-8?B?bUV0WFNmWDNvOHhyVm9Mc1R5NGJaeXJ5SnV2L2wweFJBRW1acG4rd3FNUTBD?=
 =?utf-8?B?VCszamNCcGpGNzlwbStoN2lLVDA4SjZtcXg0OEFIeDZ3K0ZROTRBbzU3ZFJV?=
 =?utf-8?B?RE10YzNjdG42dFNHU3RXNkFGQThZRWRUT0dnYzZvVHpTcHdTTTk2Qm5vbW85?=
 =?utf-8?B?VXh5MmFxN0x4R3Y5N3g1dXh6UXlOd0lLR2YyOEd1TFlGVHR5SEgyME1ITFBh?=
 =?utf-8?B?N0s5dXE0UnFDdVZERk1pQUpxak4zM2ZkQnd2REh0eHRaQUdJaGxqWDlqUkJD?=
 =?utf-8?B?dHMyRlVvZmdaOW81dWNqbmZEcnIxUkRnbDh4alRMNk84a1oxbVNaMURHd2pS?=
 =?utf-8?B?SUUyK2t1cXVMK1BhWjNHekg5M0RlVEZMWklRL2IyNVpWaWQxTEJ0eEhpUmhS?=
 =?utf-8?B?WDduNUFNVjYyaHBOSkJGRWJTSlVqTUtTanNHUnBTODRyT3RkS2VMZEx3dlRz?=
 =?utf-8?B?RnpJZUtYZ3VnNEZ5bTdBWWFjMWRVSzlUV1FhMVdmdUlNbXpjZXhVNzBtZStp?=
 =?utf-8?B?bEhzSnF0WlJKTWd1R1dGVkozdzlSUHl2c2tTbTZOeFgzOXBYenBYWXlwTjNC?=
 =?utf-8?B?L2l4a0NRQnpKQ3QxMlVPU09mUzRFRXdJYVVIbUdTa1NlSk9HVUY1OFIxUStm?=
 =?utf-8?B?eWJOMU5mSkQ5bFRCemJhbGd6VkZzS1FIbFR5TDNFb1BMNGI2Q05oMkRWMndW?=
 =?utf-8?B?M2tsb2tWV0JuNnFXU3F3bmUyNEFrUk1Pb0tFNmo0U0JseUdQWldYcE43a2pK?=
 =?utf-8?B?dlE0VmF3MkgycStnRjZYQlY5K2FUWjE4aDhtNU04aXp6WnFtVDgxemR0cHBa?=
 =?utf-8?B?NE43SUNnZEsrR2liRi9zQmNadU16ZklnNE81cHFhTDM0TTFndW50MzhLYzZ3?=
 =?utf-8?B?ZENWUmkwQUdBREUrcURQUWRiVkVlV1IyNDZscmNWRkR6VTB3UHRCdy9neUEv?=
 =?utf-8?B?TFFMYXp1YmxtNHdWRnVSaSt0OEYwaTAvSlZjREtNd2hCMmNYWXVWUU5UcGE3?=
 =?utf-8?B?ZmRETi9zeGwrYnFXS3BFYTdRalRETi84N3Evd0phVVFEMnEwTTlEbXlsU1dS?=
 =?utf-8?Q?/9vfqqu23Y2OXwKJOx8Fzvmfb9qDTXtQF2qQ+n3?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 18f5fd02-50ed-46cd-0bbf-08d9821e8ab9
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3320.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Sep 2021 01:23:14.1226
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 7X7A0NK07p1+ywWR7ZUudi8YMj0X7BjkPsik1KZrtgEWSDKKFw0Qinbbwy15iwLgKUhw39AzIYdk3lgubETrPA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR11MB3494
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 9/23/2021 6:02 PM, Thomas Gleixner wrote:
> On Mon, Sep 13 2021 at 13:01, Sohil Mehta wrote:
>
>> The user interrupt MSRs and the user interrupt state is task specific.
>> During task fork and exit clear the task state, clear the MSRs and
>> dereference the shared resources.
>>
>> Some of the memory resources like the UPID are referenced in the file
>> descriptor and could be in use while the uintr_fd is still valid.
>> Instead of freeing up  the UPID just dereference it.
> Derefencing the UPID, i.e. accessing task->upid->foo helps in which way?
>
> You want to drop the reference count I assume. Then please write that
> so.


Ah! Not sure how I associated dereference to dropping the reference. 
Will update this.

>
>> @@ -260,6 +260,7 @@ int fpu_clone(struct task_struct *dst)
>>   {
>>   	struct fpu *src_fpu = &current->thread.fpu;
>>   	struct fpu *dst_fpu = &dst->thread.fpu;
>> +	struct uintr_state *uintr_state;
>>   
>>   	/* The new task's FPU state cannot be valid in the hardware. */
>>   	dst_fpu->last_cpu = -1;
>> @@ -284,6 +285,14 @@ int fpu_clone(struct task_struct *dst)
>>   
>>   	else
>>   		save_fpregs_to_fpstate(dst_fpu);
>> +
>> +	/* UINTR state is not expected to be inherited (in the current design). */
>> +	if (static_cpu_has(X86_FEATURE_UINTR)) {
>> +		uintr_state = get_xsave_addr(&dst_fpu->state.xsave, XFEATURE_UINTR);
>> +		if (uintr_state)
>> +			memset(uintr_state, 0, sizeof(*uintr_state));
>> +	}
> 1) If the FPU registers are up to date then this can be completely
>     avoided by excluding the UINTR component from XSAVES

You mentioned this in the other thread that the UINTR state must be 
invalidated during fpu_clone().

I am not sure if understand all the nuances here. Your suggestion seems 
valid to me. I'll have to think more about this.

> 2) If the task never used that muck then UINTR is in init state and
>     clearing that memory is a redunant exercise because it has been
>     cleared already

Yes. I'll add a check for that.

>> + * exit_thread() can happen in current context when the current thread is
>> + * exiting or it can happen for a new thread that is being created.
> A right that makes sense. If a new thread is created then it can call
> exit_thread(), right?


What I meant here is that exit_thread() can also be called during 
copy_process() if it runs into an issue.

bad_fork_cleanup_thread:

     exit_thread();

In this case is_uintr_receiver() will fail. I'll update the comments to 
reflect that.

>> + * For new threads is_uintr_receiver() should fail.
> Should fail?

Thanks,

Sohil