LKML Archive on
help / color / mirror / Atom feed
From: Linus Torvalds <>
To: Steven Rostedt <>,
	Mike Rapoport <>,
	Andrew Morton <>
Cc: LKML <>,
	Ingo Molnar <>,
	Masami Hiramatsu <>,
	Linux-MM <>
Subject: Re: [GIT PULL] tracing: Fixes to bootconfig memory management
Date: Tue, 14 Sep 2021 11:01:31 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <20210914105620.677b90e5@oasis.local.home>

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

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.


  reply	other threads:[~2021-09-14 18:01 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-14 14:56 Steven Rostedt
2021-09-14 18:01 ` Linus Torvalds [this message]
2021-09-14 18:59   ` Steven Rostedt
2021-09-14 19:05     ` Linus Torvalds
2021-09-14 19:14       ` Steven Rostedt
2021-09-14 19:23       ` Linus Torvalds
2021-09-14 19:38         ` Linus Torvalds
2021-09-14 20:48           ` Linus Torvalds
2021-09-14 21:05             ` Steven Rostedt
2021-09-14 22:47               ` Vlastimil Babka
2021-09-14 23:29                 ` Linus Torvalds
2021-09-15  9:28                   ` Vlastimil Babka
2021-09-14 23:44               ` Masami Hiramatsu
2021-09-17 20:10   ` Mike Rapoport

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='' \ \ \ \ \ \ \ \ \
    --subject='Re: [GIT PULL] tracing: Fixes to bootconfig memory management' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).