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]

Date:   Fri, 28 Sep 2018 14:32:24 -0700
From:   Daniel Jordan <daniel.m.jordan@...cle.com>
To:     "Huang, Ying" <ying.huang@...el.com>
Cc:     Daniel Jordan <daniel.m.jordan@...cle.com>,
        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()

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.

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.