linux-kernel - Re: [PATCH v3 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yf3FzOUSRpFUdM/q@otcwcpicx3.sc.intel.com>
Date:   Fri, 4 Feb 2022 16:33:16 -0800
From:   Fenghua Yu <fenghua.yu@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Dave Hansen <dave.hansen@...ux.intel.com>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Andy Lutomirski <luto@...nel.org>,
        Tony Luck <tony.luck@...el.com>,
        Lu Baolu <baolu.lu@...ux.intel.com>,
        Joerg Roedel <joro@...tes.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Jacob Pan <jacob.jun.pan@...ux.intel.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        iommu@...ts.linux-foundation.org, x86 <x86@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 05/11] iommu/sva: Assign a PASID to mm on PASID
 allocation and free it on mm exit

On Sat, Feb 05, 2022 at 12:56:00AM +0100, Thomas Gleixner wrote:
> On Fri, Jan 28 2022 at 12:28, Fenghua Yu wrote:
> > To avoid complexity of updating each thread's PASID status (e.g. sending
> > IPI to update IA32_PASID MSR) on allocating and freeing PASID, once
> > allocated and assigned to an mm, the PASID stays with the mm for the
> > rest of the mm's lifetime. A reference to the PASID is taken on
> > allocating the PASID. Binding/unbinding the PASID won't change refcount.
> > The reference is dropped on mm exit and thus the PASID is freed.
> >
> > Two helpers mm_pasid_set() and mm_pasid_drop() are defined in mm because
> > the PASID operations handle the pasid member in mm_struct and should be
> > part of mm operations. Because IOASID's reference count is not used any
> > more and removed, unused ioasid_get() and iommu_sva_free_pasid()
> > are deleted and ioasid_put() is renamed to ioasid_free().
> >
> > 20-bit PASID allows up to 1M processes bound to PASIDs at the same time.
> > With cgroups and other controls that might limit the number of process
> > creation, the limited number of PASIDs is not a realistic issue for
> > lazy PASID free.
> 
> Please take a step back and think hard about it whether that changelog
> makes sense to you a year from now.
> 
> Let me walk you through:
> 
> > To avoid complexity of updating each thread's PASID status (e.g. sending
> > IPI to update IA32_PASID MSR) on allocating and freeing PASID, once
> > allocated and assigned to an mm, the PASID stays with the mm for the
> > rest of the mm's lifetime.
> 
> You are missing the oportunity to tell a story about the history of this
> decision here:
> 
>   PASIDs are process wide. It was attempted to use refcounted PASIDs to
>   free them when the last thread drops the refcount. This turned out to
>   be complex and error prone. Given the fact that the PASID space is 20
>   bits, which allows up to 1M processes to have a PASID associated
>   concurrently, PASID resource exhaustion is not a realistic concern.
> 
>   Therefore it was decided to simplify the approach and stick with lazy
>   on demand PASID allocation, but drop the eager free approach and make
>   a allocated PASID lifetime bound to the life time of the process.
> 
> > A reference to the PASID is taken on allocating the
> > PASID. Binding/unbinding the PASID won't change refcount.  The
> > reference is dropped on mm exit and thus the PASID is freed.
> 
> There is no refcount in play anymore, right? So how does this part of
> the changelog make any sense?
> 
> This is followed by:
> 
> > Two helpers mm_pasid_set() and mm_pasid_drop() are defined in mm because
> > the PASID operations handle the pasid member in mm_struct and should be
> > part of mm operations. Because IOASID's reference count is not used any
> > more and removed, unused ioasid_get() and iommu_sva_free_pasid()
> > are deleted and ioasid_put() is renamed to ioasid_free().
> 
> which does not provide much rationale and just blurbs about _what_ the
> patch is doing and not about the why and the connection to the
> above. And the refcount removal section contradicts the stale text
> above.
> 
> So this paragraph should be something like this:
> 
>   Get rid of the refcounting mechanisms and replace/rename the
>   interfaces to reflect this new approach.
> 
> Hmm?

Sure. I will update the commit message with your comments.

Thanks.

-Fenghua