linux-kernel - Re: [PATCH -V5 RESEND 03/21] swap: Support PMD swap mapping in swap

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: <877ej5f7oq.fsf@yhuang-dev.intel.com>
Date:   Sat, 29 Sep 2018 08:50:29 +0800
From:   "Huang\, Ying" <ying.huang@...el.com>
To:     Daniel Jordan <daniel.m.jordan@...cle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>, <linux-mm@...ck.org>,
        <linux-kernel@...r.kernel.org>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Michal Hocko <mhocko@...nel.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Shaohua Li <shli@...nel.org>, Hugh Dickins <hughd@...gle.com>,
        Minchan Kim <minchan@...nel.org>,
        Rik van Riel <riel@...hat.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Naoya Horiguchi <n-horiguchi@...jp.nec.com>,
        Zi Yan <zi.yan@...rutgers.edu>
Subject: Re: [PATCH -V5 RESEND 03/21] swap: Support PMD swap mapping in swap_duplicate()

Daniel Jordan <daniel.m.jordan@...cle.com> writes:

> On Fri, Sep 28, 2018 at 04:19:03PM +0800, Huang, Ying wrote:
>> Daniel Jordan <daniel.m.jordan@...cle.com> writes:
>> > One way is to change
>> > copy_one_pte's return to int so we can just pass the error code back to
>> > copy_pte_range so it knows whether to try adding the continuation.
>> 
>> There may be even more problems.  After add_swap_count_continuation(),
>> copy_one_pte() will be retried, and the CPU may hang with dead loop.
>
> That's true, it would do that.
>
>> But before the changes in this patchset, the behavior is,
>> __swap_duplicate() return an error that isn't -ENOMEM, such as -EEXIST.
>> Then copy_one_pte() would thought the operation has been done
>> successfully, and go to call set_pte_at().  This will cause the system
>> state become inconsistent, and the system may panic or hang somewhere
>> later.
>> 
>> So per my understanding, if we thought page table corruption isn't a
>> real problem (that is, __swap_duplicate() will never return e.g. -EEXIST
>> if copied by copy_one_pte() indirectly), both the original and the new
>> code should be OK.
>> 
>> If we thought it is a real problem, we need to fix the original code and
>> keep it fixed in the new code.  Do you agree?
>
> Yes, if it was a real problem, which seems less and less the case the more I
> stare at this.
>
>> There's several ways to fix the problem.  But the page table shouldn't
>> be corrupted in practice, unless there's some programming error.  So I
>> suggest to make it as simple as possible via adding,
>> 
>> VM_BUG_ON(error != -ENOMEM);
>> 
>> in swap_duplicate().
>> 
>> Do you agree?
>
> Yes, I'm ok with that, adding in -ENOTDIR along with it.

Sure.  I will do this.

> The error handling in __swap_duplicate (before this series) still leaves
> something to be desired IMHO.  Why all the different returns when callers
> ignore them or only specifically check for -ENOMEM or -EEXIST?  Could maybe
> stand a cleanup, but outside this series.

Yes.  Maybe.  I guess you will work on this?

Best Regards,
Huang, Ying