Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 30 Aug 2022 21:08:02 +0000
From:   Carlos Llamas <cmllamas@google.com>
To:     Liam Howlett <liam.howlett@oracle.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Arve =?iso-8859-1?B?SGr4bm5lduVn?= <arve@android.com>,
        Todd Kjos <tkjos@android.com>,
        Martijn Coenen <maco@android.com>,
        Joel Fernandes <joel@joelfernandes.org>,
        Christian Brauner <brauner@kernel.org>,
        Suren Baghdasaryan <surenb@google.com>,
        "kernel-team@android.com" <kernel-team@android.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 4/7] binder: remove binder_alloc_set_vma()
Message-ID: <Yw58Mm5QrPIFtYak@google.com>
References: <20220829201254.1814484-1-cmllamas@google.com>
 <20220829201254.1814484-5-cmllamas@google.com>
 <20220830185751.gpudzi2pv3jpjoib@revolver>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220830185751.gpudzi2pv3jpjoib@revolver>
Precedence: bulk

On Tue, Aug 30, 2022 at 06:57:59PM +0000, Liam Howlett wrote:
> * Carlos Llamas <cmllamas@google.com> [220829 16:13]:
> > The mmap_locked asserts here are not needed since this is only called
> > back from the mmap stack in ->mmap() and ->close() which always acquire
> > the lock first. Remove these asserts along with binder_alloc_set_vma()
> > altogether since it's trivial enough to be consumed by callers.
> 
> I agree that it is not called anywhere else today.  I think it's still
> worth while since these asserts do nothing if you don't build with
> lockdep and/or debug_vm enabled.  I was hoping having these here would
> avoid future mistakes a lot like what we have in the mm code's
> find_vma() (mm/mmap.c ~L2297).
> 

Yes, the assert in find_vma() is perfectly fine, we need to check that
callers have taken the lock before looking up a vma. However, the
scenario here is different as these are callbacks for vm_ops->close()
and mmap() so we are guaranteed to have the lock at this point. We don't
really want to duplicate checks for each user of these callbacks such as
the binder driver here.