linux-kernel - Re: [RFC PATCH 0/2] remove SWAP_MAP

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

Message-ID: <CAKEwX=NuRuu9qXA9mRMqb6Okcwa86mEkp2Ac8sQjwb0ACdS7YQ@mail.gmail.com>
Date: Wed, 25 Sep 2024 07:21:40 -0700
From: Nhat Pham <nphamcs@...il.com>
To: Barry Song <baohua@...nel.org>
Cc: "Huang, Ying" <ying.huang@...el.com>, Yosry Ahmed <yosryahmed@...gle.com>, 
	Baolin Wang <baolin.wang@...ux.alibaba.com>, akpm@...ux-foundation.org, 
	hannes@...xchg.org, hughd@...gle.com, shakeel.butt@...ux.dev, 
	ryan.roberts@....com, chrisl@...nel.org, david@...hat.com, kasong@...cent.com, 
	willy@...radead.org, viro@...iv.linux.org.uk, chengming.zhou@...ux.dev, 
	linux-mm@...ck.org, kernel-team@...a.com, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 0/2] remove SWAP_MAP_SHMEM

On Wed, Sep 25, 2024 at 12:33 AM Barry Song <baohua@...nel.org> wrote:
>
> On Wed, Sep 25, 2024 at 3:23 PM Huang, Ying <ying.huang@...el.com> wrote:
> >
> > Nhat Pham <nphamcs@...il.com> writes:
> >
> > [snip]
> >
> > >
> > > My understanding now is that there are two for loops. One for loop
> > > that checks the entry's states, and one for loop that does the actual
> > > incrementing work (or state modification).
> > >
> > > We can check in the first for loop, if it is safe to proceed:
> > >
> > > if (!count && !has_cache) {
> > >     err = -ENOENT;
> > > } else if (usage == SWAP_HAS_CACHE) {
> > > if (has_cache)
> > >     err = -EEXIST;
> > > } else if ((count & ~COUNT_CONTINUED) > SWAP_MAP_MAX) {
> > >     err = -EINVAL;
> > > } else if (usage == 1 && nr > 1 && (count & ~COUNT_CONTINUED) >=
> > > SWAP_MAP_MAX)) {
> > >     /* the batched variants currently do not support rollback */
> > >     err = -ENOMEM;
> > > }
> > >
> > > At this point, IIUC, we have not done any incrementing, so no rollback
> > > needed? :)
> >
> > I think that it's better to add a VM_WARN_ONCE() here.  If someone
> > enabled 'nr > 1' for __swap_duplicate(), the issue will be more
> > explicit.
>
> ying, i guess you missed this is the exact case Nhat is enabling
>  'nr > 1' for __swap_duplicate(). and this warning is functioning.
> and he is trying to support the nr>1 case.
>
> https://lore.kernel.org/linux-mm/20240923231142.4155415-2-nphamcs@gmail.com/

I'm only supporting the case nr > 1, when there is no need to add swap
continuations :) That's the only current use case right now (shmem) :)

1. Keep the non-batched variant:

int swap_duplicate(swp_entry_t entry)
{
    int err = 0;

    while (!err && __swap_duplicate(entry, 1, 1) == -ENOMEM)
        err = add_swap_count_continuation(entry, GFP_ATOMIC);
    return err;
}

2. Implement the batched variant:

int swap_duplicate_nr(swp_entry_t entry, int nr)
{
    swp_entry_t cur_entry;
    int i, err;

    if (nr == 1)
        return swap_duplicate(entry);

    err = __swap_duplicate(entry, 1, nr);
    if (err == -ENOMEM) {
        /* fallback to non-batched version */
        for (i = 0; i < nr; i++) {
            cur_entry = (swp_entry_t){entry.val + i};
            if (swap_duplicate(cur_entry)) {
                /* rollback */
                while (--i >= 0) {
                     cur_entry = (swp_entry_t){entry.val + i};
                     swap_free(cur_entry);
                }
            }
        }
    }
   return err;
}

How does this look? My concern is that there is not really a use for
the fallback logic. Basically dead code.

I can keep it in if you guys have a use for it soon, but otherwise I
lean towards just adding a WARN etc. there, or return -ENOMEM, and
WARN at shmem's callsite (because it cannot get -ENOMEM).