Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 27 Apr 2023 14:01:27 +0200
From:   Heiko Carstens <hca@linux.ibm.com>
To:     Claudio Imbrenda <imbrenda@linux.ibm.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-s390@vger.kernel.org, frankja@linux.ibm.com,
        borntraeger@de.ibm.com, nrb@linux.ibm.com, nsg@linux.ibm.com,
        seiden@linux.ibm.com, jgg@nvidia.com
Subject: Re: [PATCH v2 1/1] KVM: s390: fix race in gmap_make_secure
Message-ID: <ZEpkFzFFEpqa9zMv@osiris>
References: <20230426134834.35199-1-imbrenda@linux.ibm.com>
 <20230426134834.35199-2-imbrenda@linux.ibm.com>
 <ZEpUEF7H86E9vVfS@osiris>
 <20230427134649.1e482d91@p-imbrenda>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230427134649.1e482d91@p-imbrenda>
Precedence: bulk

On Thu, Apr 27, 2023 at 01:46:49PM +0200, Claudio Imbrenda wrote:
> On Thu, 27 Apr 2023 12:53:04 +0200
> Heiko Carstens <hca@linux.ibm.com> wrote:
> 
> > On Wed, Apr 26, 2023 at 03:48:34PM +0200, Claudio Imbrenda wrote:
> > > This patch fixes a potential race in gmap_make_secure and removes the
> > > last user of follow_page without FOLL_GET.
> > > 
> > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> > > Fixes: 214d9bbcd3a6 ("s390/mm: provide memory management functions for protected KVM guests")
> > > ---
> > >  arch/s390/kernel/uv.c | 32 +++++++++++---------------------
> > >  1 file changed, 11 insertions(+), 21 deletions(-)  
> > 
> > It would be helpful if this would be a bit more descriptive. "Fix
> > race" is not very helpful :)
> > 
> > What race does this fix?
> > When can this happen?
> > What are the consequences if the race window is being hit?
> 
> We are locking something we don't have a reference to, and as explained
> by Jason and David in this thread <Y9J4P/RNvY1Ztn0Q@nvidia.com> it can
> lead to all kind of bad things, including the page getting
> unmapped (MADV_DONTNEED), freed, reallocated as a larger folio and the
> unlock_page() would target the wrong bit.
> 
> Also there is another race with the FOLL_WRITE, which could race
> between the follow_page and the get_locked_pte.
> 
> The main point of the patch is to remove the last follow_page without
> FOLL_GET or FOLL_PIN, removing the races can be considered a nice bonus.

I've seen that discussion. What I'm actually asking for is that all of
this information should be added to the commit description. Nobody
will remember any of the details in one year.