From patchwork Wed Jul 11 12:15:21 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Kirill A. Shutemov" X-Patchwork-Id: 10519599 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 39E96603D7 for ; Wed, 11 Jul 2018 12:21:40 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1F28428E3E for ; Wed, 11 Jul 2018 12:21:40 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 11A0A28E4A; Wed, 11 Jul 2018 12:21:40 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, MAILING_LIST_MULTI, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 700EC28E3E for ; Wed, 11 Jul 2018 12:21:39 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9D0016B0003; Wed, 11 Jul 2018 08:21:38 -0400 (EDT) Delivered-To: linux-mm-outgoing@kvack.org Received: by kanga.kvack.org (Postfix, from userid 40) id 9801B6B0292; Wed, 11 Jul 2018 08:21:38 -0400 (EDT) X-Original-To: int-list-linux-mm@kvack.org X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 86F876B0295; Wed, 11 Jul 2018 08:21:38 -0400 (EDT) X-Original-To: linux-mm@kvack.org X-Delivered-To: linux-mm@kvack.org Received: from mail-pl0-f71.google.com (mail-pl0-f71.google.com [209.85.160.71]) by kanga.kvack.org (Postfix) with ESMTP id 484286B0003 for ; Wed, 11 Jul 2018 08:21:38 -0400 (EDT) Received: by mail-pl0-f71.google.com with SMTP id z21-v6so7257035plo.13 for ; Wed, 11 Jul 2018 05:21:38 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:dkim-signature:date:from:to:cc:subject :message-id:references:mime-version:content-disposition:in-reply-to :user-agent; bh=jbELOvLxzwiPzr0ySZKtIaNB4Xh08CmtwYy9J53koKc=; b=kkPfFl3bhUk+txtUMc3Z0nyjhaG6DAAT+KC2hDmSMD11G5VKuGqysSYgG2VTEgK+Su +S8qll2s2rXU5UolM2AQZ17ZYBI0FQN0O3hUGaYRRGvToSfX9iJkPVbobBVDicgdL50g 8ps9RmGbRXLrILIDqHuQ7m0VBvaIqWw8J3gl+Y+8yKUR9hJpiq3wmfx35wToz+UDJmCr 3rWq6TSMEkFjjDkcmEmAmEKJ41cftYn4roBKBgxvufAa8VmW7PFtKjc/uZJoyYGrocWu VsyGqHaIGbaVIcu72tvDZ8OpS6hFAaplbCIC2PtQ45VPhrLo8DRwrD3kbHDpbvhc3qha v2Pg== X-Gm-Message-State: APt69E15KkOL95rXkxDk6aZrYIUq9xJgfH5LYZLghozXSCgF/c8Qufiu dj7xsC/kAua4hwFLoqbsdfAJbKv8tDhA0TwkPDCwrR/rEm7oX2QcHSiIIQ95q72k4hcMtOUTKwo Kxwx9IQKBhp0KXCOekbVN7kBIHXCrUcl1W1jKBgXRBb19fy9wLIxsLN5y3uyzuIG4ml0MxYTgmV YUA5yBy1s7M67t0LAPcz2HRin84P8JjhK/xe5vA6wdskBifdZ8apZLUeiUtmXjbc879CuvmS2Au SBiyk0QTCSgrjlT5Iprk3yNmSL8ZGLtCb4apJejxkGeZ0WQu/DTnoKZT0OJDNjAabn1jJL5JtNx E4XnqHoWrbSRDgBULNvN66Y1T6fxMFZMF+ozuSJpIb637JSNbknfDlUsrHOJ0l2P+ziR+3GOSWQ / X-Received: by 2002:a63:4a07:: with SMTP id x7-v6mr26754244pga.34.1531311697975; Wed, 11 Jul 2018 05:21:37 -0700 (PDT) X-Received: by 2002:a63:4a07:: with SMTP id x7-v6mr26754195pga.34.1531311697162; Wed, 11 Jul 2018 05:21:37 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1531311697; cv=none; d=google.com; s=arc-20160816; b=V6l/1n+Uxt8fHJRDx0EOTJqosd8LhYOo0lLDHHv8OMXN7/YyfLYdOASJMntF9re8EN rZ0ZYkxz580TRRfK8L8L8uAYibIQczFGFPzbqbSapjtyzYQmYTy8H6GU4j8c9UljTyMr Wioclcp14uaebXo11pKOg9LN+rmHm7OmKZr6NYEeUloWpdjoERTC1dC3NhGu+yFtUg7O 1DcroVOlc5AG6ENtrFoV40EyyTzjLvJtauUh3A7bPqM1FKlzrwQMD/Xtr9H5pz3HsySY dteCXwPug9Pw204bkXfI84JaTkMetivEniZ6eni4jOngjAmra60gTMI7IIfVsmNsRMDh /56w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=user-agent:in-reply-to:content-disposition:mime-version:references :message-id:subject:cc:to:from:date:dkim-signature :arc-authentication-results; bh=jbELOvLxzwiPzr0ySZKtIaNB4Xh08CmtwYy9J53koKc=; b=NtLddjD3i/ssEDcp3JiI/Qnu6Pjbk4WD23qSwDgbdxokHF0Q3AXVEyqHrAn7PrSIK0 tPjFhGAXoMw3jdNvJ97XvcpA9XuJPm0mgNiiRWaFa25gf821unNm8VcPTyZQM/6MZkO8 j2ojGBGlQgQKc77mOuYJI6IxVKcIJpZulRbYjVeMF3FqvBOj1TcymghEyXXES0NICZ9K FMOrF0mVYB+W2CpTBXbZX6Fup+gcG/w0w0VSj3EeiTy3y9Z2eKIzzvi55cIcJKkm48Ec YCI5XhiH/zv7b5VDeYl+m/Mn86bVGOK/9mzDdr/H2MN+r1S3AAuqOa3CEnSkPXoO3UTj 1ToQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@shutemov-name.20150623.gappssmtp.com header.s=20150623 header.b=QYQjXOEK; spf=neutral (google.com: 209.85.220.65 is neither permitted nor denied by best guess record for domain of kirill@shutemov.name) smtp.mailfrom=kirill@shutemov.name Received: from mail-sor-f65.google.com (mail-sor-f65.google.com. [209.85.220.65]) by mx.google.com with SMTPS id z9-v6sor6245626pln.119.2018.07.11.05.21.37 for (Google Transport Security); Wed, 11 Jul 2018 05:21:37 -0700 (PDT) Received-SPF: neutral (google.com: 209.85.220.65 is neither permitted nor denied by best guess record for domain of kirill@shutemov.name) client-ip=209.85.220.65; Authentication-Results: mx.google.com; dkim=pass header.i=@shutemov-name.20150623.gappssmtp.com header.s=20150623 header.b=QYQjXOEK; spf=neutral (google.com: 209.85.220.65 is neither permitted nor denied by best guess record for domain of kirill@shutemov.name) smtp.mailfrom=kirill@shutemov.name DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=shutemov-name.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=jbELOvLxzwiPzr0ySZKtIaNB4Xh08CmtwYy9J53koKc=; b=QYQjXOEKjuc+1XIIRQV/GUw9U2led8/IqlDQSRaZ0rObu4X6/lNib8OXdSb9foix32 eXrufZj2sPRKZ87+RjchynBpDyLsx56khC5/QYmAzkM4ldNGMXmiHgicWWzFsHDtfiX+ NVvxWi0HSb5vsPfbjfXkyorke8O5oRbSsAarbNOkq2PvLE0IhL1ViPZ4w3dj134+jmwX 7xarliCSMy6gR+0IZnaluVsnLK28SN5ZcJsLPudGJAFMyyGXrFvY9H8GboL844dDfPqQ ep5eQz3EweM4VnmT5dA4Azp/5HvMal30EiY/yPAJct2pBwMriWi9KvjMVHy4Nxks0F0f gaPQ== X-Google-Smtp-Source: AAOMgpdscddVocFP6EYmZxkJn+1EvBb7MXNJH4r53gG62r/PVoo6TjshfjDY0/7j7YMvF6Dv6Xyymw== X-Received: by 2002:a17:902:ac1:: with SMTP id 59-v6mr27766419plp.36.1531311696785; Wed, 11 Jul 2018 05:21:36 -0700 (PDT) Received: from kshutemo-mobl1.localdomain ([192.55.54.41]) by smtp.gmail.com with ESMTPSA id n83-v6sm39654835pfk.19.2018.07.11.05.21.35 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 11 Jul 2018 05:21:36 -0700 (PDT) Received: by kshutemo-mobl1.localdomain (Postfix, from userid 1000) id 2241C30029D; Wed, 11 Jul 2018 15:15:21 +0300 (+03) Date: Wed, 11 Jul 2018 15:15:21 +0300 From: "Kirill A. Shutemov" To: Andrew Morton Cc: "Kirill A. Shutemov" , Dmitry Vyukov , Oleg Nesterov , Andrea Arcangeli , linux-mm@kvack.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Subject: Re: [PATCH 1/2] mm: Fix vma_is_anonymous() false-positives Message-ID: <20180711121521.omugjfpuuyxscjjf@kshutemo-mobl1> References: <20180710134821.84709-1-kirill.shutemov@linux.intel.com> <20180710134821.84709-2-kirill.shutemov@linux.intel.com> <20180710134858.3506f097104859b533c81bf3@linux-foundation.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20180710134858.3506f097104859b533c81bf3@linux-foundation.org> User-Agent: NeoMutt/20180622 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: X-Virus-Scanned: ClamAV using ClamSMTP On Tue, Jul 10, 2018 at 01:48:58PM -0700, Andrew Morton wrote: > On Tue, 10 Jul 2018 16:48:20 +0300 "Kirill A. Shutemov" wrote: > > > vma_is_anonymous() relies on ->vm_ops being NULL to detect anonymous > > VMA. This is unreliable as ->mmap may not set ->vm_ops. > > > > False-positive vma_is_anonymous() may lead to crashes: > > > > ... > > > > This can be fixed by assigning anonymous VMAs own vm_ops and not relying > > on it being NULL. > > > > If ->mmap() failed to set ->vm_ops, mmap_region() will set it to > > dummy_vm_ops. This way we will have non-NULL ->vm_ops for all VMAs. > > Is there a smaller, simpler fix which we can use for backporting > purposes and save the larger rework for development kernels? I've tried to move dummy_vm_ops stuff into a separate patch, but it didn't workaround. In some cases (like in create_huge_pmd()/wp_huge_pmd()) we rely on vma_is_anonymous() to guarantee that ->vm_ops is non-NULL. But with new implementation of the helper there's no such guarantee. And I see crash in create_huge_pmd(). We can add explicit ->vm_ops check in such places. But it's more risky. I may miss some instances. dummy_vm_ops should be safer here. I think it's better to backport whole patch. > > > > > ... > > > > --- a/mm/mmap.c > > +++ b/mm/mmap.c > > @@ -71,6 +71,9 @@ int mmap_rnd_compat_bits __read_mostly = CONFIG_ARCH_MMAP_RND_COMPAT_BITS; > > static bool ignore_rlimit_data; > > core_param(ignore_rlimit_data, ignore_rlimit_data, bool, 0644); > > > > +const struct vm_operations_struct anon_vm_ops = {}; > > +const struct vm_operations_struct dummy_vm_ops = {}; > > Some nice comments here would be useful. Especially for dummy_vm_ops. > Why does it exist, what is its role, etc. Fixup is below. > > static void unmap_region(struct mm_struct *mm, > > struct vm_area_struct *vma, struct vm_area_struct *prev, > > unsigned long start, unsigned long end); > > @@ -561,6 +564,8 @@ static unsigned long count_vma_pages_range(struct mm_struct *mm, > > void __vma_link_rb(struct mm_struct *mm, struct vm_area_struct *vma, > > struct rb_node **rb_link, struct rb_node *rb_parent) > > { > > + WARN_ONCE(!vma->vm_ops, "missing vma->vm_ops"); > > + > > /* Update tracking information for the gap following the new vma. */ > > if (vma->vm_next) > > vma_gap_update(vma->vm_next); > > @@ -1774,12 +1779,19 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > > */ > > WARN_ON_ONCE(addr != vma->vm_start); > > > > + /* All mappings must have ->vm_ops set */ > > + if (!vma->vm_ops) > > + vma->vm_ops = &dummy_vm_ops; > > Can this happen? Can we make it a rule that file_operations.mmap(vma) > must initialize vma->vm_ops? Should we have a WARN here to detect when > the fs implementation failed to do that? Yes, it can happen. KCOV doesn't set it now. And I'm pretty sure some drivers do not set it too. We can add warning here. But I'm not sure what value it would have. It's perfectly fine to have no need in any of vm operations. Silently set it to dummy_vm_ops should be good enough here. > > addr = vma->vm_start; > > vm_flags = vma->vm_flags; > > } else if (vm_flags & VM_SHARED) { > > error = shmem_zero_setup(vma); > > if (error) > > goto free_vma; > > + } else { > > + /* vma_is_anonymous() relies on this. */ > + vma->vm_ops = &anon_vm_ops; > > } > > > > vma_link(mm, vma, prev, rb_link, rb_parent); > > ... > > > diff --git a/mm/mmap.c b/mm/mmap.c index 0729ed06b01c..6f59ade58fa7 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -71,7 +71,16 @@ int mmap_rnd_compat_bits __read_mostly = CONFIG_ARCH_MMAP_RND_COMPAT_BITS; static bool ignore_rlimit_data; core_param(ignore_rlimit_data, ignore_rlimit_data, bool, 0644); +/* + * All anonymous VMAs have ->vm_ops set to anon_vm_ops. + * vma_is_anonymous() reiles on anon_vm_ops to detect anonymous VMA. + */ const struct vm_operations_struct anon_vm_ops = {}; + +/* + * All VMAs have to have ->vm_ops set. dummy_vm_ops can be used if the VMA + * doesn't need to handle any of the operations. + */ const struct vm_operations_struct dummy_vm_ops = {}; static void unmap_region(struct mm_struct *mm,