| 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
| ||
|
Message-ID: <CAKEwX=OPaBCYHSesm7wT_+k-MExQk9b8wzEaEg6z9581YkPevA@mail.gmail.com>
Date: Tue, 1 Oct 2024 18:58:57 -0700
From: Nhat Pham <nphamcs@...il.com>
To: Yosry Ahmed <yosryahmed@...gle.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 Tue, Oct 1, 2024 at 6:33 PM Yosry Ahmed <yosryahmed@...gle.com> wrote:
>
>
> Nit: extra blank line.
>
> > static inline void free_swap_and_cache(swp_entry_t entry)
> > {
> > free_swap_and_cache_nr(entry, 1);
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 0613421e09e7..e3f72f99be32 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -1561,7 +1561,7 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
> > __GFP_HIGH | __GFP_NOMEMALLOC | __GFP_NOWARN,
> > NULL) == 0) {
> > shmem_recalc_inode(inode, 0, nr_pages);
> > - swap_shmem_alloc(swap, nr_pages);
> > + swap_duplicate_nr(swap, nr_pages);
> > shmem_delete_from_page_cache(folio, swp_to_radix_entry(swap));
> >
> > mutex_unlock(&shmem_swaplist_mutex);
> > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > index 0cded32414a1..9bb94e618914 100644
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -1381,12 +1381,6 @@ static unsigned char __swap_entry_free_locked(struct swap_info_struct *si,
> > if (usage == SWAP_HAS_CACHE) {
> > VM_BUG_ON(!has_cache);
> > has_cache = 0;
> > - } else if (count == SWAP_MAP_SHMEM) {
> > - /*
> > - * Or we could insist on shmem.c using a special
> > - * swap_shmem_free() and free_shmem_swap_and_cache()...
> > - */
> > - count = 0;
> > } else if ((count & ~COUNT_CONTINUED) <= SWAP_MAP_MAX) {
> > if (count == COUNT_CONTINUED) {
> > if (swap_count_continued(si, offset, count))
> > @@ -3626,7 +3620,6 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage, int nr)
> >
> > offset = swp_offset(entry);
> > VM_WARN_ON(nr > SWAPFILE_CLUSTER - offset % SWAPFILE_CLUSTER);
> > - VM_WARN_ON(usage == 1 && nr > 1);
> > ci = lock_cluster_or_swap_info(si, offset);
> >
> > err = 0;
> > @@ -3652,6 +3645,13 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage, int nr)
> > err = -EEXIST;
> > } else if ((count & ~COUNT_CONTINUED) > SWAP_MAP_MAX) {
> > err = -EINVAL;
> > + } else {
> > + /*
> > + * The only swap_duplicate_nr() caller that passes nr > 1 is shmem,
> > + * who never re-duplicates any swap entry it owns. So this should
>
> nit: I think "which" is the right word here, but I am not a native speaker :)
Yeah I think it should be which. Fix(let) incoming.
>
> > + * not happen.
> > + */
> > + VM_WARN_ON(nr > 1 && (count & ~COUNT_CONTINUED) == SWAP_MAP_MAX);
>
> Why not return an error in this case? I think we should add recovery
> for bugs when it's possible and simple, which I believe is the case
> here.
>
> In shmem_writepage() we can add a WARN if swap_duplicate_nr() fails,
> or propagate an error to the caller as well (perhaps this belongs in a
> separate patch that does this for swap_shmem_alloc() first).
>
> Sorry if I am being paranoid here, please let me know if this is the case.
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);
>
> > }
> >
> > if (err)
> > @@ -3686,27 +3686,28 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage, int nr)
> > return err;
> > }
> >
> > -/*
> > - * Help swapoff by noting that swap entry belongs to shmem/tmpfs
> > - * (in which case its reference count is never incremented).
> > - */
> > -void swap_shmem_alloc(swp_entry_t entry, int nr)
> > -{
> > - __swap_duplicate(entry, SWAP_MAP_SHMEM, nr);
> > -}
> > -
> > -/*
> > - * Increase reference count of swap entry by 1.
> > +/**
> > + * swap_duplicate_nr() - Increase reference count of nr contiguous swap entries
> > + * by 1.
>
> Can we avoid the line break by using "refcount" instead of "reference count"?
>
> > + *
> > + * @entry: first swap entry from which we want to increase the refcount.
> > + * @nr: Number of entries in range.
> > + *
> > * Returns 0 for success, or -ENOMEM if a swap_count_continuation is required
> > * but could not be atomically allocated. Returns 0, just as if it succeeded,
> > * if __swap_duplicate() fails for another reason (-EINVAL or -ENOENT), which
> > * might occur if a page table entry has got corrupted.
> > + *
> > + * Note that we are currently not handling the case where nr > 1 and we need to
> > + * add swap count continuation. This is OK, because no such user exists - shmem
> > + * is the only user that can pass nr > 1, and it never re-duplicates any swap
> > + * entry it owns.
>
> Do we need this comment when we have the WARN + comment in __swap_duplicate()?
Here I'm just being cautious and include the limitation of the
function in the API documentation itself.
No strong opinions though.
>
> > */
> > -int swap_duplicate(swp_entry_t entry)
> > +int swap_duplicate_nr(swp_entry_t entry, int nr)
> > {
> > int err = 0;
> >
> > - while (!err && __swap_duplicate(entry, 1, 1) == -ENOMEM)
> > + while (!err && __swap_duplicate(entry, 1, nr) == -ENOMEM)
> > err = add_swap_count_continuation(entry, GFP_ATOMIC);
> > return err;
> > }
> > --
> > 2.43.5
Powered by blists - more mailing lists