linux-kernel - Re: [PATCH v3 2/2] mm,hwpoison: make get_hwpoison_page call get_any

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, 12 May 2021 10:55:22 +0200
From:   Oscar Salvador <osalvador@...e.de>
To:     Naoya Horiguchi <nao.horiguchi@...il.com>
Cc:     Muchun Song <songmuchun@...edance.com>, linux-mm@...ck.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Michal Hocko <mhocko@...e.com>,
        Tony Luck <tony.luck@...el.com>,
        Naoya Horiguchi <naoya.horiguchi@....com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] mm,hwpoison: make get_hwpoison_page call
 get_any_page()

On Wed, May 12, 2021 at 12:10:16AM +0900, Naoya Horiguchi wrote:
> From: Naoya Horiguchi <naoya.horiguchi@....com>
> 
> Now __get_hwpoison_page() could return -EBUSY in a race condition,
> so it's helpful to handle it by retrying.  We already have retry
> logic, so make get_hwpoison_page() call get_any_page() when called
> from memory_failure().

As I stated in your previous patch, I think you should start returning -EBUSY
from this patch onwards.

>  static int get_any_page(struct page *p, unsigned long flags)
>  {
>  	int ret = 0, pass = 0;
> @@ -1152,50 +1136,76 @@ static int get_any_page(struct page *p, unsigned long flags)
>  		count_increased = true;
>  
>  try_again:
> -	if (!count_increased && !__get_hwpoison_page(p)) {
> -		if (page_count(p)) {
> -			/* We raced with an allocation, retry. */
> -			if (pass++ < 3)
> -				goto try_again;
> -			ret = -EBUSY;
> -		} else if (!PageHuge(p) && !is_free_buddy_page(p)) {
> -			/* We raced with put_page, retry. */
> -			if (pass++ < 3)
> -				goto try_again;
> -			ret = -EIO;
> +	if (!count_increased) {
> +		ret = __get_hwpoison_page(p);
> +		if (!ret) {
> +			if (page_count(p)) {
> +				/* We raced with an allocation, retry. */
> +				if (pass++ < 3)
> +					goto try_again;
> +				ret = -EBUSY;
> +			} else if (!PageHuge(p) && !is_free_buddy_page(p)) {
> +				/* We raced with put_page, retry. */
> +				if (pass++ < 3)
> +					goto try_again;
> +				ret = -EIO;
> +			}
> +			goto out;
>  		}
> +	}

I think this can be improved.

We cannot have -EBUSY unless we come from __get_hwpoison_page() (!count_increased),
so I think a much more natural approach would be to stuff the hunk below in there,
and then place the other stuff in an else, so something like:

        if (!count_increased) {
                ret = __get_hwpoison_page(p);
                if (!ret) {
                        if (page_count(p)) {
                                /* We raced with an allocation, retry. */
                                if (pass++ < 3)
                                        goto try_again;
                                ret = -EBUSY;
                        } else if (!PageHuge(p) && !is_free_buddy_page(p)) {
                                /* We raced with put_page, retry. */
                                if (pass++ < 3)
                                        goto try_again;
                                ret = -EIO;
                        }
                        goto out;
                } else if (ret == -EBUSY) {
			/* We raced with freeing huge page to buddy, retry. */
			if (pass++ < 3)
				goto try_again;
		}
        } else {
		/* We do already have a refcount for this page, see if we can
		 * handle it.
		 */
		if (PageHuge(p) || PageLRU(p) || __PageMovable(p)) {
			ret = 1;
		} else {
			/* A page we cannot handle. Check...
		}
	}

Other than that, looks good to me.

> +
> +	if (ret == -EBUSY) {
> +		/* We raced with freeing huge page to buddy, retry. */
> +		if (pass++ < 3)
> +			goto try_again;

-- 
Oscar Salvador
SUSE L3