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=-5.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no 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 32497C433F5 for ; Tue, 14 Sep 2021 18:01:54 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 12E3C60ED8 for ; Tue, 14 Sep 2021 18:01:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231723AbhINSDK (ORCPT ); Tue, 14 Sep 2021 14:03:10 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33010 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229526AbhINSDJ (ORCPT ); Tue, 14 Sep 2021 14:03:09 -0400 Received: from mail-lj1-x22a.google.com (mail-lj1-x22a.google.com [IPv6:2a00:1450:4864:20::22a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BAC71C061574 for ; Tue, 14 Sep 2021 11:01:51 -0700 (PDT) Received: by mail-lj1-x22a.google.com with SMTP id y6so298745lje.2 for ; Tue, 14 Sep 2021 11:01:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ib9hM4SdwqyHHEn/wkrupMf2QmzPr/LZkXrSoaMswps=; b=Xidz1rsc+T9svtmTJD7hvN6YJvyHBodIjNvg1pVxXgWpIQu9qZHaboKpNrR10ArmB9 KciJDw3aX4E9V3B1SXCL6oD25Qzs6+gqzoIH6yXrLe8GTshqXeeBfT4oQVYvZwjqsJH0 WznTWkD7Km8Gj4P5r+1XSzUHpgd7Xqj364k4c= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=ib9hM4SdwqyHHEn/wkrupMf2QmzPr/LZkXrSoaMswps=; b=PpocKeWiTwnvb7Q6ptnrSQeYiA7ttBNimkiJdDpp/4WcVDHQr6dJZQuBF8Sd78RSQN DvuA1LTUxGjtyjkZn0tBKRUHpeKBoL7bWQ0ZzZTgLiJD38l5znTpN5cTiQ0fEr6si5Zc oEGJf8eOiiOFP1/dTDxR/8nuYtEpZ9em1xX+IobpMZeo7MqtRfGOXrYPC7M09ToX2scG 8tcaK6L2h8YAbce94P2iMELOKNxunnZi15aKnZw8ZB+sGTCJkOjc/VW2AYh8/iLjx1xQ jGC3xJEU3WvfJ4LUf/5KU/0//o/iNdS6Kk2pd74U5ignHDbwCPYt9iOsO3NKJxbOO1VX Sn1g== X-Gm-Message-State: AOAM533VNgZZkw67tt3IGaTFGFIxn2HKU49WqWQdiLofAFc34hACHmHa Tm5nWCUeWdraIatY0GfuNxMg4E4HVjK3dEzkqzI= X-Google-Smtp-Source: ABdhPJypQU33uGIyyeMsNQOLk6KdDk1qMa2BYLjJ1zBRGcO3E9MYy0WoUuGu9Tm4n/a/jZwc6B2QGA== X-Received: by 2002:a2e:2201:: with SMTP id i1mr16977049lji.441.1631642509581; Tue, 14 Sep 2021 11:01:49 -0700 (PDT) Received: from mail-lj1-f176.google.com (mail-lj1-f176.google.com. [209.85.208.176]) by smtp.gmail.com with ESMTPSA id 203sm1452803ljf.63.2021.09.14.11.01.48 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 14 Sep 2021 11:01:48 -0700 (PDT) Received: by mail-lj1-f176.google.com with SMTP id f2so310852ljn.1 for ; Tue, 14 Sep 2021 11:01:48 -0700 (PDT) X-Received: by 2002:a2e:8107:: with SMTP id d7mr17033299ljg.68.1631642507876; Tue, 14 Sep 2021 11:01:47 -0700 (PDT) MIME-Version: 1.0 References: <20210914105620.677b90e5@oasis.local.home> In-Reply-To: <20210914105620.677b90e5@oasis.local.home> From: Linus Torvalds Date: Tue, 14 Sep 2021 11:01:31 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [GIT PULL] tracing: Fixes to bootconfig memory management To: Steven Rostedt , Mike Rapoport , Andrew Morton Cc: LKML , Ingo Molnar , Masami Hiramatsu , Linux-MM Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 14, 2021 at 7:56 AM Steven Rostedt wrote: > > A couple of memory management fixes to the bootconfig code These may be fixes, but they are too ugly to merit the tiny theoretical leak fix. All of these are just plain wrong: > +static void *init_xbc_data_copy __initdata; > +static phys_addr_t init_xbc_data_size __initdata; > + init_xbc_data_copy = copy; > + init_xbc_data_size = size + 1; > + memblock_free(__pa(init_xbc_data_copy), init_xbc_data_size); because the xbc code already saves these as xbc_data/xbc_data_size and that final free should just be done in xbc_destroy_all(). So this fix is pointlessly ugly to begin with. But what I _really_ ended up reacting to was that > + memblock_free(__pa(copy), size + 1); where that "copy" was allocated with copy = memblock_alloc(size + 1, SMP_CACHE_BYTES); so it should damn well be free'd without any crazy "__pa()" games. This is a memblock interface bug, plain and simple. Mike - this craziness needs to just be fixed. If memblock_alloc() returns a virtual address, then memblock_free() should take one. And if somebody has physical addresses because they aren't freeing previously allocated resources, but because they are initializing the memblock data from physical resources, then it shouldn't be called "memblock_free()". Alternatively, it should just _all_ be done in physaddr_t - that would at least be consistent. But it would be *bad*. Let's just get these interfaces fixed. It might be as simple as having a "memblock_free_phys()" interface, and doing a search-and-replace with coccinelle of memblock_free(__pa(xyz), .. -> memblock_free(xyz, ... memblock_free(other, .. -> memblock_free_phys(other, .. and adding the (trivial) internal helper functions to memblock, instead of making the atcual _users_ of memblock do insanely stupid and confusing things. Doing that automatic replacement might need an intermediate to avoid the ambiguous case - first translate memblock_free(__pa(xyz), .. -> memblock_free_sane(xyz, .. and then do any remaining memblock_free(xyz, .. -> memblock_free_phys(xyz, .. and then when there are no remaining cases of 'memblock_free()' left, do a final rename memblock_free_sane(.. -> memblock_free(.. but the actual commit can and should be just a single commit that just fixes 'memblock_free()' to have sane interfaces. Happily at least the type ends up making sure that we don't have subtle mistakes (ie physaddr_t is an integer type, and a virtual pointer is a pointer, so any missed conversions would cause nice compile-time errors). I hadn't noticed this insanity until now, but now that I do, I really don't want to add to the ugliness for some unimportant theoretical leak fix. The memblock code has had enough subtleties that having inconsistent and illogical basic interfaces is certainly not a good idea. Linus 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=-5.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no 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 98113C433EF for ; Tue, 14 Sep 2021 18:01:56 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 2796360ED8 for ; Tue, 14 Sep 2021 18:01:56 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 2796360ED8 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=linux-foundation.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 60F916B0071; Tue, 14 Sep 2021 14:01:53 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5BF976B0072; Tue, 14 Sep 2021 14:01:53 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 486AE6B0073; Tue, 14 Sep 2021 14:01:53 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0029.hostedemail.com [216.40.44.29]) by kanga.kvack.org (Postfix) with ESMTP id 38CF06B0071 for ; Tue, 14 Sep 2021 14:01:53 -0400 (EDT) Received: from smtpin10.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id B205839471 for ; Tue, 14 Sep 2021 18:01:52 +0000 (UTC) X-FDA: 78586947264.10.7C6C9E4 Received: from mail-lf1-f41.google.com (mail-lf1-f41.google.com [209.85.167.41]) by imf04.hostedemail.com (Postfix) with ESMTP id 54C7E50000B6 for ; Tue, 14 Sep 2021 18:01:52 +0000 (UTC) Received: by mail-lf1-f41.google.com with SMTP id k4so187338lfj.7 for ; Tue, 14 Sep 2021 11:01:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ib9hM4SdwqyHHEn/wkrupMf2QmzPr/LZkXrSoaMswps=; b=Xidz1rsc+T9svtmTJD7hvN6YJvyHBodIjNvg1pVxXgWpIQu9qZHaboKpNrR10ArmB9 KciJDw3aX4E9V3B1SXCL6oD25Qzs6+gqzoIH6yXrLe8GTshqXeeBfT4oQVYvZwjqsJH0 WznTWkD7Km8Gj4P5r+1XSzUHpgd7Xqj364k4c= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=ib9hM4SdwqyHHEn/wkrupMf2QmzPr/LZkXrSoaMswps=; b=DE+bInfcU3eACbCTUulu/+H4joR4ulU2V7VCoSJjQv2K+ktIjaHIo1cUbVceZDuX0N 6O0RBxd0JgUYOcCwgzbJiYnInjEZYHxFK5IgHp7mzePA9bfUM1iKqgoPHK9INl/8Z8+y euntMX3/IaIarVZNHpHysCtm9SplFXBJYsvRRkhHyJNi5CqXixFLG4J3VyhyRQR/TdT/ bUot7Q2FEq6jGZsIO920dAuKTxtwn8JrHHlKGZvoTKRi+4dOjEG+EOi6I1sJHHJ7BspF jxBx6k481sgBqMij830ue5t5lSHh0x3JDyTH3Xh6s8okgbNPJahfH/VpVMGbl/h9VxIo TKGA== X-Gm-Message-State: AOAM5313s66/jC+Ofck2N2LgFmCvzEVRpo5rYMRJlJD1NoadgsKWyBqM J0KUQHLiS/z6tSDCS/JJgDK1izvb4Xmv2700cT8= X-Google-Smtp-Source: ABdhPJz0uaA+7Ae3N/TJKsWmbgMt2kE8JE9V1B9uv1/7dXm4Ypt0d0RsqLZnX4u9GuP+yV8Swie4cw== X-Received: by 2002:ac2:44d5:: with SMTP id d21mr116735lfm.322.1631642509602; Tue, 14 Sep 2021 11:01:49 -0700 (PDT) Received: from mail-lj1-f182.google.com (mail-lj1-f182.google.com. [209.85.208.182]) by smtp.gmail.com with ESMTPSA id t12sm227731lfd.112.2021.09.14.11.01.48 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 14 Sep 2021 11:01:48 -0700 (PDT) Received: by mail-lj1-f182.google.com with SMTP id g14so271540ljk.5 for ; Tue, 14 Sep 2021 11:01:48 -0700 (PDT) X-Received: by 2002:a2e:8107:: with SMTP id d7mr17033299ljg.68.1631642507876; Tue, 14 Sep 2021 11:01:47 -0700 (PDT) MIME-Version: 1.0 References: <20210914105620.677b90e5@oasis.local.home> In-Reply-To: <20210914105620.677b90e5@oasis.local.home> From: Linus Torvalds Date: Tue, 14 Sep 2021 11:01:31 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [GIT PULL] tracing: Fixes to bootconfig memory management To: Steven Rostedt , Mike Rapoport , Andrew Morton Cc: LKML , Ingo Molnar , Masami Hiramatsu , Linux-MM Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 54C7E50000B6 X-Stat-Signature: tz4xug57tt1yuzp5k3muyjkcqgt3dw9n Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=linux-foundation.org header.s=google header.b=Xidz1rsc; spf=pass (imf04.hostedemail.com: domain of torvalds@linuxfoundation.org designates 209.85.167.41 as permitted sender) smtp.mailfrom=torvalds@linuxfoundation.org; dmarc=none X-HE-Tag: 1631642512-562547 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 Tue, Sep 14, 2021 at 7:56 AM Steven Rostedt wrote: > > A couple of memory management fixes to the bootconfig code These may be fixes, but they are too ugly to merit the tiny theoretical leak fix. All of these are just plain wrong: > +static void *init_xbc_data_copy __initdata; > +static phys_addr_t init_xbc_data_size __initdata; > + init_xbc_data_copy = copy; > + init_xbc_data_size = size + 1; > + memblock_free(__pa(init_xbc_data_copy), init_xbc_data_size); because the xbc code already saves these as xbc_data/xbc_data_size and that final free should just be done in xbc_destroy_all(). So this fix is pointlessly ugly to begin with. But what I _really_ ended up reacting to was that > + memblock_free(__pa(copy), size + 1); where that "copy" was allocated with copy = memblock_alloc(size + 1, SMP_CACHE_BYTES); so it should damn well be free'd without any crazy "__pa()" games. This is a memblock interface bug, plain and simple. Mike - this craziness needs to just be fixed. If memblock_alloc() returns a virtual address, then memblock_free() should take one. And if somebody has physical addresses because they aren't freeing previously allocated resources, but because they are initializing the memblock data from physical resources, then it shouldn't be called "memblock_free()". Alternatively, it should just _all_ be done in physaddr_t - that would at least be consistent. But it would be *bad*. Let's just get these interfaces fixed. It might be as simple as having a "memblock_free_phys()" interface, and doing a search-and-replace with coccinelle of memblock_free(__pa(xyz), .. -> memblock_free(xyz, ... memblock_free(other, .. -> memblock_free_phys(other, .. and adding the (trivial) internal helper functions to memblock, instead of making the atcual _users_ of memblock do insanely stupid and confusing things. Doing that automatic replacement might need an intermediate to avoid the ambiguous case - first translate memblock_free(__pa(xyz), .. -> memblock_free_sane(xyz, .. and then do any remaining memblock_free(xyz, .. -> memblock_free_phys(xyz, .. and then when there are no remaining cases of 'memblock_free()' left, do a final rename memblock_free_sane(.. -> memblock_free(.. but the actual commit can and should be just a single commit that just fixes 'memblock_free()' to have sane interfaces. Happily at least the type ends up making sure that we don't have subtle mistakes (ie physaddr_t is an integer type, and a virtual pointer is a pointer, so any missed conversions would cause nice compile-time errors). I hadn't noticed this insanity until now, but now that I do, I really don't want to add to the ugliness for some unimportant theoretical leak fix. The memblock code has had enough subtleties that having inconsistent and illogical basic interfaces is certainly not a good idea. Linus