linux-kernel - Re: [PATCH v2 1/1] swap: shmem: 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-prev] [day] [month] [year] [list]

Message-ID: <CAJD7tkaZ8gqv+iZuGG2Ce9fhVsmy+a_SbRc-kz-CJvaJsMZdig@mail.gmail.com>
Date: Wed, 2 Oct 2024 11:06:39 -0700
From: Yosry Ahmed <yosryahmed@...gle.com>
To: Nhat Pham <nphamcs@...il.com>
Cc: akpm@...ux-foundation.org, hannes@...xchg.org, hughd@...gle.com, 
	shakeel.butt@...ux.dev, ryan.roberts@....com, ying.huang@...el.com, 
	chrisl@...nel.org, david@...hat.com, kasong@...cent.com, willy@...radead.org, 
	viro@...iv.linux.org.uk, baohua@...nel.org, chengming.zhou@...ux.dev, 
	v-songbaohua@...o.com, linux-mm@...ck.org, kernel-team@...a.com, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] swap: shmem: remove SWAP_MAP_SHMEM

On Wed, Oct 2, 2024 at 11:01 AM Nhat Pham <nphamcs@...il.com> wrote:
>
> On Tue, Oct 1, 2024 at 7:14 PM Yosry Ahmed <yosryahmed@...gle.com> wrote:
> >
> > On Tue, Oct 1, 2024 at 7:06 PM Yosry Ahmed <yosryahmed@...gle.com> wrote:
> > >
> > > On Tue, Oct 1, 2024 at 7:04 PM Nhat Pham <nphamcs@...il.com> wrote:
> > > >
> > > > On Tue, Oct 1, 2024 at 6:58 PM Nhat Pham <nphamcs@...il.com> wrote:
> > > > >
> > > > > On Tue, Oct 1, 2024 at 6:33 PM Yosry Ahmed <yosryahmed@...gle.com> wrote:
> > > > >
> > > > > I was debating between WARN-ing here, and returning -ENOMEM and
> > > > > WARN-ing at shmem's callsite.
> > > > >
> > > > > My thinking is that if we return -ENOMEM here, it will work in the
> > > > > current setup, for both shmem and other callsites. However, in the
> > > > > future, if we add another user of swap_duplicate_nr(), this time
> > > > > without guaranteeing that we won't need continuation, I think it won't
> > > > > work unless we have the fallback logic in place as well:
> > > > >
> > > > > while (!err && __swap_duplicate(entry, 1, nr) == -ENOMEM)
> > > > > err = add_swap_count_continuation(entry, GFP_ATOMIC);
> > > >
> > > > Sorry, I accidentally sent out the email without completing my explanation :)
> > > >
> > > > Anyway, the point being, with the current implementation, any new user
> > > > would immediately hit a WARN and the implementer will know to check.
> > > >
> > > > Whereas if we return -ENOMEM in __swap_duplicate(), then I think we
> > > > would just hang, no? We only try to add swap count continuation to the
> > > > first entry only, which is not sufficient to fix the problem.
> > > >
> > > > I can probably whip up the fallback logic here, but it would be dead,
> > > > untestable code (as it has no users, and I cannot even conceive one to
> > > > test it). And the swap abstraction might render all of this moot
> > > > anyway.
> > >
> > > What I had in mind is not returning -ENOMEM at all, but something like
> > > -EOPNOTSUPP. The swap_duplicate_nr() will just return the error to the
> > > caller. All callers of swap_duplicate() and swap_duplicate_nr()
> > > currently check the error except shmem.
> >
> > ..and just to be extra clear, I meant WARN _and_ return -EOPNOTSUPP.
>
> Ah ok this makes a lot of sense actually.
>
> I'll return -EOPNOTSUPP here. Do you think warn within
> __swap_duplicate() makes more sense, or at shmem's callsite make more
> sense?
>
> I feel like we should warn within __swap_duplicate callsite. That way
> if we accidentally screw up for other swap_duplicaters in the future,
> the feedback will be immediate :)

I think we should warn in __swap_duplicate(). We can also propagate
the error from shmem_writepage() to the caller, but I think this may
need extra cleanup to be properly handled, didn't look too closely.

We can also warn in swap_duplicate_nr() if we ever reach the -ENOMEM
fallback code with nr > 1, and document there that the current
fallback logic does not handle this case (instead of documenting it
above the function). This will make sure we never return -ENOMEM from
__swap_duplicate() incorrectly.