Re: [RFC PATCH 08/13] x86/process/64: Clean up uintr task fork and exit paths

[Sohil Mehta]

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