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 X-Spam-Level: X-Spam-Status: No, score=-13.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0D44DC433FE for ; Fri, 4 Dec 2020 15:25:48 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 8BDD422B42 for ; Fri, 4 Dec 2020 15:25:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8BDD422B42 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=infradead.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 197066B0071; Fri, 4 Dec 2020 10:25:47 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 16E686B0070; Fri, 4 Dec 2020 10:25:47 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 085026B0071; Fri, 4 Dec 2020 10:25:47 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0186.hostedemail.com [216.40.44.186]) by kanga.kvack.org (Postfix) with ESMTP id E667E6B006E for ; Fri, 4 Dec 2020 10:25:46 -0500 (EST) Received: from smtpin02.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 9A4DF3638 for ; Fri, 4 Dec 2020 15:25:46 +0000 (UTC) X-FDA: 77555974692.02.road96_320e7a5273c5 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin02.hostedemail.com (Postfix) with ESMTP id 58D7310097AA1 for ; Fri, 4 Dec 2020 15:25:46 +0000 (UTC) X-HE-Tag: road96_320e7a5273c5 X-Filterd-Recvd-Size: 4539 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf35.hostedemail.com (Postfix) with ESMTP for ; Fri, 4 Dec 2020 15:25:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=pHXKG6Haj3UJYJ0/Ctjg52HvNhgZPX5snB6HaKckAMw=; b=hPy86toiiQGS2Z1v7egHTV//zO N30rsVcZ2fSvK/3BRUCqDf4XYJlBb9PpoEXS6jXqU0adjw1IcgTYdX1IjbfcHItfsmXosbMpK1wf6 o0N98TWl9C4c0ep+XzdGz16sm0Y741/479IMkluzixx+AnzI1rx5WnXgqvo31kR9InV6bctjJ3N+e MQa/7VcBwVmVSy/sojbK3TGooT+xfmUZPg3qNBAckZpdx6s+UboFGZmunu/l/CgdUad/1I0CDG60k qGsBCSs3+yYyoIQlEXpSWEGIT3xnFohUSySC/HkaClk4SiNN17aEXiP7+sXPLMFn4UuK6fiaSatHv UYesnYrA==; Received: from willy by casper.infradead.org with local (Exim 4.92.3 #3 (Red Hat Linux)) id 1klCxj-0004a8-KI; Fri, 04 Dec 2020 15:25:35 +0000 Date: Fri, 4 Dec 2020 15:25:35 +0000 From: Matthew Wilcox To: Jason Gunthorpe Cc: David Hildenbrand , Liu Zixian , akpm@linux-foundation.org, linmiaohe@huawei.com, louhongxiang@huawei.com, linux-mm@kvack.org, hushiyuan@huawei.com, stable@vger.kernel.org, davem@davemloft.net Subject: Re: [PATCH v2] fix mmap return value when vma is merged after call_mmap() Message-ID: <20201204152535.GP11935@casper.infradead.org> References: <20201203085350.22624-1-liuzixian4@huawei.com> <20201204151028.GZ5487@ziepe.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201204151028.GZ5487@ziepe.ca> 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 Fri, Dec 04, 2020 at 11:10:28AM -0400, Jason Gunthorpe wrote: > On Fri, Dec 04, 2020 at 03:11:54PM +0100, David Hildenbrand wrote: > > On 03.12.20 09:53, Liu Zixian wrote: > > > On success, mmap should return the begin address of newly mapped area, > > > but patch "mm: mmap: merge vma after call_mmap() if possible" > > > set vm_start of newly merged vma to return value addr. > > > Users of mmap will get wrong address if vma is merged after call_mmap(). > > > We fix this by moving the assignment to addr before merging vma. > > > > > > Fixes: d70cec898324 ("mm: mmap: merge vma after call_mmap() if possible") > > > Signed-off-by: Liu Zixian > > > v2: > > > We want to do "addr = vma->vm_start;" unconditionally, > > > so move assignment to addr before if(unlikely) block. > > > mm/mmap.c | 26 ++++++++++++-------------- > > > 1 file changed, 12 insertions(+), 14 deletions(-) > > > > > > diff --git a/mm/mmap.c b/mm/mmap.c > > > index d91ecb00d38c..5c8b4485860d 100644 > > > +++ b/mm/mmap.c > > > @@ -1808,6 +1808,17 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > > > if (error) > > > goto unmap_and_free_vma; > > > > > > + /* Can addr have changed?? > > > + * > > > + * Answer: Yes, several device drivers can do it in their > > > + * f_op->mmap method. -DaveM > > > + * Bug: If addr is changed, prev, rb_link, rb_parent should > > > + * be updated for vma_link() > > > + */ > > > > > > Why do we tolerate device drivers doing such stuff at all? > > WARN_ON_ONCE() is just as good as BUG_ON() in many environments. If we > > support it, then no warning. If we don't support it, then why tolerate it? > > The commit that introduced this seemed pretty clear it is to catch > possibly wrong drivers. I suppose the idea was to give a migration > time where things would "work" and drivers could be fixed. Since it > has now been 8 years it should be either dropped or turned into: > > /* Drivers are not permitted to change vm_start */ > if (WARN_ON(addr != vma->vm_start)) { > err = EINVAL > goto unmap_and_free_vma > } This commit makes no sense. I know it's eight years old, so maybe the device driver which did this has long been removed from the tree, but davem's comment was (iirc) related to a device driver for a graphics card that would 256MB-align the user address. Another possibility is that userspace always asks for a 256MB-aligned address these days. I don't understand why prev/rb_link/rb_parent would need to be changed in this case. It's going to be inserted at the exact same location in the rbtree, just at a slightly shifted address.