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=-9.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable 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 974F1C4338F for ; Fri, 20 Aug 2021 08:46:56 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 80544610FF for ; Fri, 20 Aug 2021 08:46:56 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232507AbhHTIrc (ORCPT ); Fri, 20 Aug 2021 04:47:32 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:20423 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230450AbhHTIr2 (ORCPT ); Fri, 20 Aug 2021 04:47:28 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1629449210; h=from:from: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:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=2HhD3HCRTFvEVqk5yap83SXC5Y82U0v5dcvw39zc4Z4=; b=YLxiu0lCFQYyMP91p9KefzStodfvkg5GDrKgEpQMfpjqVf982M+l6Y7dVmmHQ3PfKe0vrU hcYdpLr1tP3YKcelZZi/w4wizftrP3KiK8LRCCJhCY0jUgacFrqJIPChccDGGaPRWXmcJX cRTtoocizyOEvC9lg8+JH8EdbD0IqjQ= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-488-7pUhKZP8M5KbpP4CSZXy7g-1; Fri, 20 Aug 2021 04:46:49 -0400 X-MC-Unique: 7pUhKZP8M5KbpP4CSZXy7g-1 Received: by mail-wr1-f70.google.com with SMTP id k15-20020a5d628f0000b029015501bab520so2627181wru.16 for ; Fri, 20 Aug 2021 01:46:49 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:to:cc:references:from:organization:subject :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=2HhD3HCRTFvEVqk5yap83SXC5Y82U0v5dcvw39zc4Z4=; b=aVzZ1NwaVg8jE+YlkCLYI/gcXddAoKSiiL4GGNG8GkU2VmfB8IlvLjHu3Bs/CUkpLY 3a6G4LH2bkeobI46umgJbEDSJ1upeRuVROhOef69c/lejWFRis4NCyJdkBm525Q8Evze +H0G+tUoktayeppbTykwIF33IvcchH48gKhIbf5zfJNiUzCv5YKknLqUVW6aSDLqaAyJ 8qNq7F0UlJ4BR5R2WJXQyWq/SIp1CRg2exTgizittvBw2a44r1S9d3eo+5IusbxbQubX u0SmIhsoL1m2oXZsXO5qqqoRi2OWPzSECIjx8ZArL74Y1Xc1JsAlXAGOK86DOI4CRpdc ISFA== X-Gm-Message-State: AOAM533HPSf6yyisFrjFvEYO6z4EHsR5+6AY9TfwCaC5jPXQcQD5n8+Y UPyTOELTXzkn+dUzJ28ymtOtsPLyASlBj/hbacoDIXg6e63UrQ7phVWZoe7+cLdoPjIcTaYFL+k K5bzOVI/FHaU/UBvS2iKT3VDp X-Received: by 2002:a1c:3b09:: with SMTP id i9mr2685741wma.62.1629449208321; Fri, 20 Aug 2021 01:46:48 -0700 (PDT) X-Google-Smtp-Source: ABdhPJytHgubr94As8EiptaA6/T3rm3zShRCHuT1SQpKBWMx/QJTseDVs/BhmrdZPds05dmrZoxc2A== X-Received: by 2002:a1c:3b09:: with SMTP id i9mr2685692wma.62.1629449208132; Fri, 20 Aug 2021 01:46:48 -0700 (PDT) Received: from ?IPv6:2003:d8:2f0a:7f00:fad7:3bc9:69d:31f? (p200300d82f0a7f00fad73bc9069d031f.dip0.t-ipconnect.de. [2003:d8:2f0a:7f00:fad7:3bc9:69d:31f]) by smtp.gmail.com with ESMTPSA id l9sm5187699wrt.95.2021.08.20.01.46.45 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 20 Aug 2021 01:46:47 -0700 (PDT) To: Linus Torvalds Cc: Linux Kernel Mailing List , Andrew Morton , Thomas Gleixner , Ingo Molnar , Borislav Petkov , "H. Peter Anvin" , Alexander Viro , Alexey Dobriyan , Steven Rostedt , Peter Zijlstra , Arnaldo Carvalho de Melo , Mark Rutland , Alexander Shishkin , Jiri Olsa , Namhyung Kim , Petr Mladek , Sergey Senozhatsky , Andy Shevchenko , Rasmus Villemoes , Kees Cook , "Eric W. Biederman" , Greg Ungerer , Geert Uytterhoeven , Mike Rapoport , Vlastimil Babka , Vincenzo Frascino , Chinwen Chang , Catalin Marinas , "Matthew Wilcox (Oracle)" , Huang Ying , Jann Horn , Feng Tang , Kevin Brodsky , Michael Ellerman , Shawn Anastasio , Steven Price , Nicholas Piggin , Christian Brauner , Jens Axboe , Gabriel Krisman Bertazi , Peter Xu , Suren Baghdasaryan , Shakeel Butt , Marco Elver , Daniel Jordan , Nicolas Viennot , Thomas Cedeno , Michal Hocko , Miklos Szeredi , Chengguang Xu , =?UTF-8?Q?Christian_K=c3=b6nig?= , Florian Weimer , David Laight , linux-unionfs@vger.kernel.org, Linux API , the arch/x86 maintainers , linux-fsdevel , Linux-MM References: <20210816194840.42769-1-david@redhat.com> <20210816194840.42769-3-david@redhat.com> From: David Hildenbrand Organization: Red Hat Subject: Re: [PATCH v2 2/7] kernel/fork: factor out replacing the current MM exe_file Message-ID: Date: Fri, 20 Aug 2021 10:46:45 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 19.08.21 22:51, Linus Torvalds wrote: > So I like this series. > > However, logically, I think this part in replace_mm_exe_file() no > longer makes sense: > > On Mon, Aug 16, 2021 at 12:50 PM David Hildenbrand wrote: >> >> + /* Forbid mm->exe_file change if old file still mapped. */ >> + old_exe_file = get_mm_exe_file(mm); >> + if (old_exe_file) { >> + mmap_read_lock(mm); >> + for (vma = mm->mmap; vma && !ret; vma = vma->vm_next) { >> + if (!vma->vm_file) >> + continue; >> + if (path_equal(&vma->vm_file->f_path, >> + &old_exe_file->f_path)) >> + ret = -EBUSY; >> + } >> + mmap_read_unlock(mm); >> + fput(old_exe_file); >> + if (ret) >> + return ret; >> + } > > and should just be removed. > > NOTE! I think it makes sense within the context of this patch (where > you just move code around), but that it should then be removed in the > next patch that does that "always deny write access to current MM > exe_file" thing. > > I just quoted it in the context of this patch, since the next patch > doesn't actually show this code any more. > > In the *old* model - where the ETXTBUSY was about the mmap() of the > file - the above tests make sense. > > But in the new model, walking the mappings just doesn't seem to be a > sensible operation any more. The mappings simply aren't what ETXTBUSY > is about in the new world order, and so doing that mapping walk seems > nonsensical. > > Hmm? I think this is somewhat another kind of "stop user space trying to do stupid things" thingy, not necessarily glued to ETXTBUSY: don't allow replacing exe_file if that very file is still mapped and consequently eventually still in use by the application. I don't think it necessarily has many things to do with ETXTBUSY: we only check if there is a VMA mapping that file, not that it's a VM_DENYWRITE mapping. That code originates from commit 4229fb1dc6843c49a14bb098719f8a696cdc44f8 Author: Konstantin Khlebnikov Date: Wed Jul 11 14:02:11 2012 -0700 c/r: prctl: less paranoid prctl_set_mm_exe_file() "no other files mapped" requirement from my previous patch (c/r: prctl: update prctl_set_mm_exe_file() after mm->num_exe_file_vmas removal) is too paranoid, it forbids operation even if there mapped one shared-anon vma. Let's check that current mm->exe_file already unmapped, in this case exe_file symlink already outdated and its changing is reasonable. The statement "exe_file symlink already outdated and its changing is reasonable" somewhat makes sense. Long story short, I think this check somehow makes a bit of sense, but we wouldn't lose too much if we drop it -- just another sanity check. Your call :) -- Thanks, David / dhildenb