linux-kernel - Re: [PATCH] mm: Add error check after call to rmap_walk in the function page

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>] [day] [month] [year] [list]

Message-ID: <20150626155614.04bffed1@BR9TG4T3.de.ibm.com>
Date:	Fri, 26 Jun 2015 15:56:14 +0200
From:	Dominik Dingel <dingel@...ux.vnet.ibm.com>
To:	Nicholas Krause <xerofoify@...il.com>
Cc:	akpm@...ux-foundation.org, mhocko@...e.cz, riel@...hat.com,
	kirill.shutemov@...ux.intel.com, hannes@...xchg.org,
	dave@...olabs.net, koct9i@...il.com, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: Add error check after call to rmap_walk in the
 function page_referenced

On Thu, 25 Jun 2015 21:36:37 -0400
Nicholas Krause <xerofoify@...il.com> wrote:

> This adds a return check after the call to the function rmap_walk
> in the function page_referenced as this function call can fail
> and thus should signal callers of page_referenced if this happens
> by returning the SWAP macro return value as returned by rmap_walk
> here. In addition also check if have locked the page pointer as
> passed to this particular and unlock it with unlock_page if this
> page is locked before returning our SWAP marco return code from
> rmap_walk.
> 
> Signed-off-by: Nicholas Krause <xerofoify@...il.com>
> ---
>  mm/rmap.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 171b687..e4df848 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -814,7 +814,9 @@ static bool invalid_page_referenced_vma(struct vm_area_struct *vma, void *arg)
>   * @vm_flags: collect encountered vma->vm_flags who actually referenced the page
>   *
>   * Quick test_and_clear_referenced for all mappings to a page,
> - * returns the number of ptes which referenced the page.
> + * returns the number of ptes which referenced the page.On
> + * error returns either zero or the error code returned from
> + * the failed call to rmap_walk.
>   */
>  int page_referenced(struct page *page,
>  		    int is_locked,
> @@ -855,7 +857,13 @@ int page_referenced(struct page *page,
>  		rwc.invalid_vma = invalid_page_referenced_vma;
>  	}
> 
> +

unnecessary empty line

>  	ret = rmap_walk(page, &rwc);
> +	if (!ret) {
> +		if (we_locked)
> +			unlock_page(page);
> +		return ret;
> +	}

I don't see why the function should propagate the rmap_walk return value.
rmap_walk will not set pra.referenced, so that both callers just skip.

What is the purpose of the given patch? Do you have any real case introducing such code,
which is imho incomplete as all callers need to take care of the changed return value!

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/