Hi, Tony,
> void *begin_update_one_xsave_feature(struct task_struct *tsk,
> enum xfeature xfeature, bool full) {
> struct xregs_state *xsave = &tsk->thread.fpu.state.xsave;
> struct xregs_state *xinit = &init_fpstate.xsave;
> u64 fmask = 1ull << xfeature;
> void *addr;
>
> BUG_ON(!(xsave->header.xcomp_bv & fmask));
>
> fpregs_lock();
I'm afraid we may hit the same locking issue when we send IPI to notify another
task to modify its
PASID state. Here the API is called to modify another running task's PASID
state as well without a right lock.
fpregs_lock() is not enough to deal with this, I'm afraid.
Quote from Thomas in
https://lore.kernel.org/linux-iommu/87mtsd6gr9....@nanos.tec.linutronix.de/
"FPU state of a running task is protected by fregs_lock() which is
nothing else than a local_bh_disable(). As BH disabled regions run
usually with interrupts enabled the IPI can hit a code section which
modifies FPU state and there is absolutely no guarantee that any of the
assumptions which are made for the IPI case is true."
Maybe restrict the API's scope: don't modify another task's FPU state, just the
current task's state?
> addr = __raw_xsave_addr(xsave, xfeature);
>
> if (full || tsk != current) {
> memcpy(addr, __raw_xsave_addr(xinit, xfeature),
> xstate_sizes[xfeature]);
> goto out;
> }
>
> if (!(xsave->header.xfeatures & fmask)) {
> xsave->header.xfeatures |= fmask; //<<<<<
> xsaves(xsave, fmask);
> }
>
> out:
> xsave->header.xfeatures |= fmask;
> return addr;
> }
>
> void finish_update_one_xsave_feature(struct task_struct *tsk) {
> set_ti_thread_flag(task_thread_info(tsk), TIF_NEED_FPU_LOAD);
> if (tsk == current) //<<<<<
> __cpu_invalidate_fpregs_state(); //<<<<<
> fpregs_unlock();
> }
Thanks.
-Fenghua
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
