linux-kernel - Re: [PATCH v4 4/4] selftests/hmm-tests: Add test for dirty bits

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] [day] [month] [year] [list]

Date:   Tue, 13 Sep 2022 12:33:40 +1000
From:   Alistair Popple <apopple@...dia.com>
To:     John Hubbard <jhubbard@...dia.com>
Cc:     linux-mm@...ck.org, akpm@...ux-foundation.org,
        Peter Xu <peterx@...hat.com>,
        Nadav Amit <nadav.amit@...il.com>,
        huang ying <huang.ying.caritas@...il.com>,
        LKML <linux-kernel@...r.kernel.org>,
        "Sierra Guiza, Alejandro (Alex)" <alex.sierra@....com>,
        Felix Kuehling <Felix.Kuehling@....com>,
        Jason Gunthorpe <jgg@...dia.com>,
        David Hildenbrand <david@...hat.com>,
        Ralph Campbell <rcampbell@...dia.com>,
        Matthew Wilcox <willy@...radead.org>,
        Karol Herbst <kherbst@...hat.com>,
        Lyude Paul <lyude@...hat.com>, Ben Skeggs <bskeggs@...hat.com>,
        Logan Gunthorpe <logang@...tatee.com>, paulus@...abs.org,
        linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v4 4/4] selftests/hmm-tests: Add test for dirty bits


John Hubbard <jhubbard@...dia.com> writes:

> On 9/7/22 04:13, Alistair Popple wrote:
>>>> +	/*
>>>> +	 * Attempt to migrate memory to device, which should fail because
>>>> +	 * hopefully some pages are backed by swap storage.
>>>> +	 */
>>>> +	ASSERT_TRUE(hmm_migrate_sys_to_dev(self->fd, buffer, npages));
>>>
>>> Are you really sure that you want to assert on that? Because doing so
>>> guarantees a test failure if and when we every upgrade the kernel to
>>> be able to migrate swap-backed pages. And I seem to recall that this
>>> current inability to migrate swap-backed pages is considered a flaw
>>> to be fixed, right?
>> Right, that's a good point. I was using failure (ASSERT_TRUE) here as a
>> way of detecting that at least some pages are swap-backed, because if no
>> pages end up being swap-backed the test is invalid.
>
> Yes. But "invalid" or "waived" is a much different test result than
> "failed".

True. Unfortunately our test framework needs some love as I don't think
it's possible to return a result of "invalid" or "waived". We can skip a
test though, so that might be the best option here.

>> I'm not really sure what to do about it though. It's likely the fix for
>
> Remove the assert. If the test framework allows and you prefer, you
> can print a warning.
>
>> swap-backed migration may make this bug impossible to hit anyway,
>> because the obvious fix is to just drop the pages from the swapcache
>> during migration which would force writeback during subsequent reclaim.
>> So I'm inclined to leave this here even if it only serves to remind us
>> about it when we do fix migration of swap-backed pages, because we will
>> of course run hmm-tests before submitting that fix :-) We can then
>> either fix the test or drop it if we think it's no longer possible to
>> hit.
>
> Oh no no no, please. This is not how to do tests. If you want a TODO
> list somewhere, there are other ways. But tests that require maintenance
> when you change something are an anti-pattern.

Fair enough, I think what you're asking for is a higher level test that
doesn't rely on implementation side-effects. I wrote this test mostly to
discover if we could hit problems with the current implementation hence
why it's a bit messy.

But I think I can fix this up without relying on implementation
side-effects - really I just want to confirm that at least some pages
got swapped to disk which I can do via looking at /proc/self/pagemap.

 - Alistair

> thanks,