From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752065AbdKVRrI (ORCPT ); Wed, 22 Nov 2017 12:47:08 -0500 Received: from userp1040.oracle.com ([156.151.31.81]:35658 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751472AbdKVRrG (ORCPT ); Wed, 22 Nov 2017 12:47:06 -0500 Subject: Re: [PATCH 1/1] mm/cma: fix alloc_contig_range ret code/potential leak To: Johannes Weiner Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Joonsoo Kim , Vlastimil Babka , Michal Nazarewicz , Michal Hocko , Mel Gorman , Andrew Morton , stable@vger.kernel.org References: <20171120193930.23428-1-mike.kravetz@oracle.com> <20171120193930.23428-2-mike.kravetz@oracle.com> <20171122120002.GA27270@cmpxchg.org> From: Mike Kravetz Message-ID: <15cf0f39-43f9-8287-fcfe-f2502af59e8a@oracle.com> Date: Wed, 22 Nov 2017 09:46:35 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171122120002.GA27270@cmpxchg.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Source-IP: aserv0021.oracle.com [141.146.126.233] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/22/2017 04:00 AM, Johannes Weiner wrote: > On Mon, Nov 20, 2017 at 11:39:30AM -0800, Mike Kravetz wrote: >> If the call __alloc_contig_migrate_range() in alloc_contig_range >> returns -EBUSY, processing continues so that test_pages_isolated() >> is called where there is a tracepoint to identify the busy pages. >> However, it is possible for busy pages to become available between >> the calls to these two routines. In this case, the range of pages >> may be allocated. Unfortunately, the original return code (ret >> == -EBUSY) is still set and returned to the caller. Therefore, >> the caller believes the pages were not allocated and they are leaked. >> >> Update the return code with the value from test_pages_isolated(). >> >> Fixes: 8ef5849fa8a2 ("mm/cma: always check which page caused allocation failure") >> Cc: >> Signed-off-by: Mike Kravetz > > Wow, good catch. > >> --- >> mm/page_alloc.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >> index 77e4d3c5c57b..3605ca82fd29 100644 >> --- a/mm/page_alloc.c >> +++ b/mm/page_alloc.c >> @@ -7632,10 +7632,10 @@ int alloc_contig_range(unsigned long start, unsigned long end, >> } >> >> /* Make sure the range is really isolated. */ >> - if (test_pages_isolated(outer_start, end, false)) { >> + ret = test_pages_isolated(outer_start, end, false); >> + if (ret) { >> pr_info_ratelimited("%s: [%lx, %lx) PFNs busy\n", >> __func__, outer_start, end); >> - ret = -EBUSY; >> goto done; > > Essentially, an -EBUSY from __alloc_contig_migrate_range() doesn't > mean anything, and we return 0 if the rest of the operations succeed. > > Since we never plan on returning that particular -EBUSY, would it be > more robust to reset it right then and there, rather than letting it > run on in ret for more than a screenful? > > It would also be good to note in that fall-through comment that the > pages becoming free on their own is a distinct possibility. > > As Michal points out, this is really subtle. It makes sense to make it > as explicit as possible. Ok, I thought about zero'ing ret right after the call to __alloc_contig_migrate_range and return of -EBUSY. It just didn't look right to me. But, you are correct. We should make this as explicit as possible. I will respin the patch as suggested and be sure to include an explicit comment when setting ret = 0. -- Mike Kravetz From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [PATCH 1/1] mm/cma: fix alloc_contig_range ret code/potential leak To: Johannes Weiner Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Joonsoo Kim , Vlastimil Babka , Michal Nazarewicz , Michal Hocko , Mel Gorman , Andrew Morton , stable@vger.kernel.org References: <20171120193930.23428-1-mike.kravetz@oracle.com> <20171120193930.23428-2-mike.kravetz@oracle.com> <20171122120002.GA27270@cmpxchg.org> From: Mike Kravetz Message-ID: <15cf0f39-43f9-8287-fcfe-f2502af59e8a@oracle.com> Date: Wed, 22 Nov 2017 09:46:35 -0800 MIME-Version: 1.0 In-Reply-To: <20171122120002.GA27270@cmpxchg.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: On 11/22/2017 04:00 AM, Johannes Weiner wrote: > On Mon, Nov 20, 2017 at 11:39:30AM -0800, Mike Kravetz wrote: >> If the call __alloc_contig_migrate_range() in alloc_contig_range >> returns -EBUSY, processing continues so that test_pages_isolated() >> is called where there is a tracepoint to identify the busy pages. >> However, it is possible for busy pages to become available between >> the calls to these two routines. In this case, the range of pages >> may be allocated. Unfortunately, the original return code (ret >> == -EBUSY) is still set and returned to the caller. Therefore, >> the caller believes the pages were not allocated and they are leaked. >> >> Update the return code with the value from test_pages_isolated(). >> >> Fixes: 8ef5849fa8a2 ("mm/cma: always check which page caused allocation failure") >> Cc: >> Signed-off-by: Mike Kravetz > > Wow, good catch. > >> --- >> mm/page_alloc.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >> index 77e4d3c5c57b..3605ca82fd29 100644 >> --- a/mm/page_alloc.c >> +++ b/mm/page_alloc.c >> @@ -7632,10 +7632,10 @@ int alloc_contig_range(unsigned long start, unsigned long end, >> } >> >> /* Make sure the range is really isolated. */ >> - if (test_pages_isolated(outer_start, end, false)) { >> + ret = test_pages_isolated(outer_start, end, false); >> + if (ret) { >> pr_info_ratelimited("%s: [%lx, %lx) PFNs busy\n", >> __func__, outer_start, end); >> - ret = -EBUSY; >> goto done; > > Essentially, an -EBUSY from __alloc_contig_migrate_range() doesn't > mean anything, and we return 0 if the rest of the operations succeed. > > Since we never plan on returning that particular -EBUSY, would it be > more robust to reset it right then and there, rather than letting it > run on in ret for more than a screenful? > > It would also be good to note in that fall-through comment that the > pages becoming free on their own is a distinct possibility. > > As Michal points out, this is really subtle. It makes sense to make it > as explicit as possible. Ok, I thought about zero'ing ret right after the call to __alloc_contig_migrate_range and return of -EBUSY. It just didn't look right to me. But, you are correct. We should make this as explicit as possible. I will respin the patch as suggested and be sure to include an explicit comment when setting ret = 0. -- Mike Kravetz -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org