Re: [PATCH v2 10/11] mm/hmm: add helpers for driver to safely take the mmap_sem v2

[John Hubbard]

On 3/25/19 7:40 AM, jglisse@xxxxxxxxxx wrote:
> From: JÃrÃme Glisse <jglisse@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> Reviewed-by: Ralph Campbell <rcampbell@xxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: John Hubbard <jhubbard@xxxxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> ---
>  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.

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.

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(...)
} 

> +{
> +	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?

> +	 *
> +	 * 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;
> +
> +	down_read(&mm->mmap_sem);
> +	return 0;
> +}
> +
> +/*
> + * hmm_mirror_mm_up_read() - unlock the mmap_sem from read mode
> + * @mirror: the HMM mm mirror for which we want to lock the mmap_sem
> + */
> +static inline void hmm_mirror_mm_up_read(struct hmm_mirror *mirror)
> +{
> +	up_read(&mirror->hmm->mm->mmap_sem);
> +}
> +
>  
>  /*
>   * To snapshot the CPU page table you first have to call hmm_range_register()
> @@ -463,7 +507,7 @@ void hmm_mirror_unregister(struct hmm_mirror *mirror);
>   *          if (ret)
>   *              return ret;
>   *
> - *          down_read(mm->mmap_sem);
> + *          hmm_mirror_mm_down_read(mirror);

See? The normal down_read() code never needs to check a return value, so when
someone does a "simple" upgrade, it introduces a fatal bug here: if the wrapper
returns early, then the caller proceeds without having acquired the mmap_sem.

>   *      again:
>   *
>   *          if (!hmm_range_wait_until_valid(&range, TIMEOUT)) {
> @@ -476,13 +520,13 @@ void hmm_mirror_unregister(struct hmm_mirror *mirror);
>   *
>   *          ret = hmm_range_snapshot(&range); or hmm_range_fault(&range);
>   *          if (ret == -EAGAIN) {
> - *              down_read(mm->mmap_sem);
> + *              hmm_mirror_mm_down_read(mirror);

Same problem here.


thanks,
-- 
John Hubbard
NVIDIA