linux-kernel - Re: [PATCH v2] x86/hibernate: Use fixmap for saving unmapped pages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0hmuWRtMkfsbPkiK5J=S5XvO1U2tzrL+dpPBGwbLJz=5A@mail.gmail.com>
Date:   Thu, 12 Jan 2023 20:37:58 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
Cc:     "rafael@...nel.org" <rafael@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "rppt@...nel.org" <rppt@...nel.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "pavel@....cz" <pavel@....cz>, "x86@...nel.org" <x86@...nel.org>,
        "hpa@...or.com" <hpa@...or.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "Lutomirski, Andy" <luto@...nel.org>,
        "bp@...en8.de" <bp@...en8.de>, "Brown, Len" <len.brown@...el.com>
Subject: Re: [PATCH v2] x86/hibernate: Use fixmap for saving unmapped pages

On Thu, Jan 12, 2023 at 8:15 PM Edgecombe, Rick P
<rick.p.edgecombe@...el.com> wrote:
>
> On Thu, 2023-01-12 at 15:15 +0100, Rafael J. Wysocki wrote:
> > > >
> > > > I don't think the above is needed.  The code using this function
> > > > cannot be preempted anyway AFAICS.
> > >
> > > The reason I thought it was useful was because this function is now
> > > defined in a header. Someone else might decide to use it. Does it
> > > seem
> > > more useful?
> >
> > Well, it is exposed now, but only in order to allow the __weak
> > function to be overridden.  I don't think it is logically valid to
> > use
> > it anywhere beyond its original call site.
> >
> > To make that clear, I would call it something hibernation-specific,
> > like hibernate_copy_unmapped_page() and I would add a kerneldoc
> > comment to it to describe its intended use.
>
> Ok, I'll change the name, that makes sense.
>
> On the warning, ok, I'll drop it. But to me the code stand out as
> questionable with the PTE change and only the local TLB flush. It's a
> bit of a comment as code on a rare path.
>
> >
> > Furthermore, I'm not sure about the new code layout.
> >
> > Personally, I would prefer hibernate_map_page() and
> > hibernate_unmap_page() to be turned into __weak functions and
> > possibly
> > overridden by the arch code, which would allow the amount of changes
> > to be reduced and do_copy_page() wouldn't need to be moved into the
> > header any more.
>
> Currently hibernate_map_page() maps the page on the direct map and
> doesn't return anything. This new code effectively creates a readable
> alias in the fixmap. So it would have to return an address to use so
> the core hibernate code would know where to copy from. Then it would
> have to pass it back into hibernate_unmap_page() for the arch to decide
> what to do to clean it up. I think it would be more complicated.

AFAICS, you only need hibernate_map_page() to return an address that
will be passed to do_copy_page() (that doesn't need to be touched
otherwise) and hibernate_unmap_page() would just do

clear_fixmap(FIX_HIBERNATE);

so it may as well take the page as the argument.

> There is also already multiple paths in hibernate_map_page() that would
> have to be duplicated in the arch versions.

But only if the arch decides to override the originals.

Now, the only caller of both hibernate_map_page() and
hibernate_unmap_page() is safe_copy_page() in the "else" branch.  Your
current patch replaces that branch completely with an arch version, so
they are not going to be invoked on x86 anyway.

> So I see the idea, but I'm not sure it ends up better. Can we leave this one?

I first need to be convinced that it is indeed better.