linux-kernel - Re: [PATCH 19/24] shmem, swap: refactor error check on OOM or race

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: <CAF8kJuMJMBjn2VGq8aUzv_t4+k0zPW8WvJaraYe-8GRbX0q1fg@mail.gmail.com>
Date:   Sun, 19 Nov 2023 23:04:02 -0800
From:   Chris Li <chrisl@...nel.org>
To:     Kairui Song <kasong@...cent.com>
Cc:     linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
        "Huang, Ying" <ying.huang@...el.com>,
        David Hildenbrand <david@...hat.com>,
        Hugh Dickins <hughd@...gle.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Matthew Wilcox <willy@...radead.org>,
        Michal Hocko <mhocko@...e.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 19/24] shmem, swap: refactor error check on OOM or race

Hi Kairui,

On Sun, Nov 19, 2023 at 11:49 AM Kairui Song <ryncsn@...il.com> wrote:
>
> From: Kairui Song <kasong@...cent.com>
>
> It should always check if a swap entry is already swaped in on error,
> fix potential false error issue.
>
> Signed-off-by: Kairui Song <kasong@...cent.com>
> ---
>  mm/shmem.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 81d129aa66d1..6154b5b68b8f 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1857,13 +1857,11 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
>         page = swapin_page_non_fault(swap, gfp, mpol, ilx, fault_mm, &result);
>         mpol_cond_put(mpol);
>
> -       if (PTR_ERR(page) == -EBUSY) {
> -               if (!shmem_confirm_swap(mapping, index, swap))
> -                       return -EEXIST;

Do you intentionally remove checking shmem_confirm_swap()?
I am not sure I am following.

> +       if (IS_ERR_OR_NULL(page)) {
> +               if (!page)
> +                       error = -ENOMEM;
>                 else
> -                       return -EINVAL;
> -       } else if (!page) {
> -               error = -ENOMEM;
> +                       error = -EINVAL;

The resulting code is a bit hard to read in diff. In plan source it is like:

        if (IS_ERR_OR_NULL(page)) {
                if (!page)
                        error = -ENOMEM;
                else
                        error = -EINVAL;
                goto failed;
        } else {
                folio = page_folio(page);
                if (fault_type && result != SWAP_CACHE_HIT) {
                        *fault_type |= VM_FAULT_MAJOR;
                        count_vm_event(PGMAJFAULT);
                        count_memcg_event_mm(fault_mm, PGMAJFAULT);
                }
        }

First of all, if the first always "goto failed", the second else is not needed.
The whole else block can be flatten one indentation.

 if (IS_ERR_OR_NULL(page)) {
                if (!page)
                        error = -ENOMEM;
                else
                        error = -EINVAL;
                goto failed;
  } else {

Can be rewrite as following with less indentation:

if (!page) {
     error = -ENOMEM;
     goto failed;
}
if (IS_ERR(page)) {
     error = -EINVAL;
     goto failed;
}
/* else block */

Am I missing something and misreading your code?

Chris