From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id D477AC433FE for ; Thu, 29 Sep 2022 14:39:50 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 175D28D0002; Thu, 29 Sep 2022 10:39:50 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1259E8D0001; Thu, 29 Sep 2022 10:39:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F080D8D0002; Thu, 29 Sep 2022 10:39:49 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id DE2778D0001 for ; Thu, 29 Sep 2022 10:39:49 -0400 (EDT) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id B5A4A4014F for ; Thu, 29 Sep 2022 14:39:49 +0000 (UTC) X-FDA: 79965382098.08.E8C2A41 Received: from mail-pg1-f170.google.com (mail-pg1-f170.google.com [209.85.215.170]) by imf27.hostedemail.com (Postfix) with ESMTP id 59F924001A for ; Thu, 29 Sep 2022 14:39:49 +0000 (UTC) Received: by mail-pg1-f170.google.com with SMTP id u69so1644810pgd.2 for ; Thu, 29 Sep 2022 07:39:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date; bh=5sHHbyvhxIEg2SKDqtnLGqU7In6Q6IL3pesUHjmyLA0=; b=rGRVHXyYrpvv36oZ3VLa0jBC1JDS2orf7NPuxvVw5YoLB9R3N2Lj0BaeGks1P2aV8T eNo3mQEK3GTDZun04pIeDf6hHd4yoHlHKbSGiEe4OUGiSIwQy8GNqoD8jcD0GVua7bu4 YanBy8UYYQe1zQSoyQTTrMOIXZbdPTyI6I2YaTaIKgYxxiCXWbkmch2lKxYHqH+sGY86 EvJLYzzJRyZ1F3OemelyAHO7VYniRV6ovUomXOy5aXa2DpbjbnUpJwjxUp5AmvG5zUDF B/BKs8NMVIj2L3S58D69MMr3rWaYx0l0gOwr9eFXW09pU1mGsW2lOmgJ6JzqScx0NV2Y 76eQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date; bh=5sHHbyvhxIEg2SKDqtnLGqU7In6Q6IL3pesUHjmyLA0=; b=1ZStGVlNMK5f3hPrUK7fH0iPj7p42q6kQ3Ywxl3sm0ZD8YrC62Vczn3z8Gl5jeHs/d oGjIUuRC9+2+xu7DD0wGoKDp9vHjUfCGp8RNCHgDJlO2ld3GkPWpRzLsSyfPznPdsCQm u+hi7ih3CpxyHgOLI+3ZBL2lYlYjdg910cH4GeMBaIEr7tgIG0XN5un0XsKgPmmBhnno 1H3gJu6k+6ZqSP4nFOmG6JF9eVSii1YTKLrg4HoCxAp7F9jwOTl8LjWd9qzBh257QnJ4 wHIl2eAnfiEBAnTlb7dikTwSxraajau/NEUgbAEfR3Dfn15LQ+DxIjuvaNndq39s1Z7y vhIw== X-Gm-Message-State: ACrzQf1NgmNODkwtaaEqWRml5+qJOwfRO22QgqXPnw6ZiRr8mKhqalDz 07HK0ZwkxYUOkpwVLla1ifCxKg== X-Google-Smtp-Source: AMsMyM7oLpt+EQbhrtYRPELgiQeECQgLV4y0rLW6ik66883MsXV11LwQVyB0pzBL7FbRQY3ecyMsJw== X-Received: by 2002:a63:2c90:0:b0:439:ee2c:ab2f with SMTP id s138-20020a632c90000000b00439ee2cab2fmr3324865pgs.2.1664462388088; Thu, 29 Sep 2022 07:39:48 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id v16-20020aa799d0000000b0053e38ac0ff4sm6327205pfi.115.2022.09.29.07.39.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 29 Sep 2022 07:39:47 -0700 (PDT) Date: Thu, 29 Sep 2022 14:39:43 +0000 From: Carlos Llamas To: Suren Baghdasaryan Cc: Liam Howlett , Andrew Morton , Michal Hocko , Guenter Roeck , Douglas Anderson , Christian Brauner , Greg Kroah-Hartman , "linux-mm@kvack.org" Subject: Re: BUG in binder_vma_close() at mmap_assert_locked() in stable v5.15 Message-ID: References: <20220913063625.3hgghufytudm6x4p@revolver> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1664462389; a=rsa-sha256; cv=none; b=BLZ6h5KCrBOq/61Mtjc1U0XYjj+SeShY0iICMnFyGCPPI23ZxESMGn7JfqAyrgAG1M6GOI 30BDB1Qjj1zhgHuSYCTDwRKXonkSQtzxt3K4JJVU2a/AVeemG5qojWVfpu6S6VRxA1K3kF YCFKq0AMBux4LSXrzbrpswZfZKwcmHs= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=rGRVHXyY; spf=pass (imf27.hostedemail.com: domain of cmllamas@google.com designates 209.85.215.170 as permitted sender) smtp.mailfrom=cmllamas@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1664462389; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=5sHHbyvhxIEg2SKDqtnLGqU7In6Q6IL3pesUHjmyLA0=; b=XjhP+Z3F4RHZ6cAyqlr6baVeiBgz1T9WKdMB6C7/K/MYWPrZnRhET7y0lok9Mo5F6n44Lq mYSBBmAwz34ppU7+CzZJDDgXHqLq+O3eyh5o9W/hp6kjnNSZtRl0WBeeWhfxsptbh3vXIj n8HB2teWgiIqeLPZCOVpg7vdEHrw5no= X-Stat-Signature: qa6ehj3fs3943ggqbi5q1koe6onu317m X-Rspamd-Queue-Id: 59F924001A X-Rspam-User: Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=rGRVHXyY; spf=pass (imf27.hostedemail.com: domain of cmllamas@google.com designates 209.85.215.170 as permitted sender) smtp.mailfrom=cmllamas@google.com; dmarc=pass (policy=reject) header.from=google.com X-Rspamd-Server: rspam01 X-HE-Tag: 1664462389-255956 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, Sep 28, 2022 at 04:21:27PM -0700, Suren Baghdasaryan wrote: > > However, I wonder why isn't ->mmap() being undone for this exit path in > > mmap_region()? If anything fails _after_ call_mmap() it seems we > > silently unmap and free the vma. What about allocations, atomic_incs, > > and anything done inside ->mmap()? > > I think if ->mmap() fails it is expected to undo its own changes > before returning the error. mmap_region() has no idea what kind of > changes ->mmap() has done before it failed, therefore it can't undo > them. At least that's how I understand it. I agree ->mmap() should undo its own changes if it fails. However, the scenario I'm referring to is an error _after_ ->mmap() call succeeds. In this case it can be the arch_validate_flags() check. On error, the vma is torn down and the drivers are never informed about it. > > Shouldn't a ->close() be needed here > > to undo these things as such: > > -- > > @@ -1872,6 +1889,9 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > > > > return addr; > > > > +undo_mmap: > > + if (vma->vm_ops && vma->vm_ops->close) > > + vma->vm_ops->close(vma); > > unmap_and_free_vma: > > fput(vma->vm_file); > > vma->vm_file = NULL; > > -- > > > > I don't see mmap_region() calling vma->vm_ops->open() anywhere. So why > would it have to call vma->vm_ops->close()? My understanding is ->mmap() is called on the very first mapping and vm_ops->close() is subsequently called whenever a mapping is removed. So IIUC a vm_ops->open() is not required in this exchange. There are multiple places where I can see rely on this behavior. Just to provide an example, look at coda_file_mmap() which allocates cvm_ops and increments references on ->mmap(). It then expects coda_vm_close() to be called when the vma is removed to decrement these references and lastly free cvm_ops. However, if mmap_region() fails after ->mmap() call then vm_ops->close() is never invoked and the reference count in coda is now off making cvm_ops leaked memory. Other type of issues happen depending on the ->mmap() implementation. In our case it was a BUG_ON() in binder. I don't know if adding vm_ops->close() to the exit error path is an appropriate solution. Perhaps ->mmap() should be a point of no return instead and vm_flags should be validated earlier? I would be concerned about drivers modifying the vm_flags during ->mmap() though. I'll send out a patch to get more feedback on this vm_ops->close(). -- Carlos Llamas