Message-ID: <20190328220824.GE13560@redhat.com>
Date:   Thu, 28 Mar 2019 18:08:24 -0400
From:   Jerome Glisse <jglisse@...hat.com>
To:     John Hubbard <jhubbard@...dia.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dan Williams <dan.j.williams@...el.com>
Subject: Re: [PATCH v2 10/11] mm/hmm: add helpers for driver to safely take
 the mmap_sem v2

On Thu, Mar 28, 2019 at 02:41:02PM -0700, John Hubbard wrote:
> On 3/28/19 2:30 PM, Jerome Glisse wrote:
> > On Thu, Mar 28, 2019 at 01:54:01PM -0700, John Hubbard wrote:
> >> On 3/25/19 7:40 AM, jglisse@...hat.com wrote:
> >>> From: Jérôme Glisse <jglisse@...hat.com>
> >>>
> >>> The device driver context which holds reference to mirror and thus to
> >>> core hmm struct might outlive the mm against which it was created. To
> >>> avoid every driver to check for that case provide an helper that check
> >>> if mm is still alive and take the mmap_sem in read mode if so. If the
> >>> mm have been destroy (mmu_notifier release call back did happen) then
> >>> we return -EINVAL so that calling code knows that it is trying to do
> >>> something against a mm that is no longer valid.
> >>>
> >>> Changes since v1:
> >>>     - removed bunch of useless check (if API is use with bogus argument
> >>>       better to fail loudly so user fix their code)
> >>>
> >>> Signed-off-by: Jérôme Glisse <jglisse@...hat.com>
> >>> Reviewed-by: Ralph Campbell <rcampbell@...dia.com>
> >>> Cc: Andrew Morton <akpm@...ux-foundation.org>
> >>> Cc: John Hubbard <jhubbard@...dia.com>
> >>> Cc: Dan Williams <dan.j.williams@...el.com>
> >>> ---
> >>>  include/linux/hmm.h | 50 ++++++++++++++++++++++++++++++++++++++++++---
> >>>  1 file changed, 47 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> >>> index f3b919b04eda..5f9deaeb9d77 100644
> >>> --- a/include/linux/hmm.h
> >>> +++ b/include/linux/hmm.h
> >>> @@ -438,6 +438,50 @@ struct hmm_mirror {
> >>>  int hmm_mirror_register(struct hmm_mirror *mirror, struct mm_struct *mm);
> >>>  void hmm_mirror_unregister(struct hmm_mirror *mirror);
> >>>  
> >>> +/*
> >>> + * hmm_mirror_mm_down_read() - lock the mmap_sem in read mode
> >>> + * @mirror: the HMM mm mirror for which we want to lock the mmap_sem
> >>> + * Returns: -EINVAL if the mm is dead, 0 otherwise (lock taken).
> >>> + *
> >>> + * The device driver context which holds reference to mirror and thus to core
> >>> + * hmm struct might outlive the mm against which it was created. To avoid every
> >>> + * driver to check for that case provide an helper that check if mm is still
> >>> + * alive and take the mmap_sem in read mode if so. If the mm have been destroy
> >>> + * (mmu_notifier release call back did happen) then we return -EINVAL so that
> >>> + * calling code knows that it is trying to do something against a mm that is
> >>> + * no longer valid.
> >>> + */
> >>> +static inline int hmm_mirror_mm_down_read(struct hmm_mirror *mirror)
> >>
> >> Hi Jerome,
> >>
> >> Let's please not do this. There are at least two problems here:
> >>
> >> 1. The hmm_mirror_mm_down_read() wrapper around down_read() requires a 
> >> return value. This is counter to how locking is normally done: callers do
> >> not normally have to check the return value of most locks (other than
> >> trylocks). And sure enough, your own code below doesn't check the return value.
> >> That is a pretty good illustration of why not to do this.
> > 
> > Please read the function description this is not about checking lock
> > return value it is about checking wether we are racing with process
> > destruction and avoid trying to take lock in such cases so that driver
> > do abort as quickly as possible when a process is being kill.
> > 
> >>
> >> 2. This is a weird place to randomly check for semi-unrelated state, such 
> >> as "is HMM still alive". By that I mean, if you have to detect a problem
> >> at down_read() time, then the problem could have existed both before and
> >> after the call to this wrapper. So it is providing a false sense of security,
> >> and it is therefore actually undesirable to add the code.
> > 
> > It is not, this function is use in device page fault handler which will
> > happens asynchronously from CPU event or process lifetime when a process
> > is killed or is dying we do want to avoid useless page fault work and
> > we do want to avoid blocking the page fault queue of the device. This
> > function reports to the caller that the process is dying and that it
> > should just abort the page fault and do whatever other device specific
> > thing that needs to happen.
> > 
> 
> But it's inherently racy, to check for a condition outside of any lock, so again,
> it's a false sense of security.

Yes and race are fine here, this is to avoid useless work if we are
unlucky and we race and fail to see the destruction that is just
happening then it is fine we are just going to do useless work. So
we do not care about race here we just want to bailout early if we
can witness the process dying.

> 
> >>
> >> If you insist on having this wrapper, I think it should have approximately 
> >> this form:
> >>
> >> void hmm_mirror_mm_down_read(...)
> >> {
> >> 	WARN_ON(...)
> >> 	down_read(...)
> >> } 
> > 
> > I do insist as it is useful and use by both RDMA and nouveau and the
> > above would kill the intent. The intent is do not try to take the lock
> > if the process is dying.
> 
> Could you provide me a link to those examples so I can take a peek? I
> am still convinced that this whole thing is a race condition at best.

The race is fine and ok see:

https://cgit.freedesktop.org/~glisse/linux/commit/?h=hmm-odp-v2&id=eebd4f3095290a16ebc03182e2d3ab5dfa7b05ec

which has been posted and i think i provided a link in the cover
letter to that post. The same patch exist for nouveau i need to
cleanup that tree and push it.

> > 
> > 
> >>
> >>> +{
> >>> +	struct mm_struct *mm;
> >>> +
> >>> +	/* Sanity check ... */
> >>> +	if (!mirror || !mirror->hmm)
> >>> +		return -EINVAL;
> >>> +	/*
> >>> +	 * Before trying to take the mmap_sem make sure the mm is still
> >>> +	 * alive as device driver context might outlive the mm lifetime.
> >>
> >> Let's find another way, and a better place, to solve this problem.
> >> Ref counting?
> > 
> > This has nothing to do with refcount or use after free or anthing
> > like that. It is just about checking wether we are about to do
> > something pointless. If the process is dying then it is pointless
> > to try to take the lock and it is pointless for the device driver
> > to trigger handle_mm_fault().
> 
> Well, what happens if you let such pointless code run anyway? 
> Does everything still work? If yes, then we don't need this change.
> If no, then we need a race-free version of this change.

Yes everything work, nothing bad can happen from a race, it will just
do useless work which never hurt anyone.

Cheers,
Jérôme

Powered by blists - more mailing lists