Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2 10/11] mm/hmm: add helpers for driver to safely take
 the mmap_sem v2
To:     Jerome Glisse <jglisse@redhat.com>
CC:     <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Dan Williams <dan.j.williams@intel.com>
References: <20190325144011.10560-1-jglisse@redhat.com>
 <20190325144011.10560-11-jglisse@redhat.com>
 <9df742eb-61ca-3629-a5f4-8ad1244ff840@nvidia.com>
 <20190328213047.GB13560@redhat.com>
From:   John Hubbard <jhubbard@nvidia.com>
Message-ID: <a16efd42-3e2b-1b72-c205-0c2659de2750@nvidia.com>
Date:   Thu, 28 Mar 2019 14:41:02 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.5.3
MIME-Version: 1.0
In-Reply-To: <20190328213047.GB13560@redhat.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US-large
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@redhat.com wrote:
>>> From: J=C3=A9r=C3=B4me Glisse <jglisse@redhat.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=C3=A9r=C3=B4me Glisse <jglisse@redhat.com>
>>> Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Cc: John Hubbard <jhubbard@nvidia.com>
>>> Cc: Dan Williams <dan.j.williams@intel.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 *m=
m);
>>>  void hmm_mirror_unregister(struct hmm_mirror *mirror);
>>> =20
>>> +/*
>>> + * 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 av=
oid 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 t=
hat 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=20
>> return value. This is counter to how locking is normally done: callers d=
o
>> 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.
>=20
> 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.
>=20
>>
>> 2. This is a weird place to randomly check for semi-unrelated state, suc=
h=20
>> 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 secu=
rity,
>> and it is therefore actually undesirable to add the code.
>=20
> 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.
>=20

But it's inherently racy, to check for a condition outside of any lock, so =
again,
it's a false sense of security.

>>
>> If you insist on having this wrapper, I think it should have approximate=
ly=20
>> this form:
>>
>> void hmm_mirror_mm_down_read(...)
>> {
>> 	WARN_ON(...)
>> 	down_read(...)
>> }=20
>=20
> 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.

>=20
>=20
>>
>>> +{
>>> +	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?
>=20
> 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?=20
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.

thanks,
--=20
John Hubbard
NVIDIA