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;
MIME-Version: 1.0
References: <1555106365-26137-1-git-send-email-tyhicks@canonical.com>
In-Reply-To: <1555106365-26137-1-git-send-email-tyhicks@canonical.com>
From:   Todd Kjos <tkjos@google.com>
Date:   Mon, 22 Apr 2019 07:49:28 -0700
Message-ID: <CAHRSSEymaMf1qVJUnTrFT9iujbc44qN8eaQS0TkO639SgNJc8g@mail.gmail.com>
Subject: Re: [PATCH] binder: take read mode of mmap_sem in binder_alloc_free_page()
To:     Tyler Hicks <tyhicks@canonical.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= <arve@android.com>,
        Todd Kjos <tkjos@android.com>,
        Martijn Coenen <maco@android.com>,
        Joel Fernandes <joel@joelfernandes.org>,
        Christian Brauner <christian@brauner.io>,
        Minchan Kim <minchan@kernel.org>,
        "open list:ANDROID DRIVERS" <devel@driverdev.osuosl.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Apr 12, 2019 at 2:59 PM Tyler Hicks <tyhicks@canonical.com> wrote:
>
> Restore the behavior of locking mmap_sem for reading in
> binder_alloc_free_page(), as was first done in commit 3013bf62b67a
> ("binder: reduce mmap_sem write-side lock"). That change was
> inadvertently reverted by commit 5cec2d2e5839 ("binder: fix race between
> munmap() and direct reclaim").
>
> In addition, change the name of the label for the error path to
> accurately reflect that we're taking the lock for reading.
>
> Backporting note: This fix is only needed when *both* of the commits
> mentioned above are applied. That's an unlikely situation since they
> both landed during the development of v5.1 but only one of them is
> targeted for stable.
>
> Fixes: 5cec2d2e5839 ("binder: fix race between munmap() and direct reclaim")
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>


Thanks for fixing this up.

Acked-by: Todd Kjos <tkjos@android.com>

> ---
>
> Hello - I was backporting 5cec2d2e5839 and didn't think it looked quite
> right. The patch description doesn't specifically mention a need to acquire
> mmap_sem for writing and the two commits (5cec2d2e5839 and 3013bf62b67a) landed
> relatively close in time to each other so I have a feeling that a mistake could
> have happened while resolving conflicts.
>
> I also noticed that commit 3013bf62b67a was slightly incomplete because
> it didn't rename the err_down_write_mmap_sem_failed label to
> err_down_read_mmap_sem_failed.
>
> I've tested this change by enabling CONFIG_ANDROID_BINDER_IPC_SELFTEST
> and verified that the binderfs_test selftest passes.
>
> Tyler
>
>  drivers/android/binder_alloc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> index 195f120c4e8c..bb929eb87116 100644
> --- a/drivers/android/binder_alloc.c
> +++ b/drivers/android/binder_alloc.c
> @@ -931,8 +931,8 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
>         mm = alloc->vma_vm_mm;
>         if (!mmget_not_zero(mm))
>                 goto err_mmget;
> -       if (!down_write_trylock(&mm->mmap_sem))
> -               goto err_down_write_mmap_sem_failed;
> +       if (!down_read_trylock(&mm->mmap_sem))
> +               goto err_down_read_mmap_sem_failed;
>         vma = binder_alloc_get_vma(alloc);
>
>         list_lru_isolate(lru, item);
> @@ -945,7 +945,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
>
>                 trace_binder_unmap_user_end(alloc, index);
>         }
> -       up_write(&mm->mmap_sem);
> +       up_read(&mm->mmap_sem);
>         mmput(mm);
>
>         trace_binder_unmap_kernel_start(alloc, index);
> @@ -959,7 +959,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
>         mutex_unlock(&alloc->mutex);
>         return LRU_REMOVED_RETRY;
>
> -err_down_write_mmap_sem_failed:
> +err_down_read_mmap_sem_failed:
>         mmput_async(mm);
>  err_mmget:
>  err_page_already_freed:
> --
> 2.7.4
>