From: John Hubbard <jhubbard@nvidia.com>
To: Jerome Glisse <jglisse@redhat.com>
Cc: <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
Ralph Campbell <rcampbell@nvidia.com>
Subject: Re: [PATCH 10/10] mm/hmm: add helpers for driver to safely take the mmap_sem
Date: Wed, 20 Feb 2019 14:40:20 -0800 [thread overview]
Message-ID: <41888fd2-6154-4f85-7949-7a59c434d047@nvidia.com> (raw)
In-Reply-To: <20190220221933.GB29398@redhat.com>
On 2/20/19 2:19 PM, Jerome Glisse wrote:
> On Wed, Feb 20, 2019 at 01:59:13PM -0800, John Hubbard wrote:
>> On 1/29/19 8:54 AM, jglisse@redhat.com wrote:
>>> From: Jérôme 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.
>>>
>>> Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Cc: Ralph Campbell <rcampbell@nvidia.com>
>>> Cc: John Hubbard <jhubbard@nvidia.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 b3850297352f..4a1454e3efba 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.
>>> + */
>>
>> Hi Jerome,
>>
>> Are you thinking that, throughout the HMM API, there is a problem that
>> the mm may have gone away, and so driver code needs to be littered with
>> checks to ensure that mm is non-NULL? If so, why doesn't HMM take a
>> reference on mm->count?
>>
>> This solution here cannot work. I think you'd need refcounting in order
>> to avoid this kind of problem. Just doing a check will always be open to
>> races (see below).
>>
>>
>>> +static inline int hmm_mirror_mm_down_read(struct hmm_mirror *mirror)
>>> +{
>>> + 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.
>>> + *
>>> + * FIXME: should we also check for mm that outlive its owning
>>> + * task ?
>>> + */
>>> + mm = READ_ONCE(mirror->hmm->mm);
>>> + if (mirror->hmm->dead || !mm)
>>> + return -EINVAL;
>>> +
>>
>> Nothing really prevents mirror->hmm->mm from changing to NULL right here.
>
> This is really just to catch driver mistake, if driver does not call
> hmm_mirror_unregister() then the !mm will never be true ie the
> mirror->hmm->mm can not go NULL until the last reference to hmm_mirror
> is gone.
In that case, then this again seems unnecessary, and in fact undesirable.
If the driver code has a bug, then let's let the backtrace from a NULL
dereference just happen, loud and clear.
This patch, at best, hides bugs. And it adds code that should simply be
unnecessary, so I don't like it. :) Let's make it go away.
>
>>
>>> + down_read(&mm->mmap_sem);
>>> + return 0;
>>> +}
>>> +
>>
>> ...maybe better to just drop this patch from the series, until we see a
>> pattern of uses in the calling code.
>
> It use by nouveau now.
Maybe you'd have to remove that use case in a couple steps, depending on the
order that patches are going in.
thanks,
--
John Hubbard
NVIDIA
next prev parent reply other threads:[~2019-02-20 22:40 UTC|newest]
Thread overview: 98+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-29 16:54 [PATCH 00/10] HMM updates for 5.1 jglisse
2019-01-29 16:54 ` [PATCH 01/10] mm/hmm: use reference counting for HMM struct jglisse
2019-02-20 23:47 ` John Hubbard
2019-02-20 23:59 ` Jerome Glisse
2019-02-21 0:06 ` John Hubbard
2019-02-21 0:15 ` Jerome Glisse
2019-02-21 0:32 ` John Hubbard
2019-02-21 0:37 ` Jerome Glisse
2019-02-21 0:42 ` John Hubbard
2019-01-29 16:54 ` [PATCH 02/10] mm/hmm: do not erase snapshot when a range is invalidated jglisse
2019-02-20 23:58 ` John Hubbard
2019-01-29 16:54 ` [PATCH 03/10] mm/hmm: improve and rename hmm_vma_get_pfns() to hmm_range_snapshot() jglisse
2019-02-21 0:25 ` John Hubbard
2019-02-21 0:28 ` Jerome Glisse
2019-01-29 16:54 ` [PATCH 04/10] mm/hmm: improve and rename hmm_vma_fault() to hmm_range_fault() jglisse
2019-01-29 16:54 ` [PATCH 05/10] mm/hmm: improve driver API to work and wait over a range jglisse
2019-01-29 16:54 ` [PATCH 06/10] mm/hmm: add default fault flags to avoid the need to pre-fill pfns arrays jglisse
2019-01-29 16:54 ` [PATCH 07/10] mm/hmm: add an helper function that fault pages and map them to a device jglisse
2019-03-18 20:21 ` Dan Williams
2019-03-18 20:41 ` Jerome Glisse
2019-03-18 21:30 ` Dan Williams
2019-03-18 22:15 ` Jerome Glisse
2019-03-19 3:29 ` Dan Williams
2019-03-19 13:30 ` Jerome Glisse
2019-03-19 8:44 ` Ira Weiny
2019-03-19 17:10 ` Jerome Glisse
2019-03-19 14:10 ` Ira Weiny
2019-01-29 16:54 ` [PATCH 08/10] mm/hmm: support hugetlbfs (snap shoting, faulting and DMA mapping) jglisse
2019-01-29 16:54 ` [PATCH 09/10] mm/hmm: allow to mirror vma of a file on a DAX backed filesystem jglisse
2019-01-29 18:41 ` Dan Williams
2019-01-29 19:31 ` Jerome Glisse
2019-01-29 20:51 ` Dan Williams
2019-01-29 21:21 ` Jerome Glisse
2019-01-30 2:32 ` Dan Williams
2019-01-30 3:03 ` Jerome Glisse
2019-01-30 17:25 ` Dan Williams
2019-01-30 18:36 ` Jerome Glisse
2019-01-31 3:28 ` Dan Williams
2019-01-31 4:16 ` Jerome Glisse
2019-01-31 5:44 ` Dan Williams
2019-03-05 22:16 ` Andrew Morton
2019-03-06 4:20 ` Dan Williams
2019-03-06 15:51 ` Jerome Glisse
2019-03-06 15:57 ` Dan Williams
2019-03-06 16:03 ` Jerome Glisse
2019-03-06 16:06 ` Dan Williams
2019-03-07 17:46 ` Andrew Morton
2019-03-07 18:56 ` Jerome Glisse
2019-03-12 3:13 ` Dan Williams
2019-03-12 15:25 ` Jerome Glisse
2019-03-12 16:06 ` Dan Williams
2019-03-12 19:06 ` Jerome Glisse
2019-03-12 19:30 ` Dan Williams
2019-03-12 20:34 ` Dave Chinner
2019-03-13 1:06 ` Dan Williams
2019-03-12 21:52 ` Andrew Morton
2019-03-13 0:10 ` Jerome Glisse
2019-03-13 0:46 ` Dan Williams
2019-03-13 1:00 ` Jerome Glisse
2019-03-13 16:06 ` Andrew Morton
2019-03-13 18:39 ` Jerome Glisse
2019-03-06 15:49 ` Jerome Glisse
2019-03-06 22:18 ` Andrew Morton
2019-03-07 0:36 ` Jerome Glisse
2019-01-29 16:54 ` [PATCH 10/10] mm/hmm: add helpers for driver to safely take the mmap_sem jglisse
2019-02-20 21:59 ` John Hubbard
2019-02-20 22:19 ` Jerome Glisse
2019-02-20 22:40 ` John Hubbard [this message]
2019-02-20 23:09 ` Jerome Glisse
2019-02-20 23:17 ` [PATCH 00/10] HMM updates for 5.1 John Hubbard
2019-02-20 23:36 ` Jerome Glisse
2019-02-22 23:31 ` Ralph Campbell
2019-03-13 1:27 ` Jerome Glisse
2019-03-13 16:10 ` Andrew Morton
2019-03-13 18:01 ` Jason Gunthorpe
2019-03-13 18:33 ` Jerome Glisse
2019-03-18 17:00 ` Kuehling, Felix
2019-03-18 17:04 ` Jerome Glisse
2019-03-18 18:30 ` Dan Williams
2019-03-18 18:54 ` Jerome Glisse
2019-03-18 19:18 ` Dan Williams
2019-03-18 19:28 ` Jerome Glisse
2019-03-18 19:36 ` Dan Williams
2019-03-19 16:40 ` Andrew Morton
2019-03-19 16:58 ` Jerome Glisse
2019-03-19 17:12 ` Andrew Morton
2019-03-19 17:18 ` Jerome Glisse
2019-03-19 17:33 ` Dan Williams
2019-03-19 17:45 ` Jerome Glisse
2019-03-19 18:42 ` Dan Williams
2019-03-19 19:05 ` Jerome Glisse
2019-03-19 19:13 ` Dan Williams
2019-03-19 14:18 ` Ira Weiny
2019-03-19 22:24 ` Jerome Glisse
2019-03-19 19:18 ` Jerome Glisse
2019-03-19 20:25 ` Jerome Glisse
2019-03-19 21:51 ` Stephen Rothwell
2019-03-19 18:51 ` Deucher, Alexander
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=41888fd2-6154-4f85-7949-7a59c434d047@nvidia.com \
--to=jhubbard@nvidia.com \
--cc=akpm@linux-foundation.org \
--cc=jglisse@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=rcampbell@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).