Re: [PATCH] hibernate: unlock swap bdev for writing when uswsusp is active

[Domenico Andreoli <domenico.andreoli.it@gmail.com>]

On March 3, 2020 7:02:12 PM UTC, "Darrick J. Wong" <darrick.wong@oracle.com> wrote:
>On Sun, Mar 01, 2020 at 10:35:36PM +0100, Rafael J. Wysocki wrote:
>> On Sat, Feb 29, 2020 at 9:02 PM Domenico Andreoli
>> <domenico.andreoli@linux.com> wrote:
>> >
>> > On Sat, Feb 29, 2020 at 10:38:20AM -0800, Darrick J. Wong wrote:
>> > > On Sat, Feb 29, 2020 at 07:07:16PM +0100, Domenico Andreoli
>wrote:
>> > > > On Sat, Feb 29, 2020 at 09:08:25AM -0800, Darrick J. Wong
>wrote:
>> > > > > From: Darrick J. Wong <darrick.wong@oracle.com>
>> > > > >
>> > > > > It turns out that there /is/ one use case for programs being
>able to
>> > > > > write to swap devices, and that is the userspace hibernation
>code.  The
>> > > > > uswsusp ioctls allow userspace to lease parts of swap
>devices, so turn
>> > > > > S_SWAPFILE off when invoking suspend.
>> > > > >
>> > > > > Fixes: 1638045c3677 ("mm: set S_SWAPFILE on blockdev swap
>devices")
>> > > > > Reported-by: Domenico Andreoli <domenico.andreoli@linux.com>
>> > > > > Reported-by: Marian Klein <mkleinsoft@gmail.com>
>> > > >
>> > > > I also tested it yesterday but was not satisfied, unfortunately
>I did
>> > > > not come with my comment in time.
>> > > >
>> > > > Yes, I confirm that the uswsusp works again but also checked
>that
>> > > > swap_relockall() is not triggered at all and therefore after
>the first
>> > > > hibernation cycle the S_SWAPFILE bit remains cleared and the
>whole
>> > > > swap_relockall() is useless.
>> > > >
>> > > > I'm not sure this patch should be merged in the current form.
>> > >
>> > > NNGGHHGGHGH /me is rapidly losing his sanity and will soon just
>revert
>> > > the whole security feature because I'm getting fed up with people
>> > > yelling at me *while I'm on vacation* trying to *restore* my
>sanity.  I
>> > > really don't want to be QAing userspace-directed hibernation
>right now.
>> >
>> > Maybe we could proceed with the first patch to amend the regression
>and
>> > postpone the improved fix to a later patch? Don't loose sanity for
>this.
>> 
>> I would concur here.
>> 
>> > > ...right, the patch is broken because we have to relock the
>swapfiles in
>> > > whatever code executes after we jump back to the restored kernel,
>not in
>> > > the one that's doing the restoring.  Does this help?
>> >
>> > I made a few unsuccessful attempts in kernel/power/hibernate.c and
>> > eventually I'm switching to qemu to speed up the test cycle.
>> >
>> > > OTOH, maybe we should just leave the swapfiles unlocked after
>resume.
>> > > Userspace has clearly demonstrated the one usecase for writing to
>the
>> > > swapfile, which means anyone could have jumped in while uswsusp
>was
>> > > running and written whatever crap they wanted to the parts of the
>swap
>> > > file that weren't leased for the hibernate image.
>> >
>> > Essentially, if the hibernation is supported the swapfile is not
>totally
>> > safe.
>> 
>> But that's only the case with the userspace variant, isn't it?
>
>Yes.
>
>> > Maybe user-space hibernation should be a separate option.
>> 
>> That actually is not a bad idea at all in my view.
>
>The trouble with kconfig options is that the distros will be pressued
>into setting CONFIG_HIBERNATE_USERSPACE=y to avoid regressing their
>uswsusp users, which makes the added security code pointless.  As this

True but there are not only distros otherwise the kernel would not have any option at all.

It's actually very nice that if hibernation is disabled no userspace is ever allowed to write to the swap.

>has clearly sucked me into a conflict that I don't have the resources
>to
>pursue, I'm going to revert the write patch checks and move on with
>life.

I don't see the need of reverting anything, I can deal with these issues if you are busy on something else.

>
>--D
>
>> Thanks!