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:   Wed, 7 Dec 2022 09:58:36 -0500
From:   Peter Xu <peterx@redhat.com>
To:     John Hubbard <jhubbard@nvidia.com>
Cc:     Mike Kravetz <mike.kravetz@oracle.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        James Houghton <jthoughton@google.com>,
        Jann Horn <jannh@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Rik van Riel <riel@surriel.com>,
        Nadav Amit <nadav.amit@gmail.com>,
        Miaohe Lin <linmiaohe@huawei.com>,
        Muchun Song <songmuchun@bytedance.com>,
        David Hildenbrand <david@redhat.com>
Subject: Re: [PATCH 08/10] mm/hugetlb: Make walk_hugetlb_range() safe to pmd
 unshare
Message-ID: <Y5CqHIp3nsJfnwAm@x1n>
References: <20221129193526.3588187-1-peterx@redhat.com>
 <20221129193526.3588187-9-peterx@redhat.com>
 <Y45/5ezH6uwn7Cfy@monkey>
 <0813b9ed-3c92-088c-4fb9-45fb648c6e73@nvidia.com>
 <Y49xlV8I2/92Flha@x1n>
 <97e3a8f2-df75-306e-2edf-85976c168955@nvidia.com>
 <Y4+5R+nh0W0RUX9R@x1n>
 <c5d839da-cb7e-e887-11a6-30ccd1c3c845@nvidia.com>
 <Y4/ZT3ab9TL1j5TL@x1n>
 <3f032921-c373-e942-a857-4328d7993ef0@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <3f032921-c373-e942-a857-4328d7993ef0@nvidia.com>
Precedence: bulk

On Tue, Dec 06, 2022 at 06:38:54PM -0800, John Hubbard wrote:
> On 12/6/22 16:07, Peter Xu wrote:
> > I thought I answered this one at [1] above.  If not, I can extend the
> > answer.
> 
> [1] explains it, but it doesn't mention why it's safe to drop and reacquire.
> 
> ...
> > 
> > If we touch it, it's a potential bug as you mentioned.  But we didn't.
> > 
> > Hope it explains.
> 
> I think it's OK after all, because hmm_vma_fault() does revalidate after
> it takes the vma lock, so that closes the loop that I was fretting over.
> 
> I was just also worried that I'd missed some other place, but it looks
> like that's not the case.
> 
> So, good.
> 
> How about this incremental diff on top, as an attempt to clarify what's
> going on? Or is this too much wordage? Sometimes I write too many words:

Nop, that all looks good, thanks.  I'll apply them in my new post.

> 
> 
> diff --git a/include/linux/pagewalk.h b/include/linux/pagewalk.h
> index 1f7c2011f6cb..27a6df448ee5 100644
> --- a/include/linux/pagewalk.h
> +++ b/include/linux/pagewalk.h
> @@ -21,13 +21,16 @@ struct mm_walk;
>   *			depth is -1 if not known, 0:PGD, 1:P4D, 2:PUD, 3:PMD.
>   *			Any folded depths (where PTRS_PER_P?D is equal to 1)
>   *			are skipped.
> - * @hugetlb_entry:	if set, called for each hugetlb entry.	Note that
> - *			currently the hook function is protected by hugetlb
> - *			vma lock to make sure pte_t* and the spinlock is valid
> - *			to access.  If the hook function needs to yield the
> - *			thread or retake the vma lock for some reason, it
> - *			needs to properly release the vma lock manually,
> - *			and retake it before the function returns.
> + * @hugetlb_entry:	if set, called for each hugetlb entry. This hook
> + *			function is called with the vma lock held, in order to
> + *			protect against a concurrent freeing of the pte_t* or
> + *			the ptl. In some cases, the hook function needs to drop
> + *			and retake the vma lock in order to avoid deadlocks
> + *			while calling other functions. In such cases the hook
> + *			function must either refrain from accessing the pte or
> + *			ptl after dropping the vma lock, or else revalidate
> + *			those items after re-acquiring the vma lock and before
> + *			accessing them.
>   * @test_walk:		caller specific callback function to determine whether
>   *			we walk over the current vma or not. Returning 0 means
>   *			"do page table walk over the current vma", returning
> diff --git a/mm/hmm.c b/mm/hmm.c
> index dcd624f28bcf..b428f2011cfd 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -497,7 +497,13 @@ static int hmm_vma_walk_hugetlb_entry(pte_t *pte, unsigned long hmask,
>  		spin_unlock(ptl);
>  		hugetlb_vma_unlock_read(vma);
> -		/* hmm_vma_fault() can retake the vma lock */
> +		/*
> +		 * Avoid deadlock: drop the vma lock before calling
> +		 * hmm_vma_fault(), which will itself potentially take and drop
> +		 * the vma lock. This is also correct from a protection point of
> +		 * view, because there is no further use here of either pte or
> +		 * ptl after dropping the vma lock.
> +		 */
>  		ret = hmm_vma_fault(addr, end, required_fault, walk);
>  		hugetlb_vma_lock_read(vma);
>  		return ret;
> 
> > > I guess it's on me to think of something cleaner, so if I do I'll pipe
> > > up. :)
> > 
> > That'll be very much appricated.
> > 
> > It's really that I don't know how to make this better, or I can rework the
> > series as long as it hasn't land upstream.
> > 
> 
> It's always 10x easier to notice an imperfection, than it is to improve on
> it. :)

-- 
Peter Xu