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;
From:   Liam Howlett <liam.howlett@oracle.com>
To:     Carlos Llamas <cmllamas@google.com>
CC:     "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        syzbot <syzbot+a7b60a176ec13cafb793@syzkaller.appspotmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Ondrej Mosnacek <omosnace@redhat.com>,
        "syzkaller-bugs@googlegroups.com" <syzkaller-bugs@googlegroups.com>,
        Minchan Kim <minchan@kernel.org>,
        Christian Brauner <brauner@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Hridya Valsaraju <hridya@google.com>,
        Joel Fernandes <joel@joelfernandes.org>,
        Martijn Coenen <maco@android.com>,
        Suren Baghdasaryan <surenb@google.com>,
        Todd Kjos <tkjos@android.com>,
        Matthew Wilcox <willy@infradead.org>,
        =?iso-8859-1?Q?Arve_Hj=F8nnev=E5g?= <arve@android.com>
Subject: Re: [PATCH] binder_alloc: Add missing mmap_lock calls when using the
 VMA
Thread-Topic: [PATCH] binder_alloc: Add missing mmap_lock calls when using the
 VMA
Thread-Index: AQHYrAn9C2tDT6s3j06giZb3JwoEAa2m6VSAgAADp4A=
Date:   Tue, 9 Aug 2022 19:02:17 +0000
Message-ID: <20220809190211.ew4taam2lcomramw@revolver>
References: <20220809160618.1052539-1-Liam.Howlett@oracle.com>
 <YvKsI5pMbgQ5Irup@google.com>
