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: <20150626171829.4b8651ed@BR9TG4T3.de.ibm.com>
Date:	Fri, 26 Jun 2015 17:18:29 +0200
From:	Dominik Dingel <dingel@...ux.vnet.ibm.com>
To:	nick <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 Fri, 26 Jun 2015 10:47:39 -0400
nick <xerofoify@...il.com> wrote:

> 
> 
> On 2015-06-26 09:56 AM, Dominik Dingel wrote:
> > 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!
> > 
> There is only one caller that needs to be moved over if this case is put in. Further more 
> do we care if executing rmap_walk fails as if it does this means we were unable to execute
> the function page_referenced one on the rmap_walk_control structure rwc and this can be
> a issue in my option, if not then we can just remove the ret variable and execute rmap_walk
> without checking it's return value.
> Cheers Nick 

Let me rephrase: what will happen after you return ret?
What will: 
- shrink_active_list 
- page_check_references 
now do?

For your second thinking it would be good to check how and why ret was introduced?

git log -L '/int page_referenced(/',/^}/:mm/rmap.c

--
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/