linux-kernel - Re: [v3 PATCH 3/5] mm: hwpoison: refactor refcount check handling

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: <YV4c1dOfctEMnH2s@t490s>
Date:   Wed, 6 Oct 2021 18:01:57 -0400
From:   Peter Xu <peterx@...hat.com>
To:     Yang Shi <shy828301@...il.com>
Cc:     naoya.horiguchi@....com, hughd@...gle.com,
        kirill.shutemov@...ux.intel.com, willy@...radead.org,
        osalvador@...e.de, akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [v3 PATCH 3/5] mm: hwpoison: refactor refcount check handling

On Thu, Sep 30, 2021 at 02:53:09PM -0700, Yang Shi wrote:
> +/*
> + * Return true if page is still referenced by others, otherwise return
> + * false.
> + *
> + * The dec is true when one extra refcount is expected.
> + */
> +static bool has_extra_refcount(struct page_state *ps, struct page *p,
> +			       bool dec)

Nit: would it be nicer to keep using things like "extra_pins", so we pass in 1
for swapcache dirty case and 0 for the rest?  Then it'll also match with most
of the similar cases in e.g. huge_memory.c (please try grep "extra_pins" there).

> +{
> +	int count = page_count(p) - 1;
> +
> +	if (dec)
> +		count -= 1;
> +
> +	if (count > 0) {
> +		pr_err("Memory failure: %#lx: %s still referenced by %d users\n",
> +		       page_to_pfn(p), action_page_types[ps->type], count);
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  /*
>   * Error hit kernel page.
>   * Do nothing, try to be lucky and not touch this instead. For a few cases we
>   * could be more sophisticated.
>   */
> -static int me_kernel(struct page *p, unsigned long pfn)
> +static int me_kernel(struct page_state *ps, struct page *p)

Not sure whether it's intended, but some of the action() hooks do not call the
refcount check now while in the past they'll all do.  Just to double check
they're expected, like this one and me_unknown().

>  {
>  	unlock_page(p);
>  	return MF_IGNORED;
> @@ -820,9 +852,9 @@ static int me_kernel(struct page *p, unsigned long pfn)
>  /*
>   * Page in unknown state. Do nothing.
>   */
> -static int me_unknown(struct page *p, unsigned long pfn)
> +static int me_unknown(struct page_state *ps, struct page *p)
>  {
> -	pr_err("Memory failure: %#lx: Unknown page state\n", pfn);
> +	pr_err("Memory failure: %#lx: Unknown page state\n", page_to_pfn(p));
>  	unlock_page(p);
>  	return MF_FAILED;
>  }

Thanks,

-- 
Peter Xu