In-Reply-To: <YvKsI5pMbgQ5Irup@google.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?iso-8859-1?Q?DAgAtdhUujppgHL8J4uXN0PiQkdMQAEtEFWU2YsxAB8PcduKnt/g5YJE+I?=
 =?iso-8859-1?Q?fTJs6GjSrza9zM/Aj0FIKz/uivjhmbqGvbmcp/l5lFnPZZyoT1/uM2EW8z?=
 =?iso-8859-1?Q?lezj0qRvsA6YG9AeonNN1JGdz51IwUsJkjkYay/e7R3LSb6lo4SiRyyWvs?=
 =?iso-8859-1?Q?/nBO6Fkw1kB4NxwZzTEWQseb+c0VlWBtdQrhwTbGd+I+X9oWkedbrXdCIq?=
 =?iso-8859-1?Q?sJolvFT5uvtsoplz9giVBgVWVfHn8MLJhp6GUjUAvo5vhjUtib6CtFLvpA?=
 =?iso-8859-1?Q?K8wFH2tQBPpobdqQIZ7uqBWBRB8Z1Pse/9kWZqi5UIN3yO1E5nib+N+0du?=
 =?iso-8859-1?Q?mUPYp3ri3EIP2gGR7TOJ6gkMyfat35BZ/B6jUo8C5kU1ACST5nI2w+5T5M?=
 =?iso-8859-1?Q?Va/nAW6e5Y7+nojmmNrjKRokuEML3lhrTi6KyuSeghPrz/MxNjDpcS5U/w?=
 =?iso-8859-1?Q?sgfT4PxiN2sGPvpFHiQ/z4Jer/fCKEKGXBK8fzBnoF/JxvNterUsynCrXO?=
 =?iso-8859-1?Q?cwZ2wPKb1Ap3cmpOaEOfHweRRVBLJZubX87jm8U1O+h6GHb7R0gvV/m+F1?=
 =?iso-8859-1?Q?sjB8D7kQkB6Eut9ZfewX70bTFyAI6QZLzg+LnAbvkpgDlunO5mNU2Bai3v?=
 =?iso-8859-1?Q?Qs5aZTCIPPoEr1bnWgviC4Pavz91hyW36eJWCmVQYLjJ3YqN9z0T7Yfias?=
 =?iso-8859-1?Q?8EuM4c07eGoAC7PjtL4c7OrZUMJmm2nRfLNDyPZk/rpMrURihQQ08q9jAf?=
 =?iso-8859-1?Q?VK+hVd/fs4SuXfQa6yRt7Bc8HScu8E1RZDR0Lst+CgypbNVGPmUfyn1mH6?=
 =?iso-8859-1?Q?PPf3J0SpPyy8pLciouE6JNgaQD74z5QD/VHDBeIJWO9mWw3RFA7DdCD0eZ?=
 =?iso-8859-1?Q?aWJ0uL+d6k4YB7URt67884JMzVKUGxZB5lU3M6k29egI38bRmofczJmTzl?=
 =?iso-8859-1?Q?oFBApQv0IwMbtZZ4rC4BF7b6aCFrvG7i8kQqp3wZWnKXp2jPapRURsQjPi?=
 =?iso-8859-1?Q?clAA2RvbC7XsV6JD/lkKMjtQnH2/HitBumaUbW1cWHiDFW3MIPNnPIIyP1?=
 =?iso-8859-1?Q?+hVmbpIBWuw+kVd2/woIOWu6wiNQ8Ifi3lLlnw7h1nKnOhD68/ulpoppsk?=
 =?iso-8859-1?Q?it2XtCj+aCYbbO3dBDSf8ank+m3I7r05JtEVlMOnbVwMq5rkTUP6DUcZ0r?=
 =?iso-8859-1?Q?jpnJekU8AtyvmxKQMUrQ5eH/7gfvU/KePJGAZiCqIbh4aCn6c8tcAHIYP5?=
 =?iso-8859-1?Q?HaAMdm0oEzjc/hqX+cVoDFDxocYHUD3HxQ1ibiG/DWl9aAdukO+wDRXaIO?=
 =?iso-8859-1?Q?cUhnEh/YJJNfcCpTU8pg4tpsT0IdLpz23/IOgn9YE7iiSgxJQePx3PMzCc?=
 =?iso-8859-1?Q?/hO3ZEGKeNQv2FoegJyqrbdVKyWAWnfBy1lD4glA46+m2wC+kSWX0dfKvK?=
 =?iso-8859-1?Q?9VcAf6P2DNOsTb7G7g/Q0Ad+o5v4AbphoLoNzJCEHhX/QrqAmQr+Yt/YoC?=
 =?iso-8859-1?Q?Zo7pqz5Q5BM1/w2e6LkuK4eEIBsWymWnA/Kx1bYVbclysnw6zwPNtcU8xc?=
 =?iso-8859-1?Q?Kqdmhe/I7+FrkMLBKQaq2j/ik4/XRorLEB2Xa05hUs9LU/58aTONdyeYqK?=
 =?iso-8859-1?Q?ABJfA8fHbMz1VMIzOxusvIuTDYj7xXBlDkcIYuF7R1x3XasQbLhvVTng?=
 =?iso-8859-1?Q?=3D=3D?=
Content-Type: text/plain; charset="iso-8859-1"
Content-ID: <896EC046421E064A8683C916DBB983B1@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SN6PR10MB3022.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f91d46b2-d7ef-44f0-d777-08da7a39ae0a
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Aug 2022 19:02:17.7704
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: T9NYXEzELkcWVsaJISjrzTanC+tk1DZK0Ej8DgeWIEdMmBQkqZi2P6adbXD4l2llHBK7JiSi5CuyyoXgK3L0NA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB3592
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-08-09_05,2022-08-09_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0
 malwarescore=0 mlxscore=0 adultscore=0 bulkscore=0 mlxlogscore=999
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2206140000 definitions=main-2208090072
X-Proofpoint-ORIG-GUID: MtRhjBJxkH4GGPrxnMuIOFO2ZXOAbkxP
X-Proofpoint-GUID: MtRhjBJxkH4GGPrxnMuIOFO2ZXOAbkxP
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

* Carlos Llamas <cmllamas@google.com> [220809 14:49]:
> On Tue, Aug 09, 2022 at 04:06:31PM +0000, Liam Howlett wrote:
> > Take the mmap_read_lock() when using the VMA in
> > binder_alloc_print_pages() and when checking for a VMA in
> > binder_alloc_new_buf_locked().
> >=20
> > It is worth noting binder_alloc_new_buf_locked() drops the VMA read loc=
k
> > after it verifies a VMA exists, but may be taken again deeper in the
> > call stack, if necessary.
> >=20
> > Reported-by: Ondrej Mosnacek <omosnace@redhat.com>
> > Reported-by: syzbot+a7b60a176ec13cafb793@syzkaller.appspotmail.com
> > Fixes: a43cfc87caaf (android: binder: stop saving a pointer to the VMA)
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> > ---
> >  drivers/android/binder_alloc.c | 29 +++++++++++++++++++----------
> >  1 file changed, 19 insertions(+), 10 deletions(-)
> >=20
> > diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_al=
loc.c
> > index f555eebceef6..c23cad7fe313 100644
> > --- a/drivers/android/binder_alloc.c
> > +++ b/drivers/android/binder_alloc.c
> > @@ -395,12 +395,15 @@ static struct binder_buffer *binder_alloc_new_buf=
_locked(
> >  	size_t size, data_offsets_size;
> >  	int ret;
> > =20
> > +	mmap_read_lock(alloc->vma_vm_mm);
> >  	if (!binder_alloc_get_vma(alloc)) {
> > +		mmap_read_unlock(alloc->vma_vm_mm);
> >  		binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
> >  				   "%d: binder_alloc_buf, no vma\n",
> >  				   alloc->pid);
> >  		return ERR_PTR(-ESRCH);
> >  	}
> > +	mmap_read_unlock(alloc->vma_vm_mm);
> > =20
> >  	data_offsets_size =3D ALIGN(data_size, sizeof(void *)) +
> >  		ALIGN(offsets_size, sizeof(void *));
> > @@ -922,17 +925,23 @@ void binder_alloc_print_pages(struct seq_file *m,
> >  	 * Make sure the binder_alloc is fully initialized, otherwise we migh=
t
> >  	 * read inconsistent state.
> >  	 */
> > -	if (binder_alloc_get_vma(alloc) !=3D NULL) {
> > -		for (i =3D 0; i < alloc->buffer_size / PAGE_SIZE; i++) {
> > -			page =3D &alloc->pages[i];
> > -			if (!page->page_ptr)
> > -				free++;
> > -			else if (list_empty(&page->lru))
> > -				active++;
> > -			else
> > -				lru++;
> > -		}
> > +
> > +	mmap_read_lock(alloc->vma_vm_mm);
> > +	if (binder_alloc_get_vma(alloc) =3D=3D NULL)
> > +		goto uninitialized;
> > +
> > +	for (i =3D 0; i < alloc->buffer_size / PAGE_SIZE; i++) {
>=20
> do we need to hold on to the lock while we loop through the pages here?

I think we do?  Holding this lock will ensure the pages don't go away, I
believe (looking at mm/rmap.c comments on locking at the top)?

In any case, this function is called from print_binder_proc_stats()
which looks to be a debugfs/debugging call so I thought safer would be
better than faster and with a potential race.

>=20
> > +		page =3D &alloc->pages[i];
> > +		if (!page->page_ptr)
> > +			free++;
> > +		else if (list_empty(&page->lru))
> > +			active++;
> > +		else
> > +			lru++;
> >  	}
> > +
> > +uninitialized:
> > +	mmap_read_unlock(alloc->vma_vm_mm);
> >  	mutex_unlock(&alloc->mutex);
> >  	seq_printf(m, "  pages: %d:%d:%d\n", active, lru, free);
> >  	seq_printf(m, "  pages high watermark: %zu\n", alloc->pages_high);
> > --=20
> > 2.35.1=