linux-kernel - Re: [PATCH v2 3/3] mm: remove superfluous _

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:   Wed, 19 Aug 2020 16:06:32 -0700
From:   Yang Shi <shy828301@...il.com>
To:     Yu Zhao <yuzhao@...gle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Alexander Duyck <alexander.h.duyck@...ux.intel.com>,
        Huang Ying <ying.huang@...el.com>,
        David Hildenbrand <david@...hat.com>,
        Michal Hocko <mhocko@...e.com>,
        Yang Shi <yang.shi@...ux.alibaba.com>, Qian Cai <cai@....pw>,
        Mel Gorman <mgorman@...e.de>,
        Nicholas Piggin <npiggin@...il.com>,
        Jérôme Glisse <jglisse@...hat.com>,
        Hugh Dickins <hughd@...gle.com>, Linux MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Joonsoo Kim <iamjoonsoo.kim@....com>
Subject: Re: [PATCH v2 3/3] mm: remove superfluous __ClearPageWaiters()

On Tue, Aug 18, 2020 at 11:47 AM Yu Zhao <yuzhao@...gle.com> wrote:
>
> Presumably __ClearPageWaiters() was added to follow the previously
> removed __ClearPageActive() pattern.
>
> Only flags that are in PAGE_FLAGS_CHECK_AT_FREE needs to be properly
> cleared because otherwise we think there may be some kind of leak.
> PG_waiters is not one of those flags and leaving the clearing to
> PAGE_FLAGS_CHECK_AT_PREP is more appropriate.

Actually TBH I'm not very keen to this change, it seems the clearing
is just moved around and the allocation side pays for that instead of
free side.

>
> Signed-off-by: Yu Zhao <yuzhao@...gle.com>
> ---
>  include/linux/page-flags.h | 2 +-
>  mm/filemap.c               | 2 ++
>  mm/memremap.c              | 2 --
>  mm/swap.c                  | 3 ---
>  4 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 6be1aa559b1e..dba80a2bdfba 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -318,7 +318,7 @@ static inline int TestClearPage##uname(struct page *page) { return 0; }
>         TESTSETFLAG_FALSE(uname) TESTCLEARFLAG_FALSE(uname)
>
>  __PAGEFLAG(Locked, locked, PF_NO_TAIL)
> -PAGEFLAG(Waiters, waiters, PF_ONLY_HEAD) __CLEARPAGEFLAG(Waiters, waiters, PF_ONLY_HEAD)
> +PAGEFLAG(Waiters, waiters, PF_ONLY_HEAD)
>  PAGEFLAG(Error, error, PF_NO_TAIL) TESTCLEARFLAG(Error, error, PF_NO_TAIL)
>  PAGEFLAG(Referenced, referenced, PF_HEAD)
>         TESTCLEARFLAG(Referenced, referenced, PF_HEAD)
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 1aaea26556cc..75240c7ef73f 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1079,6 +1079,8 @@ static void wake_up_page_bit(struct page *page, int bit_nr)
>                  * other pages on it.
>                  *
>                  * That's okay, it's a rare case. The next waker will clear it.
> +                * Otherwise the bit will be cleared by PAGE_FLAGS_CHECK_AT_PREP
> +                * when the page is being freed.
>                  */
>         }
>         spin_unlock_irqrestore(&q->lock, flags);
> diff --git a/mm/memremap.c b/mm/memremap.c
> index 3a06eb91cb59..a9d02ffaf9e3 100644
> --- a/mm/memremap.c
> +++ b/mm/memremap.c
> @@ -451,8 +451,6 @@ void free_devmap_managed_page(struct page *page)
>                 return;
>         }
>
> -       __ClearPageWaiters(page);
> -
>         mem_cgroup_uncharge(page);
>
>         /*
> diff --git a/mm/swap.c b/mm/swap.c
> index 999a84dbe12c..40bf20a75278 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -90,7 +90,6 @@ static void __page_cache_release(struct page *page)
>                 del_page_from_lru_list(page, lruvec, page_off_lru(page));
>                 spin_unlock_irqrestore(&pgdat->lru_lock, flags);
>         }
> -       __ClearPageWaiters(page);
>  }
>
>  static void __put_single_page(struct page *page)
> @@ -900,8 +899,6 @@ void release_pages(struct page **pages, int nr)
>                         del_page_from_lru_list(page, lruvec, page_off_lru(page));
>                 }
>
> -               __ClearPageWaiters(page);
> -
>                 list_add(&page->lru, &pages_to_free);
>         }
>         if (locked_pgdat)
> --
> 2.28.0.220.ged08abb693-goog
>
